Re: [PATCH v11 5/8] rust: time: Add wrapper for fsleep() function

[FUJITA Tomonori]

On Fri, 21 Mar 2025 23:05:23 +0100
Frederic Weisbecker <frederic@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Reviewed-by: Gary Guo <gary@xxxxxxxxxxx>
>> Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
>> Reviewed-by: Fiona Behrens <me@xxxxxxxxxx>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
> 
> 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?