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

Benno Lossin
2025-06-12T09:07:02Z

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()) };
> + }
> }
> }
>