Skip to contentSkip to conversation

rust: atomic: add `fetch_sub` and update docs

PATCH In review Andreas Hindborg 4 participants 10 messages Feb 20 – Feb 24

Subsystems (3)
Rust Atomic infrastructure Rust sync
lore
Share & tools
lore

Andreas Hindborg

author reviewer
#0 lore raw

v3 cover rust: atomic: add `fetch_sub` and update docs

Add fetch_sub and improve documentation and safety comments of of fetch_add.

Signed-off-by Andreas Hindborg <[email protected]>

---
Changes in v3:
- Remove a spurious line feed.
- Add a patch to update a safety comment for fetch_add.
- Update safety comment in fetch_sub implementation.
- Link to v2: https://msgid.link/[email protected]

Changes in v2:
- Update example to be more clear.
- Add a patch that updates documentation for fetch_add.
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Andreas Hindborg (3):
rust: atomic: add fetch_sub
rust: atomic: update documentation for fetch_add
rust: atomic: update a safety comment in impl of fetch_add

 rust/kernel/sync/atomic.rs          | 53 ++++++++++++++++++++++++++++++++-----
 rust/kernel/sync/atomic/internal.rs |  7 ++++-
 2 files changed, 53 insertions(+), 7 deletions(-)

---
base-commit: 63804fed149a6750ffd28610c5c1c98cce6bd377
change-id: 20260128-atomic-sub-2ff15962df12

Best regards,

Signature · 1 line
Andreas Hindborg <[email protected]>
author reviewer
#1 lore raw

PATCH v3 1/3 rust: atomic: add fetch_sub

Add Atomic::fetch_sub with implementation and documentation in line with existing Atomic::fetch_add implementation.

Reviewed-by Alice Ryhl <[email protected]>
Signed-off-by Andreas Hindborg <[email protected]>
---
 rust/kernel/sync/atomic.rs          | 43 +++++++++++++++++++++++++++++++++++++
 rust/kernel/sync/atomic/internal.rs |  5 +++++
 2 files changed, 48 insertions(+)
rust/kernel/sync/atomic.rs +43 -0
index 4aebeacb961a2..83e03bb7ecd24 100644
@@ -559,4 +559,47 @@ pub fn fetch_add<Rhs, Ordering: ordering::Ordering>(&self, v: Rhs, _: Ordering)
         // SAFETY: `ret` comes from reading `self.0`, which is a valid `T` per type invariants.
         unsafe { from_repr(ret) }
     }
+
+    /// Atomic fetch and subtract.
+    ///
+    /// Atomically updates `*self` to `(*self).wrapping_sub(v)`, and returns the value of `*self`
+    /// before the update.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::sync::atomic::{Atomic, Acquire, Full, Relaxed};
+    ///
+    /// let x = Atomic::new(42);
+    /// assert_eq!(42, x.load(Relaxed));
+    /// assert_eq!(42, x.fetch_sub(12, Acquire));
+    /// assert_eq!(30, x.load(Relaxed));
+    ///
+    /// let x = Atomic::new(42);
+    /// assert_eq!(42, x.load(Relaxed));
+    /// assert_eq!(42, x.fetch_sub(12, Full));
+    /// assert_eq!(30, x.load(Relaxed));
+    /// ```
+    #[inline(always)]
+    pub fn fetch_sub<Rhs, Ordering: ordering::Ordering>(&self, v: Rhs, _: Ordering) -> T
+    where
+        // Types that support addition also support subtraction.
+        T: AtomicAdd<Rhs>,
+    {
+        let v = T::rhs_into_delta(v);
+
+        // INVARIANT: `self.0` is a valid `T` after `atomic_fetch_sub*()` due to safety requirement
+        // of `AtomicAdd`.
+        let ret = {
+            match Ordering::TYPE {
+                OrderingType::Full => T::Repr::atomic_fetch_sub(&self.0, v),
+                OrderingType::Acquire => T::Repr::atomic_fetch_sub_acquire(&self.0, v),
+                OrderingType::Release => T::Repr::atomic_fetch_sub_release(&self.0, v),
+                OrderingType::Relaxed => T::Repr::atomic_fetch_sub_relaxed(&self.0, v),
+            }
+        };
+
+        // SAFETY: `ret` comes from reading `self.0`, which is a valid `T` per type invariants.
+        unsafe { from_repr(ret) }
+    }
 }
