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@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?
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
