Message-ID: <Zv6FkGIMoh6PTdKY@boqun-archlinux>
Date: Thu, 3 Oct 2024 04:52:48 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: dirk.behme@...bosch.com, andrew@...n.ch, aliceryhl@...gle.com,
	netdev@...r.kernel.org, rust-for-linux@...r.kernel.org,
	hkallweit1@...il.com, tmgross@...ch.edu, ojeda@...nel.org,
	alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.com
Subject: Re: iopoll abstraction

On Wed, Oct 02, 2024 at 09:56:36AM +0000, FUJITA Tomonori wrote:
> On Wed, 2 Oct 2024 06:39:48 +0200
> Dirk Behme <dirk.behme@...bosch.com> wrote:
> 
> >> I generally point developers at iopoll.h, because developers nearly
> >> always get this sort of polling for something to happen wrong.
> >> The kernel sleep functions guarantee the minimum sleep time. They say
> >> nothing about the maximum sleep time. You can ask it to sleep for 1ms,
> >> and in reality, due to something stealing the CPU and not being RT
> >> friendly, it actually sleeps for 10ms. This extra long sleep time
> >> blows straight past your timeout, if you have a time based timeout.
> >> What most developers do is after the sleep() returns they check to see
> >> if the timeout has been reached and then exit with -ETIMEDOUT. They
> >> don't check the state of the hardware, which given its had a long time
> >> to do its thing, probably is now in a good state. But the function
> >> returns -ETIMEDOUT.
> >> There should always be a check of the hardware state after the sleep,
> >> in order to determine ETIMEDOUT vs 0.
> >> As i said, most C developers get this wrong. So i don't really see why
> >> Rust developers also will not get this wrong. So i like to discourage
> >> this sort of code, and have Rust implementations of iopoll.h.
> > 
> > 
> > Do we talk about some simple Rust wrappers for the macros in iopoll.h?
> > E.g. something like [1]?
> > 
> > Or are we talking about some more complex (safety) dependencies which
> > need some more complex abstraction handling?
> 
> (snip)
> 
> > int rust_helper_readb_poll_timeout(const volatile void * addr,
> >                                   u64 val, u64 cond, u64 delay_us,
> >                                   u64 timeout_us)
> > {
> >        return readb_poll_timeout(addr, val, cond, delay_us, timeout_us);
> > }
> 
> I'm not sure a simple wrapper for iopoll.h works. We need to pass a
> function. I'm testing a macro like the following (not added ktime
> timeout yet):

You could use closure as a parameter to avoid macro interface, something
like:

	fn read_poll_timeout<Op, Cond, T>(
	    op: Op,
	    cond: Cond,
	    sleep: Delta,
	    timeout: Delta,
	) -> Result<T> where
	    Op: Fn() -> T,
	    cond: Fn() -> bool {

	    let __timeout = kernel::Ktime::ktime_get() + timeout;

	    let val = loop {
		let val = op();
		if cond() {
		    break Some(val);
		}
		kernel::delay::sleep(sleep);

		if __timeout.after(kernel::Ktime::ktime_get()) {
		    break None;
		}
	    };

	    if cond() {
		val
	    } else {
		Err(kernel::error::code::ETIMEDOUT)
	    }
	}

note that you don't need the args part, because `op` is a closure that
can capature value, so for example, if in C code you need to call foo(a,
b), with closure, you can do:

	<a and b are defined in the caller>
	read_poll_timeout(|| { foo(a, b) }, ...);

with above API.

Regards,
Boqun

> 
> macro_rules! read_poll_timeout {
>     ($op:expr, $val:expr, $cond:expr, $sleep:expr, $timeout:expr, $($args:expr),*) => {{
>         let _ = $val;
>         loop {
>             $val = $op($($args),*);
>             if $cond {
>                 break;
>             }
>             kernel::delay::sleep($sleep);
>         }
>         if $cond {
>             Ok(())
>         } else {
>             Err(kernel::error::code::ETIMEDOUT)
>         }
>     }};
> }
> 

Powered by blists - more mailing lists