rust/kernel/sync/atomic/internal.rs +5 -0
index 6fdd8e59f45be..a094f704be0ce 100644
@@ -261,5 +261,10 @@ fn fetch_add[acquire, release, relaxed](a: &AtomicRepr<Self>, v: Self::Delta) ->
             // SAFETY: `a.as_ptr()` is valid and properly aligned.
             unsafe { bindings::#call(v, a.as_ptr().cast()) }
         }
+
+        fn fetch_sub[acquire, release, relaxed](a: &AtomicRepr<Self>, v: Self::Delta) -> Self {
+            // SAFETY: `a.as_ptr()` guarantees the returned pointer is valid and properly aligned.
+            unsafe { bindings::#call(v, a.as_ptr().cast()) }
+        }
     }
 );
Signature · 1 line
2.51.2
author reviewer
#2 lore raw

PATCH v3 2/3 rust: atomic: update documentation for `fetch_add`

In reply to Andreas Hindborg

The documentation for fetch_add does not indicate that the original value is returned by fetch_add. Update the documentation so this is clear.

Reviewed-by Alice Ryhl <[email protected]>
Signed-off-by Andreas Hindborg <[email protected]>
---
 rust/kernel/sync/atomic.rs | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
rust/kernel/sync/atomic.rs +4 -6
index 83e03bb7ecd24..774c0c9f1c9c2 100644
@@ -527,16 +527,14 @@ pub fn add<Rhs>(&self, v: Rhs, _: ordering::Relaxed)
     /// use kernel::sync::atomic::{Atomic, Acquire, Full, Relaxed};
     ///
     /// let x = Atomic::new(42);
-    ///
     /// assert_eq!(42, x.load(Relaxed));
-    ///
-    /// assert_eq!(54, { x.fetch_add(12, Acquire); x.load(Relaxed) });
+    /// assert_eq!(42, x.fetch_add(12, Acquire));
+    /// assert_eq!(54, x.load(Relaxed));
     ///
     /// let x = Atomic::new(42);
-    ///
     /// assert_eq!(42, x.load(Relaxed));
-    ///
-    /// assert_eq!(54, { x.fetch_add(12, Full); x.load(Relaxed) } );
+    /// assert_eq!(42, x.fetch_add(12, Full));
+    /// assert_eq!(54, x.load(Relaxed));
     /// ```
     #[inline(always)]
     pub fn fetch_add<Rhs, Ordering: ordering::Ordering>(&self, v: Rhs, _: Ordering) -> T
Signature · 1 line
2.51.2
author reviewer
#3 lore raw

PATCH v3 3/3 rust: atomic: update a safety comment in impl of `fetch_add`

In reply to Andreas Hindborg

The safety comment used in the implementation of fetch_add could be read as just saying something it is true without justifying it. Update the safety comment to include justification.

Signed-off-by Andreas Hindborg <[email protected]>
---
 rust/kernel/sync/atomic/internal.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
