Message-Id: <20250322.102449.895174336060649075.fujita.tomonori@gmail.com>
Date: Sat, 22 Mar 2025 10:24:49 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: frederic@...nel.org
Cc: fujita.tomonori@...il.com, linux-kernel@...r.kernel.org,
 daniel.almeida@...labora.com, gary@...yguo.net, aliceryhl@...gle.com,
 me@...enk.dev, rust-for-linux@...r.kernel.org, netdev@...r.kernel.org,
 andrew@...n.ch, hkallweit1@...il.com, tmgross@...ch.edu, ojeda@...nel.org,
 alex.gaynor@...il.com, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...sung.com, anna-maria@...utronix.de, tglx@...utronix.de,
 arnd@...db.de, jstultz@...gle.com, sboyd@...nel.org, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, vschneid@...hat.com, tgunders@...hat.com,
 david.laight.linux@...il.com
Subject: Re: [PATCH v11 5/8] rust: time: Add wrapper for fsleep() function

On Fri, 21 Mar 2025 23:05:23 +0100
Frederic Weisbecker <frederic@...nel.org> wrote:

> Le Thu, Feb 20, 2025 at 04:06:07PM +0900, FUJITA Tomonori a écrit :
>> Add a wrapper for fsleep(), flexible sleep functions in
>> include/linux/delay.h which typically deals with hardware delays.
>> 
>> The kernel supports several sleep functions to handle various lengths
>> of delay. This adds fsleep(), automatically chooses the best sleep
>> method based on a duration.
>> 
>> sleep functions including fsleep() belongs to TIMERS, not
>> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
>> abstraction for TIMEKEEPING. To make Rust abstractions match the C
>> side, add rust/kernel/time/delay.rs for this wrapper.
>> 
>> fsleep() can only be used in a nonatomic context. This requirement is
>> not checked by these abstractions, but it is intended that klint [1]
>> or a similar tool will be used to check it in the future.
>> 
>> Link: https://rust-for-linux.com/klint [1]
>> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
>> Reviewed-by: Gary Guo <gary@...yguo.net>
>> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
>> Reviewed-by: Fiona Behrens <me@...enk.dev>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> 
> Sorry to make a late review. I don't mean to delay that any further
> but:

No problem at all. Thanks for reviewing!


>> +/// `delta` must be within `[0, i32::MAX]` microseconds;
>> +/// otherwise, it is erroneous behavior. That is, it is considered a bug
>> +/// to call this function with an out-of-range value, in which case the function
>> +/// will sleep for at least the maximum value in the range and may warn
>> +/// in the future.
>> +///
>> +/// The behavior above differs from the C side [`fsleep()`] for which out-of-range
>> +/// values mean "infinite timeout" instead.
> 
> And very important: the behaviour also differ in that the C side takes
> usecs while this takes nsecs. We should really disambiguate the situation
> as that might create confusion or misusage.
> 
> Either this should be renamed to fsleep_ns() or fsleep_nsecs(), or this should
> take microseconds directly.

You meant that `Delta` type internally tracks time in nanoseconds?

It's true but Delta type is a unit-agnostic time abstraction, designed
to represent durations across different granularities — seconds,
milliseconds, microseconds, nanoseconds. The Rust abstraction always
tries to us Delta type to represent durations.

Rust's fsleep takes Delta, internally converts it in usecs, and calls
C's fsleep.

Usually, drivers convert from a certain time unit to Delta before
calling fsleep like the following, so misuse or confusion is unlikely
to occur, I think.

fsleep(Delta::from_micros(50));

However, as you pointed out, there is a difference; C's fsleep takes
usecs while Rust's fsleep takes a unit-agnostic time type. Taking this
difference into account, if we were to rename fsleep for Rust, I think
that a name that is agnostic to the time unit would seem more
appropriate. Simply sleep(), perhaps?

Powered by blists - more mailing lists