[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <D8ZVBUTCJXYZ.1X3I8HOSTXIA7@proton.me>
Date: Sun, 06 Apr 2025 21:20:04 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alexandre Courbot <acourbot@...dia.com>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Danilo Krummrich <dakr@...nel.org>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Bjorn Helgaas <bhelgaas@...gle.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rust/revocable: add try_access_with() convenience method
On Sun Apr 6, 2025 at 3:58 PM CEST, Alexandre Courbot wrote:
> Revocable::try_access() returns a guard through which the wrapped object
> can be accessed. Code that can sleep is not allowed while the guard is
> held; thus, it is common for the caller to explicitly drop it before
> running sleepable code, e.g:
>
> let b = bar.try_access()?;
> let reg = b.readl(...);
>
> // Don't forget this or things could go wrong!
> drop(b);
>
> something_that_might_sleep();
>
> let b = bar.try_access()?;
> let reg2 = b.readl(...);
>
> This is arguably error-prone. try_access_with() provides an arguably
> safer alternative, by taking a closure that is run while the guard is
> held, and by dropping the guard automatically after the closure
> completes. This way, code can be organized more clearly around the
> critical sections and the risk of forgetting to release the guard when
> needed is considerably reduced:
>
> let reg = bar.try_access_with(|b| b.readl(...))?;
>
> something_that_might_sleep();
>
> let reg2 = bar.try_access_with(|b| b.readl(...))?;
>
> The closure can return nothing, or any value including a Result which is
> then wrapped inside the Option returned by try_access_with. Error
> management is driver-specific, so users are encouraged to create their
> own macros that map and flatten the returned values to something
> appropriate for the code they are working on.
>
> Acked-by: Danilo Krummrich <dakr@...nel.org>
> Suggested-by: Danilo Krummrich <dakr@...nel.org>
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
> ---
> rust/kernel/revocable.rs | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 1e5a9d25c21b279b01f90b02997492aa4880d84f..b91e40e8160be0cc0ff8e0699e48e063c9dbce22 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -123,6 +123,22 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
> }
> }
>
> + /// Tries to access the wrapped object and run a closure on it while the guard is held.
> + ///
> + /// This is a convenience method to run short non-sleepable code blocks while ensuring the
> + /// guard is dropped afterwards. [`Self::try_access`] carries the risk that the caller will
> + /// forget to explicitly drop that returned guard before calling sleepable code; this method
> + /// adds an extra safety to make sure it doesn't happen.
> + ///
> + /// Returns `None` if the object has been revoked and is therefore no longer accessible, or the
> + /// result of the closure wrapped in `Some`. If the closure returns a [`Result`] then the
> + /// return type becomes `Option<Result<>>`, which can be inconvenient. Users are encouraged to
> + /// define their own macro that turns the `Option` into a proper error code and flattens the
> + /// inner result into it if it makes sense within their subsystem.
I personally wouldn't have mentioned this in the docs, since to me such
a helper would be obvious, but I don't mind it either.
---
Cheers,
Benno
> + pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> + self.try_access().map(|t| f(&*t))
> + }
> +
> /// # Safety
> ///
> /// Callers must ensure that there are no more concurrent users of the revocable object.
Powered by blists - more mailing lists