Re: [PATCH v4 05/13] rust: add `Revocable` type

[Danilo Krummrich <dakr@kernel.org>]

On Fri, Dec 06, 2024 at 04:11:39PM +0100, Alice Ryhl wrote:
> On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > From: Wedson Almeida Filho <wedsonaf@gmail.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@gmail.com>
> > Co-developed-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> Overall looks reasonable, but some comments below.
> 
> > +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 `ptr` is fully initialized; on error
> > +            // `ptr` is not partially initialized and does not need to be dropped.
> > +            data <- unsafe {
> > +                Opaque::try_ffi_init(|ptr: *mut T| {
> > +                    init::PinInit::<T, core::convert::Infallible>::__pinned_init(data, ptr)
> > +                })
> 
> This is pretty awkward ... could we have an Opaque::pin_init that
> takes an `impl PinInit instead of using fii_init?

Using ffi_init was your suggestion. :) But I agree, having Opaque::pin_init
would be more convenient. I can add a patch for that.

> 
> > +            },
> > +        })
> > +    }
> > +
> > +    /// 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) {
> > +            // 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(RevocableGuard::new(self.data.get(), guard))
> > +        } else {
> > +            None
> > +        }
> > +    }
> > +
> > +    /// Tries to access the revocable wrapped object.
> > +    ///
> > +    /// 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.get() })
> > +        } else {
> > +            None
> > +        }
> > +    }
> > +
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that there are no more concurrent users of the revocable object.
> > +    unsafe fn revoke_internal(&self, sync: bool) {
> 
> This boolean could be a const generic to enforce that it must be a
> compile-time value.

Agreed.

> 
> Alice