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

["Benno Lossin" <lossin@kernel.org>]

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::`.

---
Cheers,
Benno

> +            // requirements of this function, no thread is accessing `data` anymore.
> +            unsafe { drop_in_place(self.data.get()) };
> +        }
>      }
>  
>      /// Revokes access to and drops the wrapped object.
> @@ -165,9 +151,15 @@ pub unsafe fn revoke_nosync(&self) {
>      /// [`Revocable::try_access`] beforehand and still haven't dropped the returned guard), this
>      /// function waits for the concurrent access to complete before dropping the wrapped object.
>      pub fn revoke(&self) {
> -        // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
> -        // finish.
> -        unsafe { self.revoke_internal::<true>() }
> +        if self.is_available.swap(false, Ordering::Relaxed) {
> +            // SAFETY: Just an FFI call, there are no further requirements.
> +            unsafe { bindings::synchronize_rcu() };
> +
> +            // 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 `synchronize_rcu`
> +            // ensures all prior RCU read-side critical sections have completed.
> +            unsafe { drop_in_place(self.data.get()) };
> +        }
>      }
>  }
>