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

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 23, 2025 at 10:42:58AM +0200, Benno Lossin wrote:
> On Fri May 23, 2025 at 2:13 AM CEST, Marcelo Moreira wrote:
> > 3. Clarified revoke_internal for SYNC = false and swap correction
> > Proposed Documentation:
> >  if self.is_available.swap(false, Ordering::Relaxed) {
> >  if SYNC {
> >                 // SAFETY: Just an FFI call, there are no further requirements.
> >                 unsafe { bindings::synchronize_rcu() };
> 
> @Boqun: is this true?
> 
> If the answer is yes, then we should add this as a safe function in the
> rcu module.

I think it's a case for Klint, since synchronize_rcu() must not be called from
atomic context, since it may block. Otherwise, there shouldn't be any additional
requirements.

> >  }  else {
> >        // This branch for `revoke_nosync` requires the caller to prove
> > that `data`
> >        // can be dropped immediately without waiting for any RCU grace period.
> 
> I'm not sure that having a single function that does the revocation, but
> has this going on is a good idea. The safety requirements will be pretty
> complex.
> 
> @Danilo what do you think of inlining this function?

Sure, if it makes documentation significantly easier, which seems to be the
case, then it's probably worth.

> > 4. Documented RevocableGuard<'_, T> Invariants and PhantomData Adjustment
> > Proposed Documentation:
> > /// # Invariants
> > ///
> > /// - The RCU read-side lock is held for the lifetime of this guard.
> > /// - `data_ref` points to valid data for the lifetime of this guard.
> > pub struct RevocableGuard<'a, T> {
> >     data_ref: *const T,
> >     _rcu_guard: rcu::Guard,
> >     _p: PhantomData<&'a T>,
> > }
> 
> I think we can change this type to:
> 
>     pub struct RevocableGuard<'a, T> {
>         data: &'a T,
>         _rcu_guard: rcu::Guard,
>     }
> 
> And then we don't need any invariants :)

Agreed, let's make this change in a separate patch please.