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

[Alice Ryhl]

On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>
> From: Wedson Almeida Filho <wedsonaf@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Co-developed-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>

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?

> +            },
> +        })
> +    }
> +
> +    /// 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.

Alice