On Wed, 22 Jan 2025 09:23:33 +0100
Alice Ryhl [off-list ref] wrote:

quoted

quoted

I would also say "the C side [`fsleep()`] or similar"; in other words,
both are "kernel's" at this point.

Agreed that "the C side" is better and updated the comment. I copied
that expression from the existing code; there are many "kernel's" in
rust/kernel/. "good first issues" for them?

You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
precedent for the C side functions.

I think that's a matter of taste. In the Rust ecosystem, fsleep is
more common, in the kernel ecosystem, fsleep() is more common. I've
seen both in Rust code at this point.

Understood, I'll go with [`fsleep`].

quoted

quoted

And perhaps I would simplify and say something like "The behavior
above differs from the C side [`fsleep()`] for which out-of-range
values mean "infinite timeout" instead."

Yeah, simpler is better. After applying the above changes, it ended up
as follows.

/// Sleeps for a given duration at least.
///
/// Equivalent to the C side [`fsleep`], flexible sleep function,
/// which automatically chooses the best sleep method based on a duration.
///
/// `delta` must be within [0, `i32::MAX`] microseconds;

I'd do `[0, i32::MAX]` instead for better rendering.

Yeah, looks better after redering. I'll update it.

quoted

/// 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.
///
/// This function can only be used in a nonatomic context.
///
/// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
pub fn fsleep(delta: Delta) {

quoted

quoted

A range can be used for a custom type?

I was thinking of doing it through `as_nanos()`, but it may read
worse, so please ignore it if so.

Ah, it might work. The following doesn't work. Seems that we need to
add another const like MAX_DELTA_NANOS or something. No strong
preference but I feel the current is simpler.

let delta = match delta.as_nanos() {
    0..=MAX_DELTA.as_nanos() as i32 => delta,
    _ => MAX_DELTA,
};

Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?

We need Delta type here so you meant:

let delta = std::cmp::min(delta, MAX_DELTA);

?

We also need to convert a negative delta to MAX_DELTA so we could do:

let delta = if delta.is_negative() {
    MAX_DELTA
} else {
    min(delta, MAX_DELTA)
};

looks a bit readable than the original code?