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

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

[Marcelo Moreira]

The Revocable type in rust/kernel/revocable.rs lacked a comprehensive
documentation of its safety invariants, specifically regarding the
validity of the wrapped data and the necessity of holding the RCU
read-side lock for access. This patch addresses this by:

- Adding an '# Invariants' section to the documentation of `Revocable<T>`
  clarifying that `data` is valid if and only if `is_available` is true,
  and that access to `data` requires holding the RCU read-side lock.
- Adding '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
  to explicitly refer to these invariants before accessing the underlying data.
- Adding an '# Invariants' section to the documentation of `RevocableGuard<'_, T>`
  documenting that the RCU read-side lock is held for the lifetime of the guard
  and that `data_ref` points to valid data during this time.
- Updating the safety comment in the `Deref` implementation of `RevocableGuard`
  to explicitly mention the relevant invariants.

Changes in v2:

- Refined the wording of the invariants in `Revocable<T>` to be more direct
  and address feedback regarding the phrase 'must occur'.
- Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
  as suggested by reviewers.
- Added the missing invariant for `RevocableGuard<'_, T>` regarding the
  validity of `data_ref`.
- Updated the safety comment in the `Deref` implementation of `RevocableGuard`
  to refer to the new invariant.

Reported-by: Benno Lossin <benno.lossin@proton.me>
Closes: https://github.com/Rust-for-Linux/linux/issues/1160
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 rust/kernel/revocable.rs | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 2da3e9460c07..7ef2f34782b4 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -64,12 +64,11 @@
 ///
 /// # Invariants
 ///
-/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
-/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
-///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
-/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
-///   and the object is dropped either after an RCU grace period (in [`revoke`]),
-///   or immediately (in [`revoke_nosync`]).
+/// - `data` is valid if and only if `is_available` is true.
+/// - Access to `data` requires holding the RCU read-side lock.
+/// - Once is_available is set to false, further access to data is disallowed,
+///   and the object is dropped either after an RCU grace period (in [revoke]),
+///   or immediately (in [revoke_nosync]).
 #[pin_data(PinnedDrop)]
 pub struct Revocable<T> {
     is_available: AtomicBool,
@@ -106,8 +105,9 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
     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.
+            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
+            // RCU read-side lock held by `guard` ensures that `self.data` remains valid for
+            // the lifetime of the guard.
             Some(RevocableGuard::new(self.data.get(), guard))
         } else {
             None
@@ -124,8 +124,10 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
     /// 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.
+            // SAFETY:
+            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
+            // RCU read-side lock held by `_guard` ensures that `self.data` remains valid for
+            // the lifetime of the returned reference.
             Some(unsafe { &*self.data.get() })
         } else {
             None
@@ -200,7 +202,8 @@ fn drop(self: Pin<&mut Self>) {
 ///
 /// # Invariants
 ///
-/// The RCU read-side lock is held while the guard is alive.
+/// 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,
@@ -221,8 +224,8 @@ impl<T> Deref for RevocableGuard<'_, T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {
-        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
-        // guaranteed to remain valid.
+        // SAFETY: By the invariant of `Revocable`, `self.data_ref` is valid because the
+        // RCU read-side lock is held for the lifetime of this guard.
         unsafe { &*self.data_ref }
     }
 }
-- 
2.49.0

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

[Benno Lossin]

On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> The Revocable type in rust/kernel/revocable.rs lacked a comprehensive
> documentation of its safety invariants, specifically regarding the
> validity of the wrapped data and the necessity of holding the RCU
> read-side lock for access. This patch addresses this by:
>
> - Adding an '# Invariants' section to the documentation of `Revocable<T>`
>   clarifying that `data` is valid if and only if `is_available` is true,
>   and that access to `data` requires holding the RCU read-side lock.
> - Adding '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
>   to explicitly refer to these invariants before accessing the underlying data.
> - Adding an '# Invariants' section to the documentation of `RevocableGuard<'_, T>`
>   documenting that the RCU read-side lock is held for the lifetime of the guard
>   and that `data_ref` points to valid data during this time.
> - Updating the safety comment in the `Deref` implementation of `RevocableGuard`
>   to explicitly mention the relevant invariants.
>
> Changes in v2:

The changelog should not be part of the commit message, instead place it
below the `---`, but before any file diff. It then will only appear in
the email and not the commit message.

Another thing: could you please CC Danilo Krummrich the next time you
send this patch? I think he should also take a look at this.

>
> - Refined the wording of the invariants in `Revocable<T>` to be more direct
>   and address feedback regarding the phrase 'must occur'.
> - Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
>   as suggested by reviewers.
> - Added the missing invariant for `RevocableGuard<'_, T>` regarding the
>   validity of `data_ref`.
> - Updated the safety comment in the `Deref` implementation of `RevocableGuard`
>   to refer to the new invariant.
>
> Reported-by: Benno Lossin <benno.lossin@proton.me>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1160
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> ---

^^ right here would be the place for the changelog.

>  rust/kernel/revocable.rs | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)

It seems to me that you sent this patch on top of your previous one [1].
Normally, one doesn't do that and instead sends the patch based on a tag
(like `v6.15-rc4`) or the subsystems `-next` branch (so in our case
`rust-next`). So a new version should not rely on any previous one.

[1]: https://lore.kernel.org/all/20250501005726.744027-1-marcelomoreira1905@gmail.com

> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 2da3e9460c07..7ef2f34782b4 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -64,12 +64,11 @@
>  ///
>  /// # Invariants
>  ///
> -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> -///   or immediately (in [`revoke_nosync`]).
> +/// - `data` is valid if and only if `is_available` is true.
> +/// - Access to `data` requires holding the RCU read-side lock.

I'm not sure what the correct wording here should be. The current
wording makes the `revoke_internal` function illegal, as it doesn't hold
the read-side lock, but still accesses `data`.

Maybe @Danilo can help here, but as I understand it, the value in `data`
is valid for as long as the rcu read-side lock is held *and* if
`is_available` was true at some point while holding the lock.

> +/// - Once is_available is set to false, further access to data is disallowed,
> +///   and the object is dropped either after an RCU grace period (in [revoke]),
> +///   or immediately (in [revoke_nosync]).

