| Message ID | 20241016035214.2229-8-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Add read_poll_timeout functions which poll periodically until a > condition is met or a timeout is reached. > > C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro > and a simple wrapper for Rust doesn't work. So this implements the > same functionality in Rust. > > The C version uses usleep_range() while the Rust version uses > fsleep(), which uses the best sleep method so it works with spans that > usleep_range() doesn't work nicely with. > > Unlike the C version, __might_sleep() is used instead of might_sleep() > to show proper debug info; the file name and line > number. might_resched() could be added to match what the C version > does but this function works without it. > > For the proper debug info, readx_poll_timeout() is implemented as a > macro. > > readx_poll_timeout() 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. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > Link: https://rust-for-linux.com/klint [1] > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Duplicated SOB? This should just be: Link: https://rust-for-linux.com/klint [1] Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > +/// Polls periodically until a condition is met or a timeout is reached. > +/// > +/// Public but hidden since it should only be used from public macros. > +#[doc(hidden)] > +pub fn read_poll_timeout<Op, Cond, T: Copy>( > + mut op: Op, > + cond: Cond, > + sleep_delta: Delta, > + timeout_delta: Delta, > + sleep_before_read: bool, > +) -> Result<T> > +where > + Op: FnMut() -> Result<T>, > + Cond: Fn(T) -> bool, > +{ > + let timeout = Ktime::ktime_get() + timeout_delta; > + let sleep = !sleep_delta.is_zero(); > + > + if sleep_before_read && sleep { > + fsleep(sleep_delta); > + } You always pass `false` for `sleep_before_read` so perhaps just remove this and the argument entirely? > + if cond(val) { > + break val; > + } This breaks out to another cond(val) check below. Perhaps just `return Ok(val)` here to avoid the double condition check? > + if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout { > + break op()?; > + } Shouldn't you just return `Err(ETIMEDOUT)` here? I don't think you're supposed to call `op` twice without a sleep in between. > + if sleep { > + fsleep(sleep_delta); > + } > + // SAFETY: FFI call. > + unsafe { bindings::cpu_relax() } Should cpu_relax() be in an else branch? Also, please add a safe wrapper function around cpu_relax. > +macro_rules! readx_poll_timeout { > + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ > + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] > + if !$sleep_delta.is_zero() { > + // SAFETY: FFI call. > + unsafe { > + $crate::bindings::__might_sleep( > + ::core::file!().as_ptr() as *const i8, > + ::core::line!() as i32, > + ) > + } > + } It could be nice to introduce a might_sleep macro that does this internally? Then we can reuse this logic in other places. Alice
On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > +/// Polls periodically until a condition is met or a timeout is reached. > +/// > +/// `op` is called repeatedly until `cond` returns `true` or the timeout is > +/// reached. The return value of `op` is passed to `cond`. > +/// > +/// `sleep_delta` is the duration to sleep between calls to `op`. > +/// If `sleep_delta` is less than one microsecond, the function will busy-wait. > +/// > +/// `timeout_delta` is the maximum time to wait for `cond` to return `true`. > +/// > +/// This macro can only be used in a nonatomic context. > +#[macro_export] > +macro_rules! readx_poll_timeout { > + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ > + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] > + if !$sleep_delta.is_zero() { > + // SAFETY: FFI call. > + unsafe { > + $crate::bindings::__might_sleep( > + ::core::file!().as_ptr() as *const i8, > + ::core::line!() as i32, > + ) > + } > + } I wonder if we can use #[track_caller] and core::panic::Location::caller [1] to do this without having to use a macro? I don't know whether it would work, but if it does, that would be super cool. #[track_caller] fn might_sleep() { let location = core::panic::Location::caller(); // SAFETY: FFI call. unsafe { $crate::bindings::__might_sleep( location.file().as_char_ptr(), location.line() as i32, ) } } Alice [1]: https://doc.rust-lang.org/nightly/core/panic/struct.Location.html#method.caller
On Wed, Oct 16, 2024 at 10:45 AM Alice Ryhl <aliceryhl@google.com> wrote: > > On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > +/// Polls periodically until a condition is met or a timeout is reached. > > +/// > > +/// `op` is called repeatedly until `cond` returns `true` or the timeout is > > +/// reached. The return value of `op` is passed to `cond`. > > +/// > > +/// `sleep_delta` is the duration to sleep between calls to `op`. > > +/// If `sleep_delta` is less than one microsecond, the function will busy-wait. > > +/// > > +/// `timeout_delta` is the maximum time to wait for `cond` to return `true`. > > +/// > > +/// This macro can only be used in a nonatomic context. > > +#[macro_export] > > +macro_rules! readx_poll_timeout { > > + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ > > + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] > > + if !$sleep_delta.is_zero() { > > + // SAFETY: FFI call. > > + unsafe { > > + $crate::bindings::__might_sleep( > > + ::core::file!().as_ptr() as *const i8, > > + ::core::line!() as i32, > > + ) > > + } > > + } > > I wonder if we can use #[track_caller] and > core::panic::Location::caller [1] to do this without having to use a > macro? I don't know whether it would work, but if it does, that would > be super cool. > > #[track_caller] > fn might_sleep() { > let location = core::panic::Location::caller(); > // SAFETY: FFI call. > unsafe { > $crate::bindings::__might_sleep( > location.file().as_char_ptr(), > location.line() as i32, > ) > } > } Actually, this raises a problem ... core::panic::Location doesn't give us a nul-terminated string, so we probably can't pass it to `__might_sleep`. The thing is, `::core::file!()` doesn't give us a nul-terminated string either, so this code is probably incorrect as-is. Alice
On Wed, Oct 16, 2024 at 10:52 AM Alice Ryhl <aliceryhl@google.com> wrote: > > `__might_sleep`. The thing is, `::core::file!()` doesn't give us a > nul-terminated string either, so this code is probably incorrect > as-is. Yeah, elsewhere we use `c_str!`. It would be nice to get `core::c_file!` too, though that one is less critical since we can do it ourselves. Perhaps we should also consider using Clippy's `disallowed_macros` too if we don't expect people to often need the normal one. Added to our list: https://github.com/Rust-for-Linux/linux/issues/514 Cheers, Miguel
On Wed, 16 Oct 2024 10:52:17 +0200 Alice Ryhl <aliceryhl@google.com> wrote: > On Wed, Oct 16, 2024 at 10:45 AM Alice Ryhl <aliceryhl@google.com> wrote: >> >> On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori >> <fujita.tomonori@gmail.com> wrote: >> > +/// Polls periodically until a condition is met or a timeout is reached. >> > +/// >> > +/// `op` is called repeatedly until `cond` returns `true` or the timeout is >> > +/// reached. The return value of `op` is passed to `cond`. >> > +/// >> > +/// `sleep_delta` is the duration to sleep between calls to `op`. >> > +/// If `sleep_delta` is less than one microsecond, the function will busy-wait. >> > +/// >> > +/// `timeout_delta` is the maximum time to wait for `cond` to return `true`. >> > +/// >> > +/// This macro can only be used in a nonatomic context. >> > +#[macro_export] >> > +macro_rules! readx_poll_timeout { >> > + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ >> > + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] >> > + if !$sleep_delta.is_zero() { >> > + // SAFETY: FFI call. >> > + unsafe { >> > + $crate::bindings::__might_sleep( >> > + ::core::file!().as_ptr() as *const i8, >> > + ::core::line!() as i32, >> > + ) >> > + } >> > + } >> >> I wonder if we can use #[track_caller] and >> core::panic::Location::caller [1] to do this without having to use a >> macro? I don't know whether it would work, but if it does, that would >> be super cool. Seems it works, no need to use macro. Thanks a lot! >> #[track_caller] >> fn might_sleep() { >> let location = core::panic::Location::caller(); >> // SAFETY: FFI call. >> unsafe { >> $crate::bindings::__might_sleep( >> location.file().as_char_ptr(), >> location.line() as i32, >> ) >> } >> } > > Actually, this raises a problem ... core::panic::Location doesn't give > us a nul-terminated string, so we probably can't pass it to > `__might_sleep`. The thing is, `::core::file!()` doesn't give us a > nul-terminated string either, so this code is probably incorrect > as-is. Ah, what's the recommended way to get a null-terminated string from &str?
On Fri, Oct 18, 2024 at 10:10 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Wed, 16 Oct 2024 10:52:17 +0200 > Alice Ryhl <aliceryhl@google.com> wrote: > > > On Wed, Oct 16, 2024 at 10:45 AM Alice Ryhl <aliceryhl@google.com> wrote: > >> > >> On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori > >> <fujita.tomonori@gmail.com> wrote: > >> > +/// Polls periodically until a condition is met or a timeout is reached. > >> > +/// > >> > +/// `op` is called repeatedly until `cond` returns `true` or the timeout is > >> > +/// reached. The return value of `op` is passed to `cond`. > >> > +/// > >> > +/// `sleep_delta` is the duration to sleep between calls to `op`. > >> > +/// If `sleep_delta` is less than one microsecond, the function will busy-wait. > >> > +/// > >> > +/// `timeout_delta` is the maximum time to wait for `cond` to return `true`. > >> > +/// > >> > +/// This macro can only be used in a nonatomic context. > >> > +#[macro_export] > >> > +macro_rules! readx_poll_timeout { > >> > + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ > >> > + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] > >> > + if !$sleep_delta.is_zero() { > >> > + // SAFETY: FFI call. > >> > + unsafe { > >> > + $crate::bindings::__might_sleep( > >> > + ::core::file!().as_ptr() as *const i8, > >> > + ::core::line!() as i32, > >> > + ) > >> > + } > >> > + } > >> > >> I wonder if we can use #[track_caller] and > >> core::panic::Location::caller [1] to do this without having to use a > >> macro? I don't know whether it would work, but if it does, that would > >> be super cool. > > Seems it works, no need to use macro. Thanks a lot! > > >> #[track_caller] > >> fn might_sleep() { > >> let location = core::panic::Location::caller(); > >> // SAFETY: FFI call. > >> unsafe { > >> $crate::bindings::__might_sleep( > >> location.file().as_char_ptr(), > >> location.line() as i32, > >> ) > >> } > >> } > > > > Actually, this raises a problem ... core::panic::Location doesn't give > > us a nul-terminated string, so we probably can't pass it to > > `__might_sleep`. The thing is, `::core::file!()` doesn't give us a > > nul-terminated string either, so this code is probably incorrect > > as-is. > > Ah, what's the recommended way to get a null-terminated string from > &str? In this case, you should be able to use the `c_str!` macro. `kernel::c_str!(core::file!()).as_char_ptr()` Alice
> > Ah, what's the recommended way to get a null-terminated string from > > &str? > > In this case, you should be able to use the `c_str!` macro. > > `kernel::c_str!(core::file!()).as_char_ptr()` Does this allocate memory? In this case, that would be O.K, but at some point i expect somebody is going to want the atomic version of this poll helper. You then need to pass additional flags to kalloc() if you call it in atomic context. Andrew
On Wed, 16 Oct 2024 10:37:46 +0200 Alice Ryhl <aliceryhl@google.com> wrote: > On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> Add read_poll_timeout functions which poll periodically until a >> condition is met or a timeout is reached. >> >> C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro >> and a simple wrapper for Rust doesn't work. So this implements the >> same functionality in Rust. >> >> The C version uses usleep_range() while the Rust version uses >> fsleep(), which uses the best sleep method so it works with spans that >> usleep_range() doesn't work nicely with. >> >> Unlike the C version, __might_sleep() is used instead of might_sleep() >> to show proper debug info; the file name and line >> number. might_resched() could be added to match what the C version >> does but this function works without it. >> >> For the proper debug info, readx_poll_timeout() is implemented as a >> macro. >> >> readx_poll_timeout() 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. >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> >> Link: https://rust-for-linux.com/klint [1] >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > Duplicated SOB? This should just be: > > Link: https://rust-for-linux.com/klint [1] > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Oops, I'll fix. >> +/// Polls periodically until a condition is met or a timeout is reached. >> +/// >> +/// Public but hidden since it should only be used from public macros. >> +#[doc(hidden)] >> +pub fn read_poll_timeout<Op, Cond, T: Copy>( >> + mut op: Op, >> + cond: Cond, >> + sleep_delta: Delta, >> + timeout_delta: Delta, >> + sleep_before_read: bool, >> +) -> Result<T> >> +where >> + Op: FnMut() -> Result<T>, >> + Cond: Fn(T) -> bool, >> +{ >> + let timeout = Ktime::ktime_get() + timeout_delta; >> + let sleep = !sleep_delta.is_zero(); >> + >> + if sleep_before_read && sleep { >> + fsleep(sleep_delta); >> + } > > You always pass `false` for `sleep_before_read` so perhaps just remove > this and the argument entirely? Most of users of C's this function use false buf some use true. It would be better to provide this option? Would be better to provide only read_poll_timeout() which takes the sleep_before_read argument?; No readx_poll_timeout wrapper. >> + if cond(val) { >> + break val; >> + } > > This breaks out to another cond(val) check below. Perhaps just `return > Ok(val)` here to avoid the double condition check? I think that that's how the C version works. But `return Ok(val)` here is fine, I guess. >> + if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout { >> + break op()?; >> + } > > Shouldn't you just return `Err(ETIMEDOUT)` here? I don't think you're > supposed to call `op` twice without a sleep in between. I think that it's how the C version works. >> + if sleep { >> + fsleep(sleep_delta); >> + } >> + // SAFETY: FFI call. >> + unsafe { bindings::cpu_relax() } > > Should cpu_relax() be in an else branch? I think that we could. I'll remove if you prefer. The C version always calls cpu_relax(). I think that the following commit explains why: commit b407460ee99033503993ac7437d593451fcdfe44 Author: Geert Uytterhoeven <geert+renesas@glider.be> Date: Fri Jun 2 10:50:36 2023 +0200 iopoll: Call cpu_relax() in busy loops > Also, please add a safe wrapper function around cpu_relax. Sure, which file do you think is best to add the wrapper to? rust/kernel/processor.rs ? >> +macro_rules! readx_poll_timeout { >> + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ >> + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] >> + if !$sleep_delta.is_zero() { >> + // SAFETY: FFI call. >> + unsafe { >> + $crate::bindings::__might_sleep( >> + ::core::file!().as_ptr() as *const i8, >> + ::core::line!() as i32, >> + ) >> + } >> + } > > It could be nice to introduce a might_sleep macro that does this > internally? Then we can reuse this logic in other places. I think that with #[track_caller], we can use a normal function instead of a macro. Using might_sleep name for a wrapper of __might_sleep is confusing since the C side has also might_sleep. __foo function name is acceptable?
> + // SAFETY: FFI call. > + unsafe { > + $crate::bindings::__might_sleep( > + ::core::file!().as_ptr() as *const i8, > + ::core::line!() as i32, > + ) Can this be pulled out into an easy to share macro? I expect to see this to be scattered in a number of files, so making it easy to use would be good. Andrew
On Fri, Oct 18, 2024 at 4:15 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Does this allocate memory? In this case, that would be O.K, but at No, it does not -- it is all done at compile-time. Cheers, Miguel
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index c274546bcf78..f9569ff1717e 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -12,6 +12,7 @@ #include "build_assert.c" #include "build_bug.c" #include "err.c" +#include "kernel.c" #include "kunit.c" #include "mutex.c" #include "page.c" diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c new file mode 100644 index 000000000000..da847059260b --- /dev/null +++ b/rust/helpers/kernel.c @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/kernel.h> + +void rust_helper_cpu_relax(void) +{ + cpu_relax(); +} + +void rust_helper___might_sleep(const char *file, int line) +{ + __might_sleep(file, line); +} diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 6f1587a2524e..d571b9587ed6 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -58,6 +58,7 @@ macro_rules! declare_err { declare_err!(EPIPE, "Broken pipe."); declare_err!(EDOM, "Math argument out of domain of func."); declare_err!(ERANGE, "Math result not representable."); + declare_err!(ETIMEDOUT, "Connection timed out."); declare_err!(ERESTARTSYS, "Restart the system call."); declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted."); declare_err!(ERESTARTNOHAND, "Restart if no handler."); diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs new file mode 100644 index 000000000000..033f3c4e4adf --- /dev/null +++ b/rust/kernel/io.rs @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Input and Output. + +pub mod poll; diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs new file mode 100644 index 000000000000..d4bb791f665b --- /dev/null +++ b/rust/kernel/io/poll.rs @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! IO polling. +//! +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h). + +use crate::{ + bindings, + error::{code::*, Result}, + time::{delay::fsleep, Delta, Ktime}, +}; + +/// Polls periodically until a condition is met or a timeout is reached. +/// +/// Public but hidden since it should only be used from public macros. +#[doc(hidden)] +pub fn read_poll_timeout<Op, Cond, T: Copy>( + mut op: Op, + cond: Cond, + sleep_delta: Delta, + timeout_delta: Delta, + sleep_before_read: bool, +) -> Result<T> +where + Op: FnMut() -> Result<T>, + Cond: Fn(T) -> bool, +{ + let timeout = Ktime::ktime_get() + timeout_delta; + let sleep = !sleep_delta.is_zero(); + + if sleep_before_read && sleep { + fsleep(sleep_delta); + } + + let val = loop { + let val = op()?; + if cond(val) { + break val; + } + if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout { + break op()?; + } + if sleep { + fsleep(sleep_delta); + } + // SAFETY: FFI call. + unsafe { bindings::cpu_relax() } + }; + + if cond(val) { + Ok(val) + } else { + Err(ETIMEDOUT) + } +} + +/// Polls periodically until a condition is met or a timeout is reached. +/// +/// `op` is called repeatedly until `cond` returns `true` or the timeout is +/// reached. The return value of `op` is passed to `cond`. +/// +/// `sleep_delta` is the duration to sleep between calls to `op`. +/// If `sleep_delta` is less than one microsecond, the function will busy-wait. +/// +/// `timeout_delta` is the maximum time to wait for `cond` to return `true`. +/// +/// This macro can only be used in a nonatomic context. +#[macro_export] +macro_rules! readx_poll_timeout { + ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{ + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)] + if !$sleep_delta.is_zero() { + // SAFETY: FFI call. + unsafe { + $crate::bindings::__might_sleep( + ::core::file!().as_ptr() as *const i8, + ::core::line!() as i32, + ) + } + } + + $crate::io::poll::read_poll_timeout($op, $cond, $sleep_delta, $timeout_delta, false) + }}; +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index b5f4b3ce6b48..d5fd6aeb24ca 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -35,6 +35,7 @@ #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] pub mod firmware; pub mod init; +pub mod io; pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit;