Re: [PATCH v4 3/3] rust: revocable: split revoke_internal into revoke and revoke_nosync

Marcelo Moreira
2025-06-12T19:30:10Z

Em qui., 12 de jun. de 2025 às 06:07, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> > This commit refactors the revocation mechanism by removing the generic
> > `revoke_internal` function. Its logic is now directly integrated into
> > two distinct public functions: `revoke()` and `revoke_nosync()`.
> >
> > `revoke_nosync()` is an `unsafe` function that requires the caller to
> > guarantee no concurrent users, thus avoiding an RCU grace period.
> > `revoke()` is a safe function that internally waits for the RCU grace
> > period to ensure all concurrent accesses have completed before dropping
> > the wrapped object.
> >
> > This change improves API clarity and simplifies associated `SAFETY`
> > comments by making the synchronization behavior explicit in the function
> > signatures.
> >
> > Suggested-by: Benno Lossin <lossin@kernel.org>
> > Suggested-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>
> One comment below, with that fixed:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
> > ---
> > rust/kernel/revocable.rs | 38 +++++++++++++++-----------------------
> > 1 file changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 43cc9bdc94f4..daf22e3a7d20 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -126,22 +126,6 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
> > }
> > }
> >
> > - /// # Safety
> > - ///
> > - /// Callers must ensure that there are no more concurrent users of the revocable object.
> > - unsafe fn revoke_internal<const SYNC: bool>(&self) {
> > - if self.is_available.swap(false, Ordering::Relaxed) {
> > - if SYNC {
> > - // SAFETY: Just an FFI call, there are no further requirements.
> > - unsafe { bindings::synchronize_rcu() };
> > - }
> > -
> > - // SAFETY: We know `self.data` is valid because only one CPU can succeed the
> > - // `compare_exchange` above that takes `is_available` from `true` to `false`.
> > - unsafe { drop_in_place(self.data.get()) };
> > - }
> > - }
> > -
> > /// Revokes access to and drops the wrapped object.
> > ///
> > /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`],
> > @@ -151,10 +135,12 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) {
> > ///
> > /// Callers must ensure that there are no more concurrent users of the revocable object.
> > pub unsafe fn revoke_nosync(&self) {
> > - // SAFETY: By the safety requirement of this function, the caller ensures that nobody is
> > - // accessing the data anymore and hence we don't have to wait for the grace period to
> > - // finish.
> > - unsafe { self.revoke_internal::<false>() }
> > + if self.is_available.swap(false, Ordering::Relaxed) {
> > + // SAFETY: `Self::data` is valid for writes because of `Self`'s type invariants,
> > + // as `Self::is_available` is false due to the atomic swap, and by the safety
>
> Please also use `self.data`/`self.is_available` here (& below) instead
> of `Self::`.

ok, Thanks! :D

Cheers,
Marcelo Moreira