| Message ID | 20250406-try_with-v3-1-c0947842e768@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust/revocable: add try_access_with() convenience method | expand |
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@kernel.org> > Suggested-by: Danilo Krummrich <dakr@kernel.org> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> Reviewed-by: Benno Lossin <benno.lossin@proton.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.
On Mon Apr 7, 2025 at 6:20 AM JST, Benno Lossin wrote: > 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@kernel.org> >> Suggested-by: Danilo Krummrich <dakr@kernel.org> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > Reviewed-by: Benno Lossin <benno.lossin@proton.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. Using a helper did not immediately occur to me, which is why I came with two different methods initially. I'm fine with removing this part of the doc if it sounds like it is overstepping the purpose of documentation tough.
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. + 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.
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@kernel.org> Suggested-by: Danilo Krummrich <dakr@kernel.org> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- rust/kernel/revocable.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)