Several missing `

>  #[pin_data(PinnedDrop)]
>  pub struct Revocable<T> {
>      is_available: AtomicBool,
> @@ -106,8 +105,9 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>      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.
> +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> +            // RCU read-side lock held by `guard` ensures that `self.data` remains valid for
> +            // the lifetime of the guard.
>              Some(RevocableGuard::new(self.data.get(), guard))
>          } else {
>              None
> @@ -124,8 +124,10 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
>      /// 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.
> +            // SAFETY:

Empty SAFETY comment?

> +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> +            // RCU read-side lock held by `_guard` ensures that `self.data` remains valid for
> +            // the lifetime of the returned reference.
>              Some(unsafe { &*self.data.get() })
>          } else {
>              None
> @@ -200,7 +202,8 @@ fn drop(self: Pin<&mut Self>) {
>  ///
>  /// # Invariants
>  ///
> -/// The RCU read-side lock is held while the guard is alive.
> +/// 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.

Please use a bullet point list.

---
Cheers,
Benno

>  pub struct RevocableGuard<'a, T> {
>      data_ref: *const T,
>      _rcu_guard: rcu::Guard,
> @@ -221,8 +224,8 @@ impl<T> Deref for RevocableGuard<'_, T> {
>      type Target = T;
>  
>      fn deref(&self) -> &Self::Target {
> -        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
> -        // guaranteed to remain valid.
> +        // SAFETY: By the invariant of `Revocable`, `self.data_ref` is valid because the
> +        // RCU read-side lock is held for the lifetime of this guard.
>          unsafe { &*self.data_ref }
>      }
>  }

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

[Marcelo Moreira]

Hello guys!

Thank you for the continued feedback =)

Based on your point, I'm revising the `# Invariants` section for `Revocable<T>`
to be more precise about when access to `data` is valid. I'm
considering the following wording:

- data is valid if and only if is_available is true.
- Access to data is valid while the RCU read-side lock is held, ensuring no
concurrent revocation, or within the specific scope of the
revoke_internal function
where the revocation logic guarantees exclusive access before dropping data.
 - Once is_available is set to false, further access to data outside
of the revocation
process is disallowed, and the object is dropped either after an RCU
grace period
(in [revoke]), or immediately (in [revoke_nosync]).

I've attempted to clarify that the RCU read-side lock protects against
concurrent
revocation during normal access, but that `revoke_internal` has its own safety
guarantees that allow access without the lock in that specific context. I
'd appreciate your feedback on this revised wording to ensure it accurately
reflects the intended behavior and safety invariants.

Thank you for your patience and guidance. I will send the next version
of the patch soon.

Best regards,
Marcelo Moreira


Em sex., 9 de mai. de 2025 às 07:10, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> > The Revocable type in rust/kernel/revocable.rs lacked a comprehensive
> > documentation of its safety invariants, specifically regarding the
> > validity of the wrapped data and the necessity of holding the RCU
> > read-side lock for access. This patch addresses this by:
> >
> > - Adding an '# Invariants' section to the documentation of `Revocable<T>`
> >   clarifying that `data` is valid if and only if `is_available` is true,
> >   and that access to `data` requires holding the RCU read-side lock.
> > - Adding '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
> >   to explicitly refer to these invariants before accessing the underlying data.
> > - Adding an '# Invariants' section to the documentation of `RevocableGuard<'_, T>`
> >   documenting that the RCU read-side lock is held for the lifetime of the guard
> >   and that `data_ref` points to valid data during this time.
> > - Updating the safety comment in the `Deref` implementation of `RevocableGuard`
> >   to explicitly mention the relevant invariants.
> >
> > Changes in v2:
>
> The changelog should not be part of the commit message, instead place it
> below the `---`, but before any file diff. It then will only appear in
> the email and not the commit message.
>
> Another thing: could you please CC Danilo Krummrich the next time you
> send this patch? I think he should also take a look at this.
>
> >
> > - Refined the wording of the invariants in `Revocable<T>` to be more direct
> >   and address feedback regarding the phrase 'must occur'.
> > - Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
> >   as suggested by reviewers.
> > - Added the missing invariant for `RevocableGuard<'_, T>` regarding the
> >   validity of `data_ref`.
> > - Updated the safety comment in the `Deref` implementation of `RevocableGuard`
> >   to refer to the new invariant.
> >
> > Reported-by: Benno Lossin <benno.lossin@proton.me>
> > Closes: https://github.com/Rust-for-Linux/linux/issues/1160
> > Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> > ---
>
> ^^ right here would be the place for the changelog.
>
> >  rust/kernel/revocable.rs | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
>
> It seems to me that you sent this patch on top of your previous one [1].
> Normally, one doesn't do that and instead sends the patch based on a tag
> (like `v6.15-rc4`) or the subsystems `-next` branch (so in our case
> `rust-next`). So a new version should not rely on any previous one.
>
> [1]: https://lore.kernel.org/all/20250501005726.744027-1-marcelomoreira1905@gmail.com
>
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 2da3e9460c07..7ef2f34782b4 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -64,12 +64,11 @@
> >  ///
> >  /// # Invariants
> >  ///
> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> > -///   or immediately (in [`revoke_nosync`]).
> > +/// - `data` is valid if and only if `is_available` is true.
> > +/// - Access to `data` requires holding the RCU read-side lock.
>
> I'm not sure what the correct wording here should be. The current
> wording makes the `revoke_internal` function illegal, as it doesn't hold
> the read-side lock, but still accesses `data`.
>
> Maybe @Danilo can help here, but as I understand it, the value in `data`
> is valid for as long as the rcu read-side lock is held *and* if
> `is_available` was true at some point while holding the lock.
>
> > +/// - Once is_available is set to false, further access to data is disallowed,
> > +///   and the object is dropped either after an RCU grace period (in [revoke]),
> > +///   or immediately (in [revoke_nosync]).
>
> Several missing `
>
> >  #[pin_data(PinnedDrop)]
> >  pub struct Revocable<T> {
> >      is_available: AtomicBool,
> > @@ -106,8 +105,9 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
> >      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.
> > +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> > +            // RCU read-side lock held by `guard` ensures that `self.data` remains valid for
> > +            // the lifetime of the guard.
> >              Some(RevocableGuard::new(self.data.get(), guard))
> >          } else {
> >              None
> > @@ -124,8 +124,10 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
> >      /// 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.
> > +            // SAFETY:
>
> Empty SAFETY comment?
>
> > +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> > +            // RCU read-side lock held by `_guard` ensures that `self.data` remains valid for
> > +            // the lifetime of the returned reference.
> >              Some(unsafe { &*self.data.get() })
> >          } else {
> >              None
> > @@ -200,7 +202,8 @@ fn drop(self: Pin<&mut Self>) {
> >  ///
> >  /// # Invariants
> >  ///
> > -/// The RCU read-side lock is held while the guard is alive.
> > +/// 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.
>
> Please use a bullet point list.
>
> ---
> Cheers,
> Benno
>
> >  pub struct RevocableGuard<'a, T> {
> >      data_ref: *const T,
> >      _rcu_guard: rcu::Guard,
> > @@ -221,8 +224,8 @@ impl<T> Deref for RevocableGuard<'_, T> {
> >      type Target = T;
> >
> >      fn deref(&self) -> &Self::Target {
> > -        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
> > -        // guaranteed to remain valid.
> > +        // SAFETY: By the invariant of `Revocable`, `self.data_ref` is valid because the
> > +        // RCU read-side lock is held for the lifetime of this guard.
> >          unsafe { &*self.data_ref }
> >      }
> >  }
>

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

[Benno Lossin]

On Sat May 17, 2025 at 2:03 AM CEST, Marcelo Moreira wrote:
> Hello guys!
>
> Thank you for the continued feedback =)
>
> Based on your point, I'm revising the `# Invariants` section for `Revocable<T>`
> to be more precise about when access to `data` is valid. I'm
> considering the following wording:
>
> - data is valid if and only if is_available is true.
> - Access to data is valid while the RCU read-side lock is held, ensuring no
> concurrent revocation, or within the specific scope of the
> revoke_internal function
> where the revocation logic guarantees exclusive access before dropping data.

