Re: [PATCH v2] rust: doc: Clarify safety invariants for Revocable type

[Danilo Krummrich <dakr@kernel.org>]

On Mon, May 19, 2025 at 01:10:32PM +0200, Benno Lossin wrote:
> On Mon May 19, 2025 at 11:55 AM CEST, Danilo Krummrich wrote:
> > On Mon, May 19, 2025 at 11:18:42AM +0200, Benno Lossin wrote:
> > Why not? Please show me a case where `is_available` is false, but I can still
> > technically access data (without violating a safety requirement).
> 
>     let r: Arc<Revocable<i32>> = ...;
>     let guard = r.try_access().unwrap(); // nobody else is holding a reference, so this can't fail
> 
>     let r2 = r.clone();
> 
>     // I know we don't have threads, but I don't want to have to look up
>     // how to use the workqueue or something else...
>     thread::spawn(move || {
>         r2.revoke();
>     });
> 
>     for _ in 0..10_000_000 {
>         // do some non-sleeping work that takes a while
>     }
> 
>     // now the thread above has executed `self.is_available.swap(false, Ordering::Relaxed)`
>     // in `revoke_internal` and is waiting for the `synchronize_rcu` call to return.
>     // but we can still access `guard`:
> 
>     pr_info!("{}", &*guard);

Which is perfectly correct, you're right. I think I was too focused on the
optimization case. :-)

> > However, this invariant does not need to be fulfilled for access() and
> 
> Where is `access()` defined?

https://gitlab.freedesktop.org/drm/nova/-/commit/46f91addfabbd4109fb64876a032ae4a4a924919

> > revoke_nosync(), because it would circumvent their purpose, i.e. cases where an
> > abstraction can prove that there can't be a concurrent user of the data or a
> > concurrent user revoking the data respectively.
> 
> Yes. How about something like "`data` is valid while `is_available` is
> true. It also is valid if the RCU read-side lock is being held and it
> was taken while `is_available` was true."?
> 
> That should also cover the "nobody else is accessing this" case.

Sounds good to me!

> > An example of revoke_nosync() is the original Devres implementation [1].
> > However, this was re-worked with [2] to use a different logic that doesn't need
> > revoke_nosync() anymore.
> >
> > Actually, "doesn't need revoke_nosync()" isn't exactly true. We would still
> > benefit from revoke_nosync(), but the implementation in [2] triggers the devres
> > C callback from drop() and the devres C callback calls revoke().
> >
> > If we'd had a way to know that the devres C callback has been triggered due
> > drop(), we could use revoke_nosync() in this case as an optimization.
> 
> Yeah that sounds like a plausible option. Given that, I think the
> following kind of function could be useful on `Revocable`: a safe
> `revoke_` function that takes `&mut self` and thus doesn't need to use
> RCU (since we have a unique mutable reference, only we have access).
> 
> Do you have any other uses of `revoke_nosync` that do not have
> (potential) access to `&mut Revocable`?

I could imagine abstractions that use Revocable with some external lock
protecting the data for instance. But this could probably be solved otherwise
with LockedBy.

> > [1] commit 76c01ded724b ("rust: add devres abstraction")
> > [2] commit 8ff656643d30 ("rust: devres: remove action in `Devres::drop`")