rust/kernel/sync/atomic/internal.rs +1 -1
index a094f704be0ce..67745c44e710b 100644
@@ -258,7 +258,7 @@ fn add[](a: &AtomicRepr<Self>, v: Self::Delta) {
         /// Atomically updates `*a` to `(*a).wrapping_add(v)`, and returns the value of `*a`
         /// before the update.
         fn fetch_add[acquire, release, relaxed](a: &AtomicRepr<Self>, v: Self::Delta) -> Self {
-            // SAFETY: `a.as_ptr()` is valid and properly aligned.
+            // SAFETY: `a.as_ptr()` guarantees the returned pointer is valid and properly aligned.
             unsafe { bindings::#call(v, a.as_ptr().cast()) }
         }
Signature · 1 line
2.51.2
maintainer Reviewed-by on v3 3/3
#4 lore raw

Re: [PATCH v3 3/3] rust: atomic: update a safety comment in impl of `fetch_add`

Quoted 5 lines · Andreas Hindborg

The safety comment used in the implementation of fetch_add could be read as just saying something it is true without justifying it. Update the safety comment to include justification.

Signed-off-by Andreas Hindborg <[email protected]>
Reviewed-by Alice Ryhl <[email protected]>

rust/kernel/sync/atomic/internal.rs fn add[](a: &AtomicRepr<Self>, v: Self::Delta) { · line 261

Quoted 13 lines
 rust/kernel/sync/atomic/internal.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
rust/kernel/sync/atomic/internal.rs +1 -1
index a094f704be0ce..67745c44e710b 100644
@@ -258,7 +258,7 @@ fn add[](a: &AtomicRepr<Self>, v: Self::Delta) {
         /// Atomically updates `*a` to `(*a).wrapping_add(v)`, and returns the value of `*a`
         /// before the update.
         fn fetch_add[acquire, release, relaxed](a: &AtomicRepr<Self>, v: Self::Delta) -> Self {
-            // SAFETY: `a.as_ptr()` is valid and properly aligned.
+            // SAFETY: `a.as_ptr()` guarantees the returned pointer is valid and properly aligned.

Perhaps more natural wording is:

SAFETY: a.as_ptr() always returns a pointer that is valid and properly aligned.

Because I doubt as_ptr() documents these things explicitly. (To be clear, I don't think as_ptr() needs to do so - that'd be really verbose.)

Anyway, it's fine like this too.

Alice

#5 lore raw

Re: [PATCH v3 3/3] rust: atomic: update a safety comment in impl of `fetch_add`

Quoted 4 lines · Alice Ryhl

Because I doubt as_ptr() documents these things explicitly. (To be clear, I don't think as_ptr() needs to do so - that'd be really verbose.)

It documents them, which is why I suggested it.

Cheers,
Miguel

maintainer
#6 lore raw

Re: [PATCH v3 3/3] rust: atomic: update a safety comment in impl of `fetch_add`

Quoted 7 lines · Miguel Ojeda
Quoted 4 lines · Alice Ryhl

Because I doubt as_ptr() documents these things explicitly. (To be clear, I don't think as_ptr() needs to do so - that'd be really verbose.)

It documents them, which is why I suggested it.

I guess I should have double checked. In that case the wording is great.

Alice

maintainer
#7 lore raw
In reply to Andreas Hindborg
Quoted 5 lines · Andreas Hindborg

Add fetch_sub and improve documentation and safety comments of of fetch_add.

Signed-off-by Andreas Hindborg <[email protected]>

---

I queued the whole series (with some format improvement per tip:
Documentation/process/maintainer-tip.rst). I also added a Suggested-by from Miguel on patch #3 (let me know if you or Miguel disagree with that) and appreciate Miguel's suggestion and your effort to apply it.

Thank you all!

Regards,
Boqun

Quoted 29 lines

Changes in v3:
- Remove a spurious line feed.
- Add a patch to update a safety comment for fetch_add.
- Update safety comment in fetch_sub implementation.
- Link to v2: https://msgid.link/[email protected]

Changes in v2:
- Update example to be more clear.
- Add a patch that updates documentation for fetch_add.
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Andreas Hindborg (3):
rust: atomic: add fetch_sub
rust: atomic: update documentation for fetch_add
rust: atomic: update a safety comment in impl of fetch_add

 rust/kernel/sync/atomic.rs          | 53 ++++++++++++++++++++++++++++++++-----
 rust/kernel/sync/atomic/internal.rs |  7 ++++-
 2 files changed, 53 insertions(+), 7 deletions(-)

---
base-commit: 63804fed149a6750ffd28610c5c1c98cce6bd377
change-id: 20260128-atomic-sub-2ff15962df12

Best regards,

Signature · 1 line
Andreas Hindborg <[email protected]>
author reviewer
#8 lore raw
Quoted 11 lines · Boqun Feng
Quoted 5 lines · Andreas Hindborg

Add fetch_sub and improve documentation and safety comments of of fetch_add.

Signed-off-by Andreas Hindborg <[email protected]>

---

I queued the whole series (with some format improvement per tip:
Documentation/process/maintainer-tip.rst). I also added a Suggested-by from Miguel on patch #3 (let me know if you or Miguel disagree with that) and appreciate Miguel's suggestion and your effort to apply it.

Thanks for adding the proper tags!

Best regards,
Andreas Hindborg

#9 lore raw
In reply to Boqun Feng
Quoted 5 lines · Boqun Feng

I queued the whole series (with some format improvement per tip:
Documentation/process/maintainer-tip.rst). I also added a Suggested-by from Miguel on patch #3 (let me know if you or Miguel disagree with that) and appreciate Miguel's suggestion and your effort to apply it.

Thanks Boqun!

Cheers,
Miguel