How about we combine these two into:

* `data` is valid for reads if the RCU read-side lock is held and
  `is_available` was true after taking the lock.

>  - Once is_available is set to false, further access to data outside
> of the revocation
> process is disallowed, and the object is dropped either after an RCU
> grace period
> (in [revoke]), or immediately (in [revoke_nosync]).

I wouldn't name the functions, since there might be more added. How
about:
* `is_available` is only set to `false` once; that thread takes
  ownership of `data`.

Let's also see what Danilo thinks.

---
Cheers,
Benno

> I've attempted to clarify that the RCU read-side lock protects against
> concurrent
> revocation during normal access, but that `revoke_internal` has its own safety
> guarantees that allow access without the lock in that specific context. I
> 'd appreciate your feedback on this revised wording to ensure it accurately
> reflects the intended behavior and safety invariants.
>
> Thank you for your patience and guidance. I will send the next version
> of the patch soon.
>
> Best regards,
> Marcelo Moreira

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

[Danilo Krummrich]

On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> >  /// # Invariants
> >  ///
> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> > -///   or immediately (in [`revoke_nosync`]).
> > +/// - `data` is valid if and only if `is_available` is true.
> > +/// - Access to `data` requires holding the RCU read-side lock.
> 
> I'm not sure what the correct wording here should be. The current
> wording makes the `revoke_internal` function illegal, as it doesn't hold
> the read-side lock, but still accesses `data`.
> 
> Maybe @Danilo can help here, but as I understand it, the value in `data`
> is valid for as long as the rcu read-side lock is held *and* if
> `is_available` was true at some point while holding the lock.

IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
justification for how it is ensured that the `is_available` atomic cannot be
altered during the usage of `data`. So, it shouldn't be part of the type
invariants.

For instance, we also have the Revocalbe::access() [1], which is an unsafe
direct accessor for `data`. It has the following safety requirement:

	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
	 and won't be revoked as long as the returned `&T` lives."

Which is equal to the caller must ensure that `is_available` is true, and won't
be altered to false as long as the returned reference lives.

One valid way for the caller would be to wrap it into an RCU read side critical
section and check `is_available`. However, depending on the context, there are
also other justifications, e.g. [2].

