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

["Benno Lossin" <lossin@kernel.org>]

On Mon May 19, 2025 at 1:37 PM CEST, Danilo Krummrich wrote:
> 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. :-)

And I was assuming the example was obvious with just saying "But the
`is_available` atomic *can* be altered during the usage of `data`." :-)

>> > However, this invariant does not need to be fulfilled for access() and
>> 
>> Where is `access()` defined?
>
> https://gitlab.freedesktop.org/drm/nova/-/commit/46f91addfabbd4109fb64876a032ae4a4a924919

"Reviewed-by: Benno Lossin <benno.lossin@proton.me>" huh, I already
forgot about the patch... But that also shouldn't be difficult to
support with the right invariant.

>> > 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!

I'm not happy with the sentence structure, so how about:

* `data` is valid for reads in two cases:
    * while `is_available` is true, or
    * while the RCU read-side lock is taken and it was acquired while `is_available` was `true`.
* `data` is valid for writes when `is_available` was atomically changed from `true` to `false`.

The last one is needed in order to call `drop_in_place`.

---
Cheers,
Benno