Message-ID: <CAH5fLgjcy=DQrCYt-k40D4_NcwgdrykUW9d74srGn5hxxo2Xmw@mail.gmail.com>
Date: Tue, 29 Oct 2024 14:26:55 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, 
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, 
	tmgross@...ch.edu, a.hindborg@...sung.com, airlied@...il.com, 
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com, 
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org, 
	daniel.almeida@...labora.com, saravanak@...gle.com, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	Wedson Almeida Filho <wedsonaf@...il.com>
Subject: Re: [PATCH v3 07/16] rust: add `Revocable` type

On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> From: Wedson Almeida Filho <wedsonaf@...il.com>
>
> Revocable allows access to objects to be safely revoked at run time.
>
> This is useful, for example, for resources allocated during device probe;
> when the device is removed, the driver should stop accessing the device
> resources even if another state is kept in memory due to existing
> references (i.e., device context data is ref-counted and has a non-zero
> refcount after removal of the device).
>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>  rust/kernel/lib.rs       |   1 +
>  rust/kernel/revocable.rs | 211 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 212 insertions(+)
>  create mode 100644 rust/kernel/revocable.rs
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 89f6bd2efcc0..b603b67dcd71 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -54,6 +54,7 @@
>  pub mod prelude;
>  pub mod print;
>  pub mod rbtree;
> +pub mod revocable;
>  pub mod sizes;
>  mod static_assert;
>  #[doc(hidden)]
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> new file mode 100644
> index 000000000000..83455558d795
> --- /dev/null
> +++ b/rust/kernel/revocable.rs
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Revocable objects.
> +//!
> +//! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
> +//! of a [`RevocableGuard`] ensures that objects remain valid.
> +
> +use crate::{
> +    bindings,
> +    init::{self},
> +    prelude::*,
> +    sync::rcu,
> +};
> +use core::{
> +    cell::UnsafeCell,
> +    marker::PhantomData,
> +    mem::MaybeUninit,
> +    ops::Deref,
> +    ptr::drop_in_place,
> +    sync::atomic::{AtomicBool, Ordering},
> +};
> +
> +/// An object that can become inaccessible at runtime.
> +///
> +/// Once access is revoked and all concurrent users complete (i.e., all existing instances of
> +/// [`RevocableGuard`] are dropped), the wrapped object is also dropped.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// # use kernel::revocable::Revocable;
> +///
> +/// struct Example {
> +///     a: u32,
> +///     b: u32,
> +/// }
> +///
> +/// fn add_two(v: &Revocable<Example>) -> Option<u32> {
> +///     let guard = v.try_access()?;
> +///     Some(guard.a + guard.b)
> +/// }
> +///
> +/// let v = KBox::pin_init(Revocable::new(Example { a: 10, b: 20 }), GFP_KERNEL).unwrap();
> +/// assert_eq!(add_two(&v), Some(30));
> +/// v.revoke();
> +/// assert_eq!(add_two(&v), None);
> +/// ```
> +///
> +/// Sample example as above, but explicitly using the rcu read side lock.
> +///
> +/// ```
> +/// # use kernel::revocable::Revocable;
> +/// use kernel::sync::rcu;
> +///
> +/// struct Example {
> +///     a: u32,
> +///     b: u32,
> +/// }
> +///
> +/// fn add_two(v: &Revocable<Example>) -> Option<u32> {
> +///     let guard = rcu::read_lock();
> +///     let e = v.try_access_with_guard(&guard)?;
> +///     Some(e.a + e.b)
> +/// }
> +///
> +/// let v = KBox::pin_init(Revocable::new(Example { a: 10, b: 20 }), GFP_KERNEL).unwrap();
> +/// assert_eq!(add_two(&v), Some(30));
> +/// v.revoke();
> +/// assert_eq!(add_two(&v), None);
> +/// ```
> +#[pin_data(PinnedDrop)]
> +pub struct Revocable<T> {
> +    is_available: AtomicBool,
> +    #[pin]
> +    data: MaybeUninit<UnsafeCell<T>>,
> +}
> +
> +// SAFETY: `Revocable` is `Send` if the wrapped object is also `Send`. This is because while the
> +// functionality exposed by `Revocable` can be accessed from any thread/CPU, it is possible that
> +// this isn't supported by the wrapped object.
> +unsafe impl<T: Send> Send for Revocable<T> {}
> +
> +// SAFETY: `Revocable` is `Sync` if the wrapped object is both `Send` and `Sync`. We require `Send`
> +// from the wrapped object as well because  of `Revocable::revoke`, which can trigger the `Drop`
> +// implementation of the wrapped object from an arbitrary thread.
> +unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
> +
> +impl<T> Revocable<T> {
> +    /// Creates a new revocable instance of the given data.
> +    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
> +        pin_init!(Self {
> +            is_available: AtomicBool::new(true),
> +            // SAFETY: The closure only returns `Ok(())` if `slot` is fully initialized; on error
> +            // `slot` is not partially initialized and does not need to be dropped.
> +            data <- unsafe {
> +                init::pin_init_from_closure(move |slot: *mut MaybeUninit<UnsafeCell<T>>| {
> +                    init::PinInit::<T, core::convert::Infallible>::__pinned_init(data,
> +                                                                                 slot as *mut T)?;
> +                    Ok::<(), core::convert::Infallible>(())
> +                })

If you change `data` to be `Opaque`, then this can just be

data <- Opaque::ffi_init(data)

(or maybe you need try_ffi_init)

> +
> +    /// Tries to access the \[revocable\] wrapped object.
> +    ///
> +    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
> +    ///
> +    /// Returns a guard that gives access to the object otherwise; the object is guaranteed to
> +    /// remain accessible while the guard is alive. In such cases, callers are not allowed to sleep
> +    /// because another CPU may be waiting to complete the revocation of this object.
> +    pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
> +        let guard = rcu::read_lock();
> +        if self.is_available.load(Ordering::Relaxed) {
> +            // SAFETY: Since `self.is_available` is true, data is initialised and has to remain
> +            // valid because the RCU read side lock prevents it from being dropped.
> +            Some(unsafe { RevocableGuard::new(self.data.assume_init_ref().get(), guard) })
> +        } else {
> +            None
> +        }
> +    }
> +
> +    /// Tries to access the \[revocable\] wrapped object.

These backslashes seem wrong.

> +    /// Returns `None` if the object has been revoked and is therefore no longer accessible.
> +    ///
> +    /// Returns a shared reference to the object otherwise; the object is guaranteed to
> +    /// remain accessible while the rcu read side guard is alive. In such cases, callers are not
> +    /// allowed to sleep because another CPU may be waiting to complete the revocation of this
> +    /// object.
> +    pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a T> {
> +        if self.is_available.load(Ordering::Relaxed) {
> +            // SAFETY: Since `self.is_available` is true, data is initialised and has to remain
> +            // valid because the RCU read side lock prevents it from being dropped.
> +            Some(unsafe { &*self.data.assume_init_ref().get() })
> +        } else {
> +            None
> +        }
> +    }
> +
> +    /// Revokes access to and drops the wrapped object.
> +    ///
> +    /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`]. If
> +    /// there are concurrent users of the object (i.e., ones that called [`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) {
> +        if self
> +            .is_available
> +            .compare_exchange(true, false, Ordering::Relaxed, Ordering::Relaxed)
> +            .is_ok()
> +        {
> +            // 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.assume_init_ref().get()) };
> +        }
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T> PinnedDrop for Revocable<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // Drop only if the data hasn't been revoked yet (in which case it has already been
> +        // dropped).
> +        // SAFETY: We are not moving out of `p`, only dropping in place
> +        let p = unsafe { self.get_unchecked_mut() };
> +        if *p.is_available.get_mut() {
> +            // SAFETY: We know `self.data` is valid because no other CPU has changed
> +            // `is_available` to `false` yet, and no other CPU can do it anymore because this CPU
> +            // holds the only reference (mutable) to `self` now.
> +            unsafe { drop_in_place(p.data.assume_init_ref().get()) };
> +        }
> +    }
> +}
> +
> +/// A guard that allows access to a revocable object and keeps it alive.
> +///
> +/// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
> +/// holding the RCU read-side lock.
> +///
> +/// # Invariants
> +///
> +/// The RCU read-side lock is held while the guard is alive.
> +pub struct RevocableGuard<'a, T> {
> +    data_ref: *const T,
> +    _rcu_guard: rcu::Guard,
> +    _p: PhantomData<&'a ()>,
> +}

Is this needed? Can't all users just use `try_access_with_guard`?

Alice

Powered by blists - more mailing lists