[1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/revocable.rs?ref_type=heads#L148
[2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/devres.rs?ref_type=heads#L221

> > +/// - Once is_available is set to false, further access to data is disallowed,
> > +///   and the object is dropped either after an RCU grace period (in [revoke]),
> > +///   or immediately (in [revoke_nosync]).

Same here, RCU isn't a relevant factor for the type invariant IMHO. It's just
how part of the implementation guarantees to up-hold the invariant.

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

[Benno Lossin]

On Sat May 17, 2025 at 11:54 AM CEST, Danilo Krummrich wrote:
> On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
>> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
>> >  /// # Invariants
>> >  ///
>> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
>> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
>> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
>> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
>> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
>> > -///   or immediately (in [`revoke_nosync`]).
>> > +/// - `data` is valid if and only if `is_available` is true.
>> > +/// - Access to `data` requires holding the RCU read-side lock.
>> 
>> I'm not sure what the correct wording here should be. The current
>> wording makes the `revoke_internal` function illegal, as it doesn't hold
>> the read-side lock, but still accesses `data`.
>> 
>> Maybe @Danilo can help here, but as I understand it, the value in `data`
>> is valid for as long as the rcu read-side lock is held *and* if
>> `is_available` was true at some point while holding the lock.
>
> IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
> justification for how it is ensured that the `is_available` atomic cannot be
> altered during the usage of `data`. So, it shouldn't be part of the type
> invariants.

But the `is_available` atomic *can* be altered during the usage of
`data`. And in that case it isn't clear to me how you still allow usage
without relying on rcu.

> For instance, we also have the Revocalbe::access() [1], which is an unsafe
> direct accessor for `data`. It has the following safety requirement:
>
> 	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
> 	 and won't be revoked as long as the returned `&T` lives."
>
> Which is equal to the caller must ensure that `is_available` is true, and won't
> be altered to false as long as the returned reference lives.

Sure. We could add that it remains valid while `is_available` is true,
but when it changes, the data is still valid until the end of the rcu
grace period?

> One valid way for the caller would be to wrap it into an RCU read side critical
> section and check `is_available`. However, depending on the context, there are
> also other justifications, e.g. [2].

For the justification in [2], you need a type invariant.

---
Cheers,
Benno

> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/revocable.rs?ref_type=heads#L148
> [2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/devres.rs?ref_type=heads#L221
>
>> > +/// - Once is_available is set to false, further access to data is disallowed,
>> > +///   and the object is dropped either after an RCU grace period (in [revoke]),
>> > +///   or immediately (in [revoke_nosync]).
>
> Same here, RCU isn't a relevant factor for the type invariant IMHO. It's just
> how part of the implementation guarantees to up-hold the invariant.

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

[Danilo Krummrich]

On Sat, May 17, 2025 at 09:09:02PM +0200, Benno Lossin wrote:
> On Sat May 17, 2025 at 11:54 AM CEST, Danilo Krummrich wrote:
> > On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
> >> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> >> >  /// # Invariants
> >> >  ///
> >> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> >> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> >> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> >> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> >> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> >> > -///   or immediately (in [`revoke_nosync`]).
> >> > +/// - `data` is valid if and only if `is_available` is true.
> >> > +/// - Access to `data` requires holding the RCU read-side lock.
> >> 
> >> I'm not sure what the correct wording here should be. The current
> >> wording makes the `revoke_internal` function illegal, as it doesn't hold
> >> the read-side lock, but still accesses `data`.
> >> 
> >> Maybe @Danilo can help here, but as I understand it, the value in `data`
> >> is valid for as long as the rcu read-side lock is held *and* if
> >> `is_available` was true at some point while holding the lock.
> >
> > IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
> > justification for how it is ensured that the `is_available` atomic cannot be
> > altered during the usage of `data`. So, it shouldn't be part of the type
> > invariants.
> 
> But the `is_available` atomic *can* be altered during the usage of
> `data`. And in that case it isn't clear to me how you still allow usage
> without relying on rcu.

The only real rule is that the data is only valid as long as `is_available` is
true.

Some functions - the safe ones, i.e. try_access(), try_access_with_guard(),
revoke() - ensure this by using RCU.

Some other functions though - the unsafe ones, i.e. access() and revoke_nosync()
- leave it to the caller to ensure this, because sometimes the caller (such as
Devres) can give this gurarantee through other circumstances.

So, the internal lock (RCU or any other kind of lock) isn't really part of the
type invariant, it's just one tool to uphold the type invariant. Another such
tool used by Revocable is `unsafe`, where we just require the caller to uphold
this invariant.

> > For instance, we also have the Revocalbe::access() [1], which is an unsafe
> > direct accessor for `data`. It has the following safety requirement:
> >
> > 	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
> > 	 and won't be revoked as long as the returned `&T` lives."
> >
> > Which is equal to the caller must ensure that `is_available` is true, and won't
> > be altered to false as long as the returned reference lives.
> 
> Sure. We could add that it remains valid while `is_available` is true,
> but when it changes, the data is still valid until the end of the rcu
> grace period?

Well, yes. But that would be true for every other lock as well. Let's say we'd
protect is_available with a mutex, the data would still be guaranteed to be
valid until the mutex is released. But for the reasons given above I don't think
the "protection method" is part of the invariant, so we should just say:
"remains valid at least as long as `is_available` is true".

> > One valid way for the caller would be to wrap it into an RCU read side critical
> > section and check `is_available`. However, depending on the context, there are
> > also other justifications, e.g. [2].
> 
> For the justification in [2], you need a type invariant.

Do you mean that we should document the invariant that Devres does not revoke
things until the device it has been created with is unbound?

If so, I didn't think of documenting that, since it is the whole purpose of
Devres, but I agree, formally it should be documented.

> > [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/revocable.rs?ref_type=heads#L148
> > [2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/devres.rs?ref_type=heads#L221
> >
> >> > +/// - Once is_available is set to false, further access to data is disallowed,
> >> > +///   and the object is dropped either after an RCU grace period (in [revoke]),
> >> > +///   or immediately (in [revoke_nosync]).
> >
> > Same here, RCU isn't a relevant factor for the type invariant IMHO. It's just
> > how part of the implementation guarantees to up-hold the invariant.

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

[Benno Lossin]

On Mon May 19, 2025 at 10:50 AM CEST, Danilo Krummrich wrote:
> On Sat, May 17, 2025 at 09:09:02PM +0200, Benno Lossin wrote:
>> On Sat May 17, 2025 at 11:54 AM CEST, Danilo Krummrich wrote:
>> > On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
>> >> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
>> >> >  /// # Invariants
>> >> >  ///
>> >> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
>> >> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
>> >> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
>> >> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
>> >> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
>> >> > -///   or immediately (in [`revoke_nosync`]).
>> >> > +/// - `data` is valid if and only if `is_available` is true.
>> >> > +/// - Access to `data` requires holding the RCU read-side lock.
>> >> 
>> >> I'm not sure what the correct wording here should be. The current
>> >> wording makes the `revoke_internal` function illegal, as it doesn't hold
>> >> the read-side lock, but still accesses `data`.
>> >> 
>> >> Maybe @Danilo can help here, but as I understand it, the value in `data`
>> >> is valid for as long as the rcu read-side lock is held *and* if
>> >> `is_available` was true at some point while holding the lock.
>> >
>> > IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
>> > justification for how it is ensured that the `is_available` atomic cannot be
>> > altered during the usage of `data`. So, it shouldn't be part of the type
>> > invariants.
>> 
>> But the `is_available` atomic *can* be altered during the usage of
>> `data`. And in that case it isn't clear to me how you still allow usage
>> without relying on rcu.
>
> The only real rule is that the data is only valid as long as `is_available` is
> true.
>
> Some functions - the safe ones, i.e. try_access(), try_access_with_guard(),
> revoke() - ensure this by using RCU.
>
> Some other functions though - the unsafe ones, i.e. access() and revoke_nosync()
> - leave it to the caller to ensure this, because sometimes the caller (such as
> Devres) can give this gurarantee through other circumstances.
>
> So, the internal lock (RCU or any other kind of lock) isn't really part of the
> type invariant, it's just one tool to uphold the type invariant. Another such
> tool used by Revocable is `unsafe`, where we just require the caller to uphold
> this invariant.

I get what you're trying to say, but the statement above about `data`
being valid while `is_available` is `true` is not correct. When using
RCU, you rely on the other functions like `revoke` to wait for the RCU
grace period to end. At the moment nothing prevents them from not doing
so, which means some safety documentation is wrong.

Currently `RevocableGuard` also mentions RCU in its type invariants, is
that correct?

>> > For instance, we also have the Revocalbe::access() [1], which is an unsafe
>> > direct accessor for `data`. It has the following safety requirement:
>> >
>> > 	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
>> > 	 and won't be revoked as long as the returned `&T` lives."
>> >
>> > Which is equal to the caller must ensure that `is_available` is true, and won't
>> > be altered to false as long as the returned reference lives.
>> 
>> Sure. We could add that it remains valid while `is_available` is true,
>> but when it changes, the data is still valid until the end of the rcu
>> grace period?
>
> Well, yes. But that would be true for every other lock as well. Let's say we'd
> protect is_available with a mutex, the data would still be guaranteed to be
> valid until the mutex is released. But for the reasons given above I don't think
> the "protection method" is part of the invariant, so we should just say:
> "remains valid at least as long as `is_available` is true".

No, in the mutex case, the `revoke` function could be implemented like
this:
    
    struct Revocable<T> {
        data: Mutex<Option<T>>,
    }

    fn revoke(&self) {
        self.mutex.lock().take();
    }

Now, anyone accessing first has to check if the value still exists and
while eg `try_access` is holding a `MutexGuard`, the data won't ever be
revoked.

Now this is not a feasible implementation due to its performance &
access pattern, but we could use the same design if we were to use a
standard read-write lock (which would probably still be worse than RCU).

But since `try_access` is using `RCU` under the hood, anyone revoking
**needs** to wait for the grace period to end after setting
`is_available` to false. That's just how life is and so we need to
document that in a type invariant (otherwise how is `try_access` going
to know that what it does is correct?). So if you want `try_access` to
be safe, the functions that don't respect the grace period need to
mention RCU or somehow prevent users from accessing `try_access`.
`revoke_nosync` at the moment does this with the safety requirement
"Callers need to ensure that there are no more concurrent users of the
revocable object." which is pretty vague to me.

I'd probably be able to help more with the safety comments if you can
give me the different use-cases of this function, at the moment nothing
calls that function.

>> > One valid way for the caller would be to wrap it into an RCU read side critical
>> > section and check `is_available`. However, depending on the context, there are
>> > also other justifications, e.g. [2].
>> 
>> For the justification in [2], you need a type invariant.
>
> Do you mean that we should document the invariant that Devres does not revoke
> things until the device it has been created with is unbound?
>
> If so, I didn't think of documenting that, since it is the whole purpose of
> Devres, but I agree, formally it should be documented.

Yes, it should be a type invariant. The more obvious the safety docs are
the better.

---
Cheers,
Benno


>> > [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/revocable.rs?ref_type=heads#L148
>> > [2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/devres.rs?ref_type=heads#L221
>> >
>> >> > +/// - Once is_available is set to false, further access to data is disallowed,
>> >> > +///   and the object is dropped either after an RCU grace period (in [revoke]),
>> >> > +///   or immediately (in [revoke_nosync]).
>> >
>> > Same here, RCU isn't a relevant factor for the type invariant IMHO. It's just
>> > how part of the implementation guarantees to up-hold the invariant.

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

[Danilo Krummrich]

On Mon, May 19, 2025 at 11:18:42AM +0200, Benno Lossin wrote:
> On Mon May 19, 2025 at 10:50 AM CEST, Danilo Krummrich wrote:
> > On Sat, May 17, 2025 at 09:09:02PM +0200, Benno Lossin wrote:
> >> On Sat May 17, 2025 at 11:54 AM CEST, Danilo Krummrich wrote:
> >> > On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
> >> >> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> >> >> >  /// # Invariants
> >> >> >  ///
> >> >> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> >> >> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> >> >> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> >> >> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> >> >> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> >> >> > -///   or immediately (in [`revoke_nosync`]).
> >> >> > +/// - `data` is valid if and only if `is_available` is true.
> >> >> > +/// - Access to `data` requires holding the RCU read-side lock.
> >> >> 
> >> >> I'm not sure what the correct wording here should be. The current
> >> >> wording makes the `revoke_internal` function illegal, as it doesn't hold
> >> >> the read-side lock, but still accesses `data`.
> >> >> 
> >> >> Maybe @Danilo can help here, but as I understand it, the value in `data`
> >> >> is valid for as long as the rcu read-side lock is held *and* if
> >> >> `is_available` was true at some point while holding the lock.
> >> >
> >> > IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
> >> > justification for how it is ensured that the `is_available` atomic cannot be
> >> > altered during the usage of `data`. So, it shouldn't be part of the type
> >> > invariants.
> >> 
> >> But the `is_available` atomic *can* be altered during the usage of
> >> `data`. And in that case it isn't clear to me how you still allow usage
> >> without relying on rcu.
> >
> > The only real rule is that the data is only valid as long as `is_available` is
> > true.
> >
> > Some functions - the safe ones, i.e. try_access(), try_access_with_guard(),
> > revoke() - ensure this by using RCU.
> >
> > Some other functions though - the unsafe ones, i.e. access() and revoke_nosync()
> > - leave it to the caller to ensure this, because sometimes the caller (such as
> > Devres) can give this gurarantee through other circumstances.
> >
> > So, the internal lock (RCU or any other kind of lock) isn't really part of the
> > type invariant, it's just one tool to uphold the type invariant. Another such
> > tool used by Revocable is `unsafe`, where we just require the caller to uphold
> > this invariant.
> 
> I get what you're trying to say, but the statement above about `data`
> being valid while `is_available` is `true` is not correct.

Why not? Please show me a case where `is_available` is false, but I can still
technically access data (without violating a safety requirement).

> When using
> RCU, you rely on the other functions like `revoke` to wait for the RCU
> grace period to end. At the moment nothing prevents them from not doing
> so, which means some safety documentation is wrong.

So, you're saying the problem is that with only the invariant above, revoke()
could be implemented without synchronize_rcu()?

If so, why does not need to be covered by a type invariant? The implementation
uses a lock to uphold its type invariant, if it doesn't it simply is a bug, no?

> Currently `RevocableGuard` also mentions RCU in its type invariants, is
> that correct?

Yes, we either access the data through the Guard (which is an RCU guard, but
could also be any other lock guard) or with unsafe accessors directly.

> >> > For instance, we also have the Revocalbe::access() [1], which is an unsafe
> >> > direct accessor for `data`. It has the following safety requirement:
> >> >
> >> > 	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
> >> > 	 and won't be revoked as long as the returned `&T` lives."
> >> >
> >> > Which is equal to the caller must ensure that `is_available` is true, and won't
> >> > be altered to false as long as the returned reference lives.
> >> 
> >> Sure. We could add that it remains valid while `is_available` is true,
> >> but when it changes, the data is still valid until the end of the rcu
> >> grace period?
> >
> > Well, yes. But that would be true for every other lock as well. Let's say we'd
> > protect is_available with a mutex, the data would still be guaranteed to be
> > valid until the mutex is released. But for the reasons given above I don't think
> > the "protection method" is part of the invariant, so we should just say:
> > "remains valid at least as long as `is_available` is true".
> 
> No, in the mutex case, the `revoke` function could be implemented like
> this:
>     
>     struct Revocable<T> {
>         data: Mutex<Option<T>>,
>     }
> 
>     fn revoke(&self) {
>         self.mutex.lock().take();
>     }
> 
> Now, anyone accessing first has to check if the value still exists and
> while eg `try_access` is holding a `MutexGuard`, the data won't ever be
> revoked.
> 
> Now this is not a feasible implementation due to its performance &
> access pattern, but we could use the same design if we were to use a
> standard read-write lock (which would probably still be worse than RCU).
> 
> But since `try_access` is using `RCU` under the hood, anyone revoking
> **needs** to wait for the grace period to end after setting
> `is_available` to false. That's just how life is and so we need to
> document that in a type invariant (otherwise how is `try_access` going
> to know that what it does is correct?). So if you want `try_access` to
> be safe, the functions that don't respect the grace period need to
> mention RCU or somehow prevent users from accessing `try_access`.
> `revoke_nosync` at the moment does this with the safety requirement
> "Callers need to ensure that there are no more concurrent users of the
> revocable object." which is pretty vague to me.
> 
> I'd probably be able to help more with the safety comments if you can
> give me the different use-cases of this function, at the moment nothing
> calls that function.

While we were talking past each other a bit with this, I guess I get your point
now, i.e. with a Mutex for instance, revoke() couldn't get it wrong, since
there is no way to "bypass" the mutex, but with RCU, there is a away to bypass
the lock by just not calling synchronize_rcu().

So, you want a type invariant that guarantees the call to synchronize_rcu() to
justify the try_access() variants, correct?

However, this invariant does not need to be fulfilled for access() and
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.

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.

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

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

[Benno Lossin]

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:
>> On Mon May 19, 2025 at 10:50 AM CEST, Danilo Krummrich wrote:
>> > On Sat, May 17, 2025 at 09:09:02PM +0200, Benno Lossin wrote:
>> >> On Sat May 17, 2025 at 11:54 AM CEST, Danilo Krummrich wrote:
>> >> > On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
>> >> >> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
>> >> >> >  /// # Invariants
>> >> >> >  ///
>> >> >> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
>> >> >> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
>> >> >> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
>> >> >> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
>> >> >> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
>> >> >> > -///   or immediately (in [`revoke_nosync`]).
>> >> >> > +/// - `data` is valid if and only if `is_available` is true.
>> >> >> > +/// - Access to `data` requires holding the RCU read-side lock.
>> >> >> 
>> >> >> I'm not sure what the correct wording here should be. The current
>> >> >> wording makes the `revoke_internal` function illegal, as it doesn't hold
>> >> >> the read-side lock, but still accesses `data`.
>> >> >> 
>> >> >> Maybe @Danilo can help here, but as I understand it, the value in `data`
>> >> >> is valid for as long as the rcu read-side lock is held *and* if
>> >> >> `is_available` was true at some point while holding the lock.
>> >> >
>> >> > IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
>> >> > justification for how it is ensured that the `is_available` atomic cannot be
>> >> > altered during the usage of `data`. So, it shouldn't be part of the type
>> >> > invariants.
>> >> 
>> >> But the `is_available` atomic *can* be altered during the usage of
>> >> `data`. And in that case it isn't clear to me how you still allow usage
>> >> without relying on rcu.
>> >
>> > The only real rule is that the data is only valid as long as `is_available` is
>> > true.
>> >
>> > Some functions - the safe ones, i.e. try_access(), try_access_with_guard(),
>> > revoke() - ensure this by using RCU.
>> >
>> > Some other functions though - the unsafe ones, i.e. access() and revoke_nosync()
>> > - leave it to the caller to ensure this, because sometimes the caller (such as
>> > Devres) can give this gurarantee through other circumstances.
>> >
>> > So, the internal lock (RCU or any other kind of lock) isn't really part of the
>> > type invariant, it's just one tool to uphold the type invariant. Another such
>> > tool used by Revocable is `unsafe`, where we just require the caller to uphold
>> > this invariant.
>> 
>> I get what you're trying to say, but the statement above about `data`
>> being valid while `is_available` is `true` is not correct.
>
> 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);

>> When using
>> RCU, you rely on the other functions like `revoke` to wait for the RCU
>> grace period to end. At the moment nothing prevents them from not doing
>> so, which means some safety documentation is wrong.
>
> So, you're saying the problem is that with only the invariant above, revoke()
> could be implemented without synchronize_rcu()?

Indeed.

> If so, why does not need to be covered by a type invariant? The implementation
> uses a lock to uphold its type invariant, if it doesn't it simply is a bug, no?

I don't follow this sentence. `try_access` is relying on RCU and thus
anyone else accessing the value concurrently needs to respect that.

>> Currently `RevocableGuard` also mentions RCU in its type invariants, is
>> that correct?
>
> Yes, we either access the data through the Guard (which is an RCU guard, but
> could also be any other lock guard) or with unsafe accessors directly.
>
>> >> > For instance, we also have the Revocalbe::access() [1], which is an unsafe
>> >> > direct accessor for `data`. It has the following safety requirement:
>> >> >
>> >> > 	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
>> >> > 	 and won't be revoked as long as the returned `&T` lives."
>> >> >
>> >> > Which is equal to the caller must ensure that `is_available` is true, and won't
>> >> > be altered to false as long as the returned reference lives.
>> >> 
>> >> Sure. We could add that it remains valid while `is_available` is true,
>> >> but when it changes, the data is still valid until the end of the rcu
>> >> grace period?
>> >
>> > Well, yes. But that would be true for every other lock as well. Let's say we'd
>> > protect is_available with a mutex, the data would still be guaranteed to be
>> > valid until the mutex is released. But for the reasons given above I don't think
>> > the "protection method" is part of the invariant, so we should just say:
>> > "remains valid at least as long as `is_available` is true".
>> 
>> No, in the mutex case, the `revoke` function could be implemented like
>> this:
>>     
>>     struct Revocable<T> {
>>         data: Mutex<Option<T>>,
>>     }
>> 
>>     fn revoke(&self) {
>>         self.mutex.lock().take();
>>     }
>> 
>> Now, anyone accessing first has to check if the value still exists and
>> while eg `try_access` is holding a `MutexGuard`, the data won't ever be
>> revoked.
>> 
>> Now this is not a feasible implementation due to its performance &
>> access pattern, but we could use the same design if we were to use a
>> standard read-write lock (which would probably still be worse than RCU).
>> 
>> But since `try_access` is using `RCU` under the hood, anyone revoking
>> **needs** to wait for the grace period to end after setting
>> `is_available` to false. That's just how life is and so we need to
>> document that in a type invariant (otherwise how is `try_access` going
>> to know that what it does is correct?). So if you want `try_access` to
>> be safe, the functions that don't respect the grace period need to
>> mention RCU or somehow prevent users from accessing `try_access`.
>> `revoke_nosync` at the moment does this with the safety requirement
>> "Callers need to ensure that there are no more concurrent users of the
>> revocable object." which is pretty vague to me.
>> 
>> I'd probably be able to help more with the safety comments if you can
>> give me the different use-cases of this function, at the moment nothing
>> calls that function.
>
> While we were talking past each other a bit with this,

Yeah I thought that as well. But I feel like we're getting closer to
mutual understanding :)

> I guess I get your point
> now, i.e. with a Mutex for instance, revoke() couldn't get it wrong, since
> there is no way to "bypass" the mutex, but with RCU, there is a away to bypass
> the lock by just not calling synchronize_rcu().

Exactly.

This difficulty makes a lot of sense, since we don't have a proper RCU
primitive yet (which has several reasons, one of them being that RCU
just has so many different applications). I guess `Revocable` is one
such use-case for RCU (but also should support circumventing it when
otherwise deemed correct).

> So, you want a type invariant that guarantees the call to synchronize_rcu() to
> justify the try_access() variants, correct?

I want a type invariant that allows `try_access` to give a valid pointer
to a valid `T` to the guard. (By the way, why doesn't the guard store a
`&T` instead of a raw pointer?)

> However, this invariant does not need to be fulfilled for access() and

Where is `access()` defined?

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

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

---
Cheers,
Benno

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

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

[Danilo Krummrich]

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`")

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

[Benno Lossin]

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

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

[Marcelo Moreira]

Thanks a lot for the discussions. I was reading them one by one and
trying to consolidate the ideas.
I am preparing a new patch version focused on improving everything
that was discussed.

Before submitting the patch, I would like to understand if things are
clear to me. Here is what I am preparing:

1. Refined `Revocable<T>` Invariants, suggested by Benno:
Proposed Documentation:
/// # Invariants
///
/// - `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`.


 2. Updated try_access and try_access_with_guard Safety Comments:
Proposed Documentation:
// INVARIANT: `self.data` is valid for reads because
`self.is_available` is true,
// and the RCU read-side lock ensures this condition is maintained
during access.

These comments now directly reference the new Revocable<T> read invariants.
This addresses the  point about justifying the access to data via the
invariants,
especially considering the RCU read-side lock.


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() };
 }  else {
       // This branch for `revoke_nosync` requires the caller to prove
that `data`
       // can be dropped immediately without waiting for any RCU grace period.
}
// SAFETY: We know `self.data` is valid because only one CPU can succeed the
// `swap` above that takes `is_available` from `true` to `false`.
 unsafe { drop_in_place(self.data.get()) };
   }
}

I've added a  comment for the SYNC = false branch (used by revoke_nosync).
This explicit else block, even if semantically empty of executable code, is for
documenting the unsafe contract for revoke_nosync(). It  states that the caller
must prove data safety since no RCU grace period is awaited, aligning with
Danilo's explanation of revoke_nosync's purpose. Additionally, I've corrected
the comment from compare_exchange to swap to accurately reflect the
atomic operation used.


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>,
}

This adds the # Invariants section for RevocableGuard, as we agreed it should
state its guarantees, including the RCU read-side lock. The adjustment of
_p: PhantomData<&'a ()> to _p: PhantomData<&'a T> is a minor improvement in
type-level documentation, making the lifetime dependency more explicit to the
Rust compiler and future readers.


5. Revised RevocableGuard Deref Safety Comment
Proposed Documentation:
fn deref(&self) -> &Self::Target {
        // SAFETY: By the invariants of `RevocableGuard`, the RCU
read-side lock is held,
        // and `data_ref` points to valid data for the lifetime of this guard.
        unsafe { &*self.data_ref }
    }

The // SAFETY: comment for Deref is updated to refer to the RevocableGuard's new
invariants, providing a justification for the unsafe dereference.


6. PinnedDrop for Revocable<T> Safety Documentation & Readability
Proposed Documentation:
let p = unsafe { self.get_unchecked_mut() };
        if *p.is_available.get_mut() {
            // INVARIANT: `is_available` is true, so `data` is valid for reads.
            // SAFETY: `self.data` is valid for writes because
`is_available` was atomically changed from true to false
            // in `PinnedDrop::drop`, and only one thread can do it
now as we have `&mut self`.
            // This ensures `data` is valid for writes in this
context, allowing `drop_in_place`.
            unsafe { drop_in_place(p.data.get()) };
        }

The // INVARIANT: and // SAFETY: comments within the PinnedDrop
implementation are
updated to reflect the new write invariant of Revocable<T>, justifying
the drop_in_place call.


Please let me know if something is wrong or unnecessary... I'll try to
rephrase if appropriate. Tks :D

---
Cheers,
Marcelo Moreira :-)


Em seg., 19 de mai. de 2025 às 09:26, Benno Lossin <lossin@kernel.org> escreveu:
>
> 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

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

[Danilo Krummrich]

On 5/19/25 2:26 PM, Benno Lossin wrote:
> 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`.

That sounds good!

> * `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`.

If think for this you have the same conditional, in the RCU case you can't call
drop_in_place() immediately after is_available was altered, but have to wait for
synchronize_rcu() to return.

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

[Benno Lossin]

On Fri May 23, 2025 at 9:19 AM CEST, Danilo Krummrich wrote:
> On 5/19/25 2:26 PM, Benno Lossin wrote:
>> 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`.
>
> That sounds good!
>
>> * `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`.
>
> If think for this you have the same conditional, in the RCU case you can't call
> drop_in_place() immediately after is_available was altered, but have to wait for
> synchronize_rcu() to return.

Oh yeah, how about:

* `data` is valid for writes when `is_available` was atomically changed from `true to `false`
  and no thread is holding an RCU read-side lock that was acquired prior to the change in
  `is_available`.

---
Cheers,
Benno

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

[Benno Lossin]

On Fri May 23, 2025 at 2:13 AM CEST, Marcelo Moreira wrote:
> Thanks a lot for the discussions. I was reading them one by one and
> trying to consolidate the ideas.
> I am preparing a new patch version focused on improving everything
> that was discussed.
>
> Before submitting the patch, I would like to understand if things are
> clear to me. Here is what I am preparing:
>
> 1. Refined `Revocable<T>` Invariants, suggested by Benno:
> Proposed Documentation:
> /// # Invariants
> ///
> /// - `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`.

A few comments:

* your email client seems to wrap this line :(
* the "two cases" should be indented, as they logically belong to the
  bullet point above.
* the information for writing `data` needs to be changed according to
  what I suggested in reply to Danilo

>  2. Updated try_access and try_access_with_guard Safety Comments:
> Proposed Documentation:
> // INVARIANT: `self.data` is valid for reads because
> `self.is_available` is true,
> // and the RCU read-side lock ensures this condition is maintained
> during access.

I'm missing the context *where* this invariant comment is placed. Maybe
send the patch with the modification or send a link to a branch where
you have the commit.

> These comments now directly reference the new Revocable<T> read invariants.
> This addresses the  point about justifying the access to data via the
> invariants,
> especially considering the RCU read-side lock.
>
>
> 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.

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

> }
> // SAFETY: We know `self.data` is valid because only one CPU can succeed the
> // `swap` above that takes `is_available` from `true` to `false`.
>  unsafe { drop_in_place(self.data.get()) };
>    }
> }
>
> I've added a  comment for the SYNC = false branch (used by revoke_nosync).
> This explicit else block, even if semantically empty of executable code, is for
> documenting the unsafe contract for revoke_nosync(). It  states that the caller
> must prove data safety since no RCU grace period is awaited, aligning with
> Danilo's explanation of revoke_nosync's purpose. Additionally, I've corrected
> the comment from compare_exchange to swap to accurately reflect the
> atomic operation used.
>
>
> 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 :)

> This adds the # Invariants section for RevocableGuard, as we agreed it should
> state its guarantees, including the RCU read-side lock. The adjustment of
> _p: PhantomData<&'a ()> to _p: PhantomData<&'a T> is a minor improvement in
> type-level documentation, making the lifetime dependency more explicit to the
> Rust compiler and future readers.
>
>
> 5. Revised RevocableGuard Deref Safety Comment
> Proposed Documentation:
> fn deref(&self) -> &Self::Target {
>         // SAFETY: By the invariants of `RevocableGuard`, the RCU
> read-side lock is held,
>         // and `data_ref` points to valid data for the lifetime of this guard.
>         unsafe { &*self.data_ref }

With the above change, this body becomes `self.data` :)

>     }
>
> The // SAFETY: comment for Deref is updated to refer to the RevocableGuard's new
> invariants, providing a justification for the unsafe dereference.
>
>
> 6. PinnedDrop for Revocable<T> Safety Documentation & Readability
> Proposed Documentation:
> let p = unsafe { self.get_unchecked_mut() };
>         if *p.is_available.get_mut() {
>             // INVARIANT: `is_available` is true, so `data` is valid for reads.
>             // SAFETY: `self.data` is valid for writes because
> `is_available` was atomically changed from true to false

This is wrong? It still is `true`?

>             // in `PinnedDrop::drop`, and only one thread can do it
> now as we have `&mut self`.
>             // This ensures `data` is valid for writes in this
> context, allowing `drop_in_place`.
>             unsafe { drop_in_place(p.data.get()) };
>         }
>
> The // INVARIANT: and // SAFETY: comments within the PinnedDrop
> implementation are
> updated to reflect the new write invariant of Revocable<T>, justifying
> the drop_in_place call.
>
>
> Please let me know if something is wrong or unnecessary... I'll try to
> rephrase if appropriate. Tks :D

It already looks much better than what we have currently. It's fine to
send a patch that's still needing review (and it makes it easier to
review when I have the full context). So I'd just do that and we go from
there. We're only on v2 on this one and I have seen patches go into the
double digits, so don't worry about that.

---
Cheers,
Benno

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

[Danilo Krummrich]

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.

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

[Benno Lossin]

On Fri May 23, 2025 at 10:55 AM CEST, Danilo Krummrich wrote:
> 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.

Makes sense, but then we should add this as a safe function (in a
separate patch). After all we already have other safe functions that
should be unsafe were it not for klint.

>> >  }  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.

Yeah then let's do it (also separate patch).

---
Cheers,
Benno

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

[Marcelo Moreira]

Guys, thanks for your patience and kindness =D

I'm sending v3 right now!

In Brazil we say "Tamo Junto!" - We're in this together!
Marcelo Moreira


Em sex., 23 de mai. de 2025 às 08:53, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Fri May 23, 2025 at 10:55 AM CEST, Danilo Krummrich wrote:
> > 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.
>
> Makes sense, but then we should add this as a safe function (in a
> separate patch). After all we already have other safe functions that
> should be unsafe were it not for klint.
>
> >> >  }  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.
>
> Yeah then let's do it (also separate patch).
>
> ---
> Cheers,
> Benno