[PATCH 0/3] implement `kernel::sync::Refcount` and convert users

[PATCH 0/3] implement `kernel::sync::Refcount` and convert users

[Gary Guo]

Currently there're two refcount usage in rust/, `Arc` and `block::mq`.
`Arc` uses `refcount_t` with FFI calls directly, and `block::mq` use
Rust atomics and custom refcounting.

This series consolidate them to have a single `Refcount` which wraps
`refcount_t` and have it used by both.

With the removal of Rust `AtomicU64` this would also make the code work
for 32-bit systems. See [1] as a previous attempt to fix this issue.

Cc: Will Deacon <will@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Link: https://lore.kernel.org/rust-for-linux/20240905061214.3954271-1-davidgow@google.com/ [1]

Gary Guo (3):
  rust: implement `kernel::sync::Refcount`
  rust: convert `Arc` to use `Refcount`
  rust: block: convert `block::mq` to use `Refcount`

 rust/helpers/refcount.c            |  15 ++++
 rust/kernel/block/mq/operations.rs |   7 +-
 rust/kernel/block/mq/request.rs    |  70 ++++++-------------
 rust/kernel/sync.rs                |   2 +
 rust/kernel/sync/arc.rs            |  45 ++++++------
 rust/kernel/sync/refcount.rs       | 106 +++++++++++++++++++++++++++++
 6 files changed, 168 insertions(+), 77 deletions(-)
 create mode 100644 rust/kernel/sync/refcount.rs


base-commit: c94d8cdb9ade6357824d6c2338e03d1cc726d43a
-- 
2.44.1

[PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Gary Guo]

This is a wrapping layer of `include/linux/refcount.h`. Currently only
the most basic operations (read/set/inc/dec/dec_and_test) are implemented,
additional methods can be implemented when they are needed.

Currently the kernel refcount has already been used in `Arc`, however it
calls into FFI directly.

Cc: Will Deacon <will@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 rust/helpers/refcount.c      | 15 ++++++
 rust/kernel/sync.rs          |  2 +
 rust/kernel/sync/refcount.rs | 94 ++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 rust/kernel/sync/refcount.rs

diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
index f47afc148ec3..39649443426b 100644
--- a/rust/helpers/refcount.c
+++ b/rust/helpers/refcount.c
@@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
 	return (refcount_t)REFCOUNT_INIT(n);
 }
 
+unsigned int rust_helper_refcount_read(refcount_t *r)
+{
+	return refcount_read(r);
+}
+
+void rust_helper_refcount_set(refcount_t *r, int n)
+{
+	refcount_set(r, n);
+}
+
 void rust_helper_refcount_inc(refcount_t *r)
 {
 	refcount_inc(r);
 }
 
+void rust_helper_refcount_dec(refcount_t *r)
+{
+	refcount_dec(r);
+}
+
 bool rust_helper_refcount_dec_and_test(refcount_t *r)
 {
 	return refcount_dec_and_test(r);
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 0ab20975a3b5..534f098a44eb 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -11,12 +11,14 @@
 mod condvar;
 pub mod lock;
 mod locked_by;
+mod refcount;
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
 pub use lock::mutex::{new_mutex, Mutex};
 pub use lock::spinlock::{new_spinlock, SpinLock};
 pub use locked_by::LockedBy;
+pub use refcount::Refcount;
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
diff --git a/rust/kernel/sync/refcount.rs b/rust/kernel/sync/refcount.rs
new file mode 100644
index 000000000000..4a5b815adc05
--- /dev/null
+++ b/rust/kernel/sync/refcount.rs
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Atomic reference counting.
+//!
+//! C header: [`include/linux/refcount.h`](srctree/include/linux/refcount.h)
+
+use crate::types::Opaque;
+use core::sync::atomic::AtomicI32;
+
+/// Atomic reference counter.
+///
+/// This type is conceptually an atomic integer, but provides a saturation semantics compared to
+/// normal atomic integers. Values in the negative range when viewed as a signed integer are
+/// saturation (bad) values. For details about the saturation semantics, please refer to top of
+/// [`include/linux/refcount.h`](srctree/include/refcount.h).
+///
+/// Wraps the kernel's C `refcount_t`.
+#[repr(transparent)]
+pub struct Refcount(Opaque<bindings::refcount_t>);
+
+impl Refcount {
+    /// Construct a new [`Refcount`] from an initial value.
+    #[inline]
+    pub fn new(value: i32) -> Self {
+        // SAFETY: There are no safety requirements for this FFI call.
+        Self(Opaque::new(unsafe { bindings::REFCOUNT_INIT(value) }))
+    }
+
+    #[inline]
+    fn as_ptr(&self) -> *mut bindings::refcount_t {
+        self.0.get()
+    }
+
+    /// Get a refcount's value.
+    #[inline]
+    pub fn read(&self) -> i32 {
+        // SAFETY: `self.as_ptr()` is valid.
+        unsafe { bindings::refcount_read(self.as_ptr()) as _ }
+    }
+
+    /// Set a refcount's value.
+    #[inline]
+    pub fn set(&self, value: i32) {
+        // SAFETY: `self.as_ptr()` is valid.
+        unsafe { bindings::refcount_set(self.as_ptr(), value) }
+    }
+
+    /// Increment a refcount.
+    ///
+    /// It will saturate if overflows and `WARN`. It will also `WARN` is the refcount is 0, as this
+    /// represents a possible use-after-free condition.
+    ///
+    /// Provides no memory ordering, it is assumed that caller already has a reference on the
+    /// object.
+    #[inline]
+    pub fn inc(&self) {
+        // SAFETY: self is valid.
+        unsafe { bindings::refcount_inc(self.as_ptr()) }
+    }
+
+    /// Decrement a refcount.
+    ///
+    /// It will `WARN` on underflow and fail to decrement when saturated.
+    ///
+    /// Provides release memory ordering, such that prior loads and stores are done
+    /// before.
+    #[inline]
+    pub fn dec(&self) {
+        // SAFETY: `self.as_ptr()` is valid.
+        unsafe { bindings::refcount_dec(self.as_ptr()) }
+    }
+
+    /// Decrement a refcount and test if it is 0.
+    ///
+    /// It will `WARN` on underflow and fail to decrement when saturated.
+    ///
+    /// Provides release memory ordering, such that prior loads and stores are done
+    /// before, and provides an acquire ordering on success such that memory deallocation
+    /// must come after.
+    ///
+    /// Returns true if the resulting refcount is 0, false otherwise.
+    #[inline]
+    #[must_use = "use `dec` instead you do not need to test if it is 0"]
+    pub fn dec_and_test(&self) -> bool {
+        // SAFETY: `self.as_ptr()` is valid.
+        unsafe { bindings::refcount_dec_and_test(self.as_ptr()) }
+    }
+}
+
+// SAFETY: `refcount_t` is thread-safe.
+unsafe impl Send for Refcount {}
+
+// SAFETY: `refcount_t` is thread-safe.
+unsafe impl Sync for Refcount {}
-- 
2.44.1

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Dirk Behme]

Am 04.10.24 um 17:52 schrieb Gary Guo:
> This is a wrapping layer of `include/linux/refcount.h`. Currently only
> the most basic operations (read/set/inc/dec/dec_and_test) are implemented,
> additional methods can be implemented when they are needed.
> 
> Currently the kernel refcount has already been used in `Arc`, however it
> calls into FFI directly.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>   rust/helpers/refcount.c      | 15 ++++++
...
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 0ab20975a3b5..534f098a44eb 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -11,12 +11,14 @@
>   mod condvar;
>   pub mod lock;
>   mod locked_by;
> +mod refcount;
>   
>   pub use arc::{Arc, ArcBorrow, UniqueArc};
>   pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
>   pub use lock::mutex::{new_mutex, Mutex};
>   pub use lock::spinlock::{new_spinlock, SpinLock};
>   pub use locked_by::LockedBy;
> +pub use refcount::Refcount;
>   
>   /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>   #[repr(transparent)]
> diff --git a/rust/kernel/sync/refcount.rs b/rust/kernel/sync/refcount.rs
> new file mode 100644
> index 000000000000..4a5b815adc05
> --- /dev/null
> +++ b/rust/kernel/sync/refcount.rs
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Atomic reference counting.
> +//!
> +//! C header: [`include/linux/refcount.h`](srctree/include/linux/refcount.h)
> +
> +use crate::types::Opaque;
> +use core::sync::atomic::AtomicI32;
> +
> +/// Atomic reference counter.
> +///
> +/// This type is conceptually an atomic integer, but provides a saturation semantics compared to

"a ... samantics": Do you like to check that? Either "a ... semantic" 
(without 's')? Or if it shall be plural drop the 'a'?

...
> +    /// Increment a refcount.
> +    ///
> +    /// It will saturate if overflows and `WARN`. It will also `WARN` is the refcount is 0, as this

WARN if ? Not "is" ?

Best regards

Dirk

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Andreas Hindborg]

Hi Gary,

"Gary Guo" <gary@garyguo.net> writes:

[...]

> diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> index f47afc148ec3..39649443426b 100644
> --- a/rust/helpers/refcount.c
> +++ b/rust/helpers/refcount.c
> @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
>  	return (refcount_t)REFCOUNT_INIT(n);
>  }
>
> +unsigned int rust_helper_refcount_read(refcount_t *r)
> +{
> +	return refcount_read(r);
> +}

+EXPORT_SYMBOL_GPL(rust_helper_refcount_read);

> +
> +void rust_helper_refcount_set(refcount_t *r, int n)
> +{
> +	refcount_set(r, n);
> +}

+EXPORT_SYMBOL_GPL(rust_helper_refcount_set);

BR Andreas

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Gary Guo]

On Fri, 04 Oct 2024 20:51:22 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> Hi Gary,
> 
> "Gary Guo" <gary@garyguo.net> writes:
> 
> [...]
> 
> > diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> > index f47afc148ec3..39649443426b 100644
> > --- a/rust/helpers/refcount.c
> > +++ b/rust/helpers/refcount.c
> > @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
> >  	return (refcount_t)REFCOUNT_INIT(n);
> >  }
> >
> > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > +{
> > +	return refcount_read(r);
> > +}  
> 
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> 
> > +
> > +void rust_helper_refcount_set(refcount_t *r, int n)
> > +{
> > +	refcount_set(r, n);
> > +}  
> 
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_set);
> 
> BR Andreas
> 

Helper symbol export is automatic after
e26fa546042a (rust: kbuild: auto generate helper exports)

Best,
Gary

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Fri, 04 Oct 2024 20:51:22 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> Hi Gary,
>>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>> [...]
>>
>> > diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
>> > index f47afc148ec3..39649443426b 100644
>> > --- a/rust/helpers/refcount.c
>> > +++ b/rust/helpers/refcount.c
>> > @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
>> >  	return (refcount_t)REFCOUNT_INIT(n);
>> >  }
>> >
>> > +unsigned int rust_helper_refcount_read(refcount_t *r)
>> > +{
>> > +	return refcount_read(r);
>> > +}
>>
>> +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
>>
>> > +
>> > +void rust_helper_refcount_set(refcount_t *r, int n)
>> > +{
>> > +	refcount_set(r, n);
>> > +}
>>
>> +EXPORT_SYMBOL_GPL(rust_helper_refcount_set);
>>
>> BR Andreas
>>
>
> Helper symbol export is automatic after
> e26fa546042a (rust: kbuild: auto generate helper exports)

All good then 👍 I was applying to 6.11 and got some build errors.
Downstream `rnull` is still on 6.11.

Benchmarks will run next week.

BR Andreas

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Dirk Behme]

Am 04.10.24 um 21:08 schrieb Gary Guo:
> On Fri, 04 Oct 2024 20:51:22 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
>> Hi Gary,
>>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>> [...]
>>
>>> diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
>>> index f47afc148ec3..39649443426b 100644
>>> --- a/rust/helpers/refcount.c
>>> +++ b/rust/helpers/refcount.c
>>> @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
>>>   	return (refcount_t)REFCOUNT_INIT(n);
>>>   }
>>>
>>> +unsigned int rust_helper_refcount_read(refcount_t *r)
>>> +{
>>> +	return refcount_read(r);
>>> +}
>>
>> +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
>>
>>> +
>>> +void rust_helper_refcount_set(refcount_t *r, int n)
>>> +{
>>> +	refcount_set(r, n);
>>> +}
>>
>> +EXPORT_SYMBOL_GPL(rust_helper_refcount_set);
>>
>> BR Andreas
>>
> 
> Helper symbol export is automatic after
> e26fa546042a (rust: kbuild: auto generate helper exports)

Yes :)

What results in a question, though:

When to use exports.h and when not?

There are several users of

#include <linux/export.h>

like

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/helpers/err.c
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/helpers/kunit.c
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/helpers/mutex.c

but some don't use it

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/helpers/rbtree.c
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/helpers/uaccess.c

Any rule?

Best regards

Dirk

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Greg KH]

On Fri, Oct 04, 2024 at 04:52:22PM +0100, Gary Guo wrote:
> This is a wrapping layer of `include/linux/refcount.h`. Currently only
> the most basic operations (read/set/inc/dec/dec_and_test) are implemented,
> additional methods can be implemented when they are needed.
> 
> Currently the kernel refcount has already been used in `Arc`, however it
> calls into FFI directly.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  rust/helpers/refcount.c      | 15 ++++++
>  rust/kernel/sync.rs          |  2 +
>  rust/kernel/sync/refcount.rs | 94 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 rust/kernel/sync/refcount.rs
> 
> diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> index f47afc148ec3..39649443426b 100644
> --- a/rust/helpers/refcount.c
> +++ b/rust/helpers/refcount.c
> @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
>  	return (refcount_t)REFCOUNT_INIT(n);
>  }
>  
> +unsigned int rust_helper_refcount_read(refcount_t *r)
> +{
> +	return refcount_read(r);
> +}

Reading a refcount is almost always a wrong thing to do (it can change
right after you read it), and I don't see any of the later patches in
this series use this call, so can you just drop this?

thanks,

greg k-h

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Gary Guo]

On Sat, 5 Oct 2024 09:40:53 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Oct 04, 2024 at 04:52:22PM +0100, Gary Guo wrote:
> > This is a wrapping layer of `include/linux/refcount.h`. Currently only
> > the most basic operations (read/set/inc/dec/dec_and_test) are implemented,
> > additional methods can be implemented when they are needed.
> > 
> > Currently the kernel refcount has already been used in `Arc`, however it
> > calls into FFI directly.
> > 
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Gary Guo <gary@garyguo.net>
> > ---
> >  rust/helpers/refcount.c      | 15 ++++++
> >  rust/kernel/sync.rs          |  2 +
> >  rust/kernel/sync/refcount.rs | 94 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 111 insertions(+)
> >  create mode 100644 rust/kernel/sync/refcount.rs
> > 
> > diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> > index f47afc148ec3..39649443426b 100644
> > --- a/rust/helpers/refcount.c
> > +++ b/rust/helpers/refcount.c
> > @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
> >  	return (refcount_t)REFCOUNT_INIT(n);
> >  }
> >  
> > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > +{
> > +	return refcount_read(r);
> > +}  
> 
> Reading a refcount is almost always a wrong thing to do (it can change
> right after you read it), and I don't see any of the later patches in
> this series use this call, so can you just drop this?
> 
> thanks,
> 
> greg k-h

I originally introduced this thinking I can replace Andreas's atomic
2->0 operation with a read + set, but ended up couldn't do it.

The refcount read is still useful to determine if the current value is
1 -- in fact, `Arc::into_unique_or_drop` could use this rather than
decrementing the refcount and then incrementing it again (just doing a
refcount read would be much better codegen-wise than the current
behaviour). I'll probably make this change in the next version of the
series.

It might also be useful for debugging, so we can have a `Debug` impl
for `Refcount` which prints out the current value. But I didn't
introduce it due to no user.

Best,
Gary

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Gary Guo]

On Sat, 5 Oct 2024 14:31:06 +0100
Gary Guo <gary@garyguo.net> wrote:

> On Sat, 5 Oct 2024 09:40:53 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Oct 04, 2024 at 04:52:22PM +0100, Gary Guo wrote:  
> > > This is a wrapping layer of `include/linux/refcount.h`. Currently only
> > > the most basic operations (read/set/inc/dec/dec_and_test) are implemented,
> > > additional methods can be implemented when they are needed.
> > > 
> > > Currently the kernel refcount has already been used in `Arc`, however it
> > > calls into FFI directly.
> > > 
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Gary Guo <gary@garyguo.net>
> > > ---
> > >  rust/helpers/refcount.c      | 15 ++++++
> > >  rust/kernel/sync.rs          |  2 +
> > >  rust/kernel/sync/refcount.rs | 94 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 111 insertions(+)
> > >  create mode 100644 rust/kernel/sync/refcount.rs
> > > 
> > > diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> > > index f47afc148ec3..39649443426b 100644
> > > --- a/rust/helpers/refcount.c
> > > +++ b/rust/helpers/refcount.c
> > > @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
> > >  	return (refcount_t)REFCOUNT_INIT(n);
> > >  }
> > >  
> > > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > > +{
> > > +	return refcount_read(r);
> > > +}    
> > 
> > Reading a refcount is almost always a wrong thing to do (it can change
> > right after you read it), and I don't see any of the later patches in
> > this series use this call, so can you just drop this?
> > 
> > thanks,
> > 
> > greg k-h  
> 
> I originally introduced this thinking I can replace Andreas's atomic
> 2->0 operation with a read + set, but ended up couldn't do it.
> 
> The refcount read is still useful to determine if the current value is
> 1 -- in fact, `Arc::into_unique_or_drop` could use this rather than
> decrementing the refcount and then incrementing it again (just doing a
> refcount read would be much better codegen-wise than the current
> behaviour). I'll probably make this change in the next version of the
> series.

Actually `into_unique_or_drop` can't use this because it needs to avoid
running destructor when it races with other threads. The semantics for
that function is better reflected with `refcount_dec_not_one`, which
I'll introduce in v2, and I'll drop `read` in v2.

Best,
Gary

> 
> It might also be useful for debugging, so we can have a `Debug` impl
> for `Refcount` which prints out the current value. But I didn't
> introduce it due to no user.
> 
> Best,
> Gary
> 

Re: [PATCH 1/3] rust: implement `kernel::sync::Refcount`

[Alice Ryhl]

On Sat, Oct 5, 2024 at 4:26 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Sat, 5 Oct 2024 14:31:06 +0100
> Gary Guo <gary@garyguo.net> wrote:
>
> > On Sat, 5 Oct 2024 09:40:53 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > > On Fri, Oct 04, 2024 at 04:52:22PM +0100, Gary Guo wrote:
> > > > This is a wrapping layer of `include/linux/refcount.h`. Currently only
> > > > the most basic operations (read/set/inc/dec/dec_and_test) are implemented,
> > > > additional methods can be implemented when they are needed.
> > > >
> > > > Currently the kernel refcount has already been used in `Arc`, however it
> > > > calls into FFI directly.
> > > >
> > > > Cc: Will Deacon <will@kernel.org>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Signed-off-by: Gary Guo <gary@garyguo.net>
> > > > ---
> > > >  rust/helpers/refcount.c      | 15 ++++++
> > > >  rust/kernel/sync.rs          |  2 +
> > > >  rust/kernel/sync/refcount.rs | 94 ++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 111 insertions(+)
> > > >  create mode 100644 rust/kernel/sync/refcount.rs
> > > >
> > > > diff --git a/rust/helpers/refcount.c b/rust/helpers/refcount.c
> > > > index f47afc148ec3..39649443426b 100644
> > > > --- a/rust/helpers/refcount.c
> > > > +++ b/rust/helpers/refcount.c
> > > > @@ -8,11 +8,26 @@ refcount_t rust_helper_REFCOUNT_INIT(int n)
> > > >   return (refcount_t)REFCOUNT_INIT(n);
> > > >  }
> > > >
> > > > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > > > +{
> > > > + return refcount_read(r);
> > > > +}
> > >
> > > Reading a refcount is almost always a wrong thing to do (it can change
> > > right after you read it), and I don't see any of the later patches in
> > > this series use this call, so can you just drop this?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I originally introduced this thinking I can replace Andreas's atomic
> > 2->0 operation with a read + set, but ended up couldn't do it.
> >
> > The refcount read is still useful to determine if the current value is
> > 1 -- in fact, `Arc::into_unique_or_drop` could use this rather than
> > decrementing the refcount and then incrementing it again (just doing a
> > refcount read would be much better codegen-wise than the current
> > behaviour). I'll probably make this change in the next version of the
> > series.
>
> Actually `into_unique_or_drop` can't use this because it needs to avoid
> running destructor when it races with other threads. The semantics for
> that function is better reflected with `refcount_dec_not_one`, which
> I'll introduce in v2, and I'll drop `read` in v2.

Ah, I did not know that C had a refcount_dec_not_one. Yeah, that's
exactly what into_unique_or_drop does.

Though I don't know if the cmpxchg loop is really more efficient than
just doing an infallible decrement like I do right now?

Alice

[PATCH 2/3] rust: convert `Arc` to use `Refcount`

[Gary Guo]

With `Refcount` type created, `Arc` can use `Refcount` instead of
calling into FFI directly.

Cc: Will Deacon <will@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 rust/kernel/sync/arc.rs | 45 +++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3021f30fd822..a1fa58b127c8 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -8,7 +8,7 @@
 //! threads.
 //!
 //! It is different from the standard library's [`Arc`] in a few ways:
-//! 1. It is backed by the kernel's `refcount_t` type.
+//! 1. It is backed by the kernel's [`Refcount`] type.
 //! 2. It does not support weak references, which allows it to be half the size.
 //! 3. It saturates the reference count instead of aborting when it goes over a threshold.
 //! 4. It does not provide a `get_mut` method, so the ref counted object is pinned.
@@ -18,10 +18,10 @@
 
 use crate::{
     alloc::{box_ext::BoxExt, AllocError, Flags},
-    bindings,
     init::{self, InPlaceInit, Init, PinInit},
+    sync::Refcount,
     try_init,
-    types::{ForeignOwnable, Opaque},
+    types::ForeignOwnable,
 };
 use alloc::boxed::Box;
 use core::{
@@ -134,7 +134,7 @@ pub struct Arc<T: ?Sized> {
 #[pin_data]
 #[repr(C)]
 struct ArcInner<T: ?Sized> {
-    refcount: Opaque<bindings::refcount_t>,
+    refcount: Refcount,
     data: T,
 }
 
@@ -146,7 +146,7 @@ impl<T: ?Sized> ArcInner<T> {
     /// `ptr` must have been returned by a previous call to [`Arc::into_raw`], and the `Arc` must
     /// not yet have been destroyed.
     unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> {
-        let refcount_layout = Layout::new::<bindings::refcount_t>();
+        let refcount_layout = Layout::new::<Refcount>();
         // SAFETY: The caller guarantees that the pointer is valid.
         let val_layout = Layout::for_value(unsafe { &*ptr });
         // SAFETY: We're computing the layout of a real struct that existed when compiling this
@@ -199,8 +199,7 @@ impl<T> Arc<T> {
     pub fn new(contents: T, flags: Flags) -> Result<Self, AllocError> {
         // INVARIANT: The refcount is initialised to a non-zero value.
         let value = ArcInner {
-            // SAFETY: There are no safety requirements for this FFI call.
-            refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
+            refcount: Refcount::new(1),
             data: contents,
         };
 
@@ -308,18 +307,15 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
         // We will manually manage the refcount in this method, so we disable the destructor.
         let me = ManuallyDrop::new(self);
         // SAFETY: We own a refcount, so the pointer is still valid.
-        let refcount = unsafe { me.ptr.as_ref() }.refcount.get();
+        let refcount = unsafe { &me.ptr.as_ref().refcount };
 
         // If the refcount reaches a non-zero value, then we have destroyed this `Arc` and will
         // return without further touching the `Arc`. If the refcount reaches zero, then there are
         // no other arcs, and we can create a `UniqueArc`.
-        //
-        // SAFETY: We own a refcount, so the pointer is not dangling.
-        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
+        let is_zero = refcount.dec_and_test();
         if is_zero {
-            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
-            // accesses to the refcount.
-            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
+            // We have exclusive access to the arc, so we can modify the refcount at will.
+            refcount.set(1);
 
             // INVARIANT: We own the only refcount to this arc, so we may create a `UniqueArc`. We
             // must pin the `UniqueArc` because the values was previously in an `Arc`, and they pin
@@ -376,10 +372,10 @@ fn as_ref(&self) -> &T {
 
 impl<T: ?Sized> Clone for Arc<T> {
     fn clone(&self) -> Self {
-        // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero.
+        // INVARIANT: `Refcount` saturates the refcount, so it cannot overflow to zero.
         // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
         // safe to increment the refcount.
-        unsafe { bindings::refcount_inc(self.ptr.as_ref().refcount.get()) };
+        unsafe { self.ptr.as_ref().refcount.inc() };
 
         // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
         unsafe { Self::from_inner(self.ptr) }
@@ -388,16 +384,14 @@ fn clone(&self) -> Self {
 
 impl<T: ?Sized> Drop for Arc<T> {
     fn drop(&mut self) {
-        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
-        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
-        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
-        // freed/invalid memory as long as it is never dereferenced.
-        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
-
         // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
         // this instance is being dropped, so the broken invariant is not observable.
-        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
-        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
+        // SAFETY: By the type invariant, there is necessarily a reference to the object.
+        // NOTE: we cannot touch `refcount` after it's decremented to a non-zero value because
+        // another thread/CPU may concurrently decrement it to zero and free it. However it is okay
+        // to have a transient reference to decrement the refcount, see
+        // https://github.com/rust-lang/rust/issues/55005.
+        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };
         if is_zero {
             // The count reached zero, we must free the memory.
             //
@@ -649,8 +643,7 @@ pub fn new_uninit(flags: Flags) -> Result<UniqueArc<MaybeUninit<T>>, AllocError>
         // INVARIANT: The refcount is initialised to a non-zero value.
         let inner = Box::try_init::<AllocError>(
             try_init!(ArcInner {
-                // SAFETY: There are no safety requirements for this FFI call.
-                refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
+                refcount: Refcount::new(1),
                 data <- init::uninit::<T, AllocError>(),
             }? AllocError),
             flags,
-- 
2.44.1

Re: [PATCH 2/3] rust: convert `Arc` to use `Refcount`

[Alice Ryhl]

On Fri, Oct 4, 2024 at 5:53 PM Gary Guo <gary@garyguo.net> wrote:
>
> With `Refcount` type created, `Arc` can use `Refcount` instead of
> calling into FFI directly.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Gary Guo <gary@garyguo.net>

[...]

> -            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> -            // accesses to the refcount.
> -            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> +            // We have exclusive access to the arc, so we can modify the refcount at will.
> +            refcount.set(1);

Why are you changing this to an atomic write? We just took ownership,
so we have exclusive access and can perform an unsynchronized write.

>  impl<T: ?Sized> Drop for Arc<T> {
>      fn drop(&mut self) {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> -        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> -        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> -        // freed/invalid memory as long as it is never dereferenced.
> -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> -
>          // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
>          // this instance is being dropped, so the broken invariant is not observable.
> -        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
> -        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object.
> +        // NOTE: we cannot touch `refcount` after it's decremented to a non-zero value because
> +        // another thread/CPU may concurrently decrement it to zero and free it. However it is okay
> +        // to have a transient reference to decrement the refcount, see
> +        // https://github.com/rust-lang/rust/issues/55005.
> +        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };

This code needs to make use of this guarantee for correctness:

For both `&T` without `UnsafeCell<_>` and `&mut T`, you must also not
deallocate the data until the reference expires. As a special
exception, given an `&T`, any part of it that is inside an
`UnsafeCell<_>` may be deallocated during the lifetime of the
reference, after the last time the reference is used (dereferenced or
reborrowed). Since you cannot deallocate a part of what a reference
points to, this means the memory an `&T` points to can be deallocated
only if *every part of it* (including padding) is inside an
`UnsafeCell`.

from https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html

so when invoking `dec_and_test()` you can have a reference to the
`Refcount`, but not necessarily to other parts of `ArcInner` like you
do here.

Alice

Re: [PATCH 2/3] rust: convert `Arc` to use `Refcount`

[Gary Guo]

On Sat, 5 Oct 2024 14:06:48 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, Oct 4, 2024 at 5:53 PM Gary Guo <gary@garyguo.net> wrote:
> >
> > With `Refcount` type created, `Arc` can use `Refcount` instead of
> > calling into FFI directly.
> >
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Gary Guo <gary@garyguo.net>  
> 
> [...]
> 
> > -            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
> > -            // accesses to the refcount.
> > -            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
> > +            // We have exclusive access to the arc, so we can modify the refcount at will.
> > +            refcount.set(1);  
> 
> Why are you changing this to an atomic write? We just took ownership,
> so we have exclusive access and can perform an unsynchronized write.

Because I can avoid an unsafe, and use the new method. This is a
relaxed write, so I don't think there'll be any real difference.

> 
> >  impl<T: ?Sized> Drop for Arc<T> {
> >      fn drop(&mut self) {
> > -        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> > -        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> > -        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> > -        // freed/invalid memory as long as it is never dereferenced.
> > -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> > -
> >          // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
> >          // this instance is being dropped, so the broken invariant is not observable.
> > -        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
> > -        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> > +        // SAFETY: By the type invariant, there is necessarily a reference to the object.
> > +        // NOTE: we cannot touch `refcount` after it's decremented to a non-zero value because
> > +        // another thread/CPU may concurrently decrement it to zero and free it. However it is okay
> > +        // to have a transient reference to decrement the refcount, see
> > +        // https://github.com/rust-lang/rust/issues/55005.
> > +        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };  
> 
> This code needs to make use of this guarantee for correctness:
> 
> For both `&T` without `UnsafeCell<_>` and `&mut T`, you must also not
> deallocate the data until the reference expires. As a special
> exception, given an `&T`, any part of it that is inside an
> `UnsafeCell<_>` may be deallocated during the lifetime of the
> reference, after the last time the reference is used (dereferenced or
> reborrowed). Since you cannot deallocate a part of what a reference
> points to, this means the memory an `&T` points to can be deallocated
> only if *every part of it* (including padding) is inside an
> `UnsafeCell`.
> 
> from https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html
> 
> so when invoking `dec_and_test()` you can have a reference to the
> `Refcount`, but not necessarily to other parts of `ArcInner` like you
> do here.

This is fine.

A reference only lives until it's last used. This has been the case
ever since NLL. This is totally legal code:

	let x = Box::new(42);
	let y = &*x;
	drop(x);

This is true for safe code and unsafe code. You can dereference a
pointer and hold onto a reference, and then free the pointer. As long
as you don't use that reference after it's freed, it's fine.

The only exception is when the reference is passed in through a
function parameter, and then the text you quoted matters. Normally these
references are assumed to be alive for the duration of function, except
when `UnsafeCell` is used, then this requirement is cancelled. I linked
to a closed Rust issue, and I think the paragraph you have quoted is a
direct result of it.

Now back to this particular case. Before calling `dec_and_test`, the
entire `ArcInner` is still alive, so it's valid to create a reference
to it. That reference is then used to derive a reference to the
`Refcount` field, which is needed for the function call. Then we call
`dec_and_test`. The call is okay because all bytes `Refcount` are
covered by `UnsafeCell`. The reference to `ArcInner` is never used
after the call, so we are all good.

In fact, the standard library also does
`self.inner().refcount.fetch_sub`, which also creates this transient
reference to `ArcInner`.

Best,
Gary

[PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Gary Guo]

Currently there's a custom reference counting in `block::mq`, which uses
`AtomicU64` Rust atomics, and this type doesn't exist on some 32-bit
architectures. We cannot just change it to use 32-bit atomics, because
doing so will make it vulnerable to refcount overflow. So switch it to
use the kernel refcount `kernel::sync::Refcount` instead.

There is an operation needed by `block::mq`, atomically decreasing
refcount from 2 to 0, which is not available through refcount.h, so
I exposed `Refcount::as_atomic` which allows accessing the refcount
directly.

Cc: Will Deacon <will@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 rust/kernel/block/mq/operations.rs |  7 +--
 rust/kernel/block/mq/request.rs    | 70 ++++++++++--------------------
 rust/kernel/sync/refcount.rs       | 12 +++++
 3 files changed, 38 insertions(+), 51 deletions(-)

diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
index 9ba7fdfeb4b2..36ee5f96c66d 100644
--- a/rust/kernel/block/mq/operations.rs
+++ b/rust/kernel/block/mq/operations.rs
@@ -9,9 +9,10 @@
     block::mq::request::RequestDataWrapper,
     block::mq::Request,
     error::{from_result, Result},
+    sync::Refcount,
     types::ARef,
 };
-use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering};
+use core::marker::PhantomData;
 
 /// Implement this trait to interface blk-mq as block devices.
 ///
@@ -77,7 +78,7 @@ impl<T: Operations> OperationsVTable<T> {
         let request = unsafe { &*(*bd).rq.cast::<Request<T>>() };
 
         // One refcount for the ARef, one for being in flight
-        request.wrapper_ref().refcount().store(2, Ordering::Relaxed);
+        request.wrapper_ref().refcount().set(2);
 
         // SAFETY:
         //  - We own a refcount that we took above. We pass that to `ARef`.
@@ -186,7 +187,7 @@ impl<T: Operations> OperationsVTable<T> {
 
             // SAFETY: The refcount field is allocated but not initialized, so
             // it is valid for writes.
-            unsafe { RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(AtomicU64::new(0)) };
+            unsafe { RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(Refcount::new(0)) };
 
             Ok(0)
         })
diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
index a0e22827f3f4..7b63c02bdce7 100644
--- a/rust/kernel/block/mq/request.rs
+++ b/rust/kernel/block/mq/request.rs
@@ -8,12 +8,13 @@
     bindings,
     block::mq::Operations,
     error::Result,
+    sync::Refcount,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
 use core::{
     marker::PhantomData,
     ptr::{addr_of_mut, NonNull},
-    sync::atomic::{AtomicU64, Ordering},
+    sync::atomic::Ordering,
 };
 
 /// A wrapper around a blk-mq `struct request`. This represents an IO request.
@@ -37,6 +38,9 @@
 /// We need to track C and D to ensure that it is safe to end the request and hand
 /// back ownership to the block layer.
 ///
+/// Note that driver can still obtain new `ARef` even there is no `ARef`s in existence by using
+/// `tag_to_rq`, hence the need to distinct B and C.
+///
 /// The states are tracked through the private `refcount` field of
 /// `RequestDataWrapper`. This structure lives in the private data area of the C
 /// `struct request`.
@@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
     /// C `struct request`. If the operation fails, `this` is returned in the
     /// `Err` variant.
     fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
-        // We can race with `TagSet::tag_to_rq`
-        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
-            2,
-            0,
-            Ordering::Relaxed,
-            Ordering::Relaxed,
-        ) {
+        // To hand back the ownership, we need the current refcount to be 2.
+        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
+        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
+        // atomics directly.
+        if this
+            .wrapper_ref()
+            .refcount()
+            .as_atomic()
+            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
+            .is_err()
+        {
             return Err(this);
         }
 
@@ -159,13 +167,13 @@ pub(crate) struct RequestDataWrapper {
     /// - 0: The request is owned by C block layer.
     /// - 1: The request is owned by Rust abstractions but there are no ARef references to it.
     /// - 2+: There are `ARef` references to the request.
-    refcount: AtomicU64,
+    refcount: Refcount,
 }
 
 impl RequestDataWrapper {
     /// Return a reference to the refcount of the request that is embedding
     /// `self`.
-    pub(crate) fn refcount(&self) -> &AtomicU64 {
+    pub(crate) fn refcount(&self) -> &Refcount {
         &self.refcount
     }
 
@@ -175,7 +183,7 @@ pub(crate) fn refcount(&self) -> &AtomicU64 {
     /// # Safety
     ///
     /// - `this` must point to a live allocation of at least the size of `Self`.
-    pub(crate) unsafe fn refcount_ptr(this: *mut Self) -> *mut AtomicU64 {
+    pub(crate) unsafe fn refcount_ptr(this: *mut Self) -> *mut Refcount {
         // SAFETY: Because of the safety requirements of this function, the
         // field projection is safe.
         unsafe { addr_of_mut!((*this).refcount) }
@@ -191,47 +199,13 @@ unsafe impl<T: Operations> Send for Request<T> {}
 // mutate `self` are internally synchronized`
 unsafe impl<T: Operations> Sync for Request<T> {}
 
-/// Store the result of `op(target.load())` in target, returning new value of
-/// target.
-fn atomic_relaxed_op_return(target: &AtomicU64, op: impl Fn(u64) -> u64) -> u64 {
-    let old = target.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |x| Some(op(x)));
-
-    // SAFETY: Because the operation passed to `fetch_update` above always
-    // return `Some`, `old` will always be `Ok`.
-    let old = unsafe { old.unwrap_unchecked() };
-
-    op(old)
-}
-
-/// Store the result of `op(target.load)` in `target` if `target.load() !=
-/// pred`, returning true if the target was updated.
-fn atomic_relaxed_op_unless(target: &AtomicU64, op: impl Fn(u64) -> u64, pred: u64) -> bool {
-    target
-        .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |x| {
-            if x == pred {
-                None
-            } else {
-                Some(op(x))
-            }
-        })
-        .is_ok()
-}
-
 // SAFETY: All instances of `Request<T>` are reference counted. This
 // implementation of `AlwaysRefCounted` ensure that increments to the ref count
 // keeps the object alive in memory at least until a matching reference count
 // decrement is executed.
 unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
     fn inc_ref(&self) {
-        let refcount = &self.wrapper_ref().refcount();
-
-        #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))]
-        let updated = atomic_relaxed_op_unless(refcount, |x| x + 1, 0);
-
-        #[cfg(CONFIG_DEBUG_MISC)]
-        if !updated {
-            panic!("Request refcount zero on clone")
-        }
+        self.wrapper_ref().refcount().inc();
     }
 
     unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
@@ -243,10 +217,10 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
         let refcount = unsafe { &*RequestDataWrapper::refcount_ptr(wrapper_ptr) };
 
         #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))]
-        let new_refcount = atomic_relaxed_op_return(refcount, |x| x - 1);
+        let is_zero = refcount.dec_and_test();
 
         #[cfg(CONFIG_DEBUG_MISC)]
-        if new_refcount == 0 {
+        if is_zero {
             panic!("Request reached refcount zero in Rust abstractions");
         }
     }
diff --git a/rust/kernel/sync/refcount.rs b/rust/kernel/sync/refcount.rs
index 4a5b815adc05..2cdcb20e8ee2 100644
--- a/rust/kernel/sync/refcount.rs
+++ b/rust/kernel/sync/refcount.rs
@@ -31,6 +31,18 @@ fn as_ptr(&self) -> *mut bindings::refcount_t {
         self.0.get()
     }
 
+    /// Get the underlying atomic counter that backs the refcount.
+    ///
+    /// NOTE: This will be changed to LKMM atomic in the future.
+    #[inline]
+    pub fn as_atomic(&self) -> &AtomicI32 {
+        let ptr = self.0.get() as *const AtomicI32;
+        // SAFETY: `refcount_t` is a transparent wrapper of `atomic_t`, which is an atomic 32-bit
+        // integer that is layout-wise compatible with `AtomicI32`. All values are valid for
+        // `refcount_t`, despite some of the values are considered saturated and "bad".
+        unsafe { &*ptr }
+    }
+
     /// Get a refcount's value.
     #[inline]
     pub fn read(&self) -> i32 {
-- 
2.44.1

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Dirk Behme]

Am 04.10.24 um 17:52 schrieb Gary Guo:
> Currently there's a custom reference counting in `block::mq`, which uses
> `AtomicU64` Rust atomics, and this type doesn't exist on some 32-bit
> architectures. We cannot just change it to use 32-bit atomics, because
> doing so will make it vulnerable to refcount overflow. So switch it to
> use the kernel refcount `kernel::sync::Refcount` instead.
> 
> There is an operation needed by `block::mq`, atomically decreasing
> refcount from 2 to 0, which is not available through refcount.h, so
> I exposed `Refcount::as_atomic` which allows accessing the refcount
> directly.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
...
> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
> index a0e22827f3f4..7b63c02bdce7 100644
> --- a/rust/kernel/block/mq/request.rs
> +++ b/rust/kernel/block/mq/request.rs
....
> @@ -37,6 +38,9 @@
>   /// We need to track C and D to ensure that it is safe to end the request and hand
>   /// back ownership to the block layer.
>   ///
> +/// Note that driver can still obtain new `ARef` even there is no `ARef`s in existence by using

Do you like to check this sentence? Maybe:

Note that "a" driver can still obtain "a" new `ARef` even "if" there 
"are" no `ARef`s in existence by using ....

?

Best regards

Dirk

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

Hi Gary,

"Gary Guo" <gary@garyguo.net> writes:

> Currently there's a custom reference counting in `block::mq`, which uses
> `AtomicU64` Rust atomics, and this type doesn't exist on some 32-bit
> architectures. We cannot just change it to use 32-bit atomics, because
> doing so will make it vulnerable to refcount overflow. So switch it to
> use the kernel refcount `kernel::sync::Refcount` instead.
>
> There is an operation needed by `block::mq`, atomically decreasing
> refcount from 2 to 0, which is not available through refcount.h, so
> I exposed `Refcount::as_atomic` which allows accessing the refcount
> directly.

I would rather wait with this patch until the helper LTO patches land
upstream. Or at least let me run the benchmarks to see the effect of not
inlining these refcount operations.

Best regards,
Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Gary Guo]

On Fri, 04 Oct 2024 20:05:41 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> Hi Gary,
> 
> "Gary Guo" <gary@garyguo.net> writes:
> 
> > Currently there's a custom reference counting in `block::mq`, which uses
> > `AtomicU64` Rust atomics, and this type doesn't exist on some 32-bit
> > architectures. We cannot just change it to use 32-bit atomics, because
> > doing so will make it vulnerable to refcount overflow. So switch it to
> > use the kernel refcount `kernel::sync::Refcount` instead.
> >
> > There is an operation needed by `block::mq`, atomically decreasing
> > refcount from 2 to 0, which is not available through refcount.h, so
> > I exposed `Refcount::as_atomic` which allows accessing the refcount
> > directly.  
> 
> I would rather wait with this patch until the helper LTO patches land
> upstream. Or at least let me run the benchmarks to see the effect of not
> inlining these refcount operations.
> 
> Best regards,
> Andreas
> 

The helper LTO patch series still need some time. I'd want to be able to
test on 32-bit archs in the meantime.

Best,
Gary

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Fri, 04 Oct 2024 20:05:41 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> Hi Gary,
>>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>> > Currently there's a custom reference counting in `block::mq`, which uses
>> > `AtomicU64` Rust atomics, and this type doesn't exist on some 32-bit
>> > architectures. We cannot just change it to use 32-bit atomics, because
>> > doing so will make it vulnerable to refcount overflow. So switch it to
>> > use the kernel refcount `kernel::sync::Refcount` instead.
>> >
>> > There is an operation needed by `block::mq`, atomically decreasing
>> > refcount from 2 to 0, which is not available through refcount.h, so
>> > I exposed `Refcount::as_atomic` which allows accessing the refcount
>> > directly.
>>
>> I would rather wait with this patch until the helper LTO patches land
>> upstream. Or at least let me run the benchmarks to see the effect of not
>> inlining these refcount operations.
>>
>> Best regards,
>> Andreas
>>
>
> The helper LTO patch series still need some time. I'd want to be able to
> test on 32-bit archs in the meantime.

In that case I would rather just conditionally gate the `block` module
and the `rnull` driver on 64 bit arch.

BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

Hi Gary,

"Gary Guo" <gary@garyguo.net> writes:

[...]

>  // SAFETY: All instances of `Request<T>` are reference counted. This
>  // implementation of `AlwaysRefCounted` ensure that increments to the ref count
>  // keeps the object alive in memory at least until a matching reference count
>  // decrement is executed.
>  unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
>      fn inc_ref(&self) {
> -        let refcount = &self.wrapper_ref().refcount();
> -
> -        #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))]
> -        let updated = atomic_relaxed_op_unless(refcount, |x| x + 1, 0);
> -
> -        #[cfg(CONFIG_DEBUG_MISC)]
> -        if !updated {
> -            panic!("Request refcount zero on clone")
> -        }
> +        self.wrapper_ref().refcount().inc();

What happened to the debug code?


BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Gary Guo]

On Fri, 04 Oct 2024 20:34:01 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> Hi Gary,
> 
> "Gary Guo" <gary@garyguo.net> writes:
> 
> [...]
> 
> >  // SAFETY: All instances of `Request<T>` are reference counted. This
> >  // implementation of `AlwaysRefCounted` ensure that increments to the ref count
> >  // keeps the object alive in memory at least until a matching reference count
> >  // decrement is executed.
> >  unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
> >      fn inc_ref(&self) {
> > -        let refcount = &self.wrapper_ref().refcount();
> > -
> > -        #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))]
> > -        let updated = atomic_relaxed_op_unless(refcount, |x| x + 1, 0);
> > -
> > -        #[cfg(CONFIG_DEBUG_MISC)]
> > -        if !updated {
> > -            panic!("Request refcount zero on clone")
> > -        }
> > +        self.wrapper_ref().refcount().inc();  
> 
> What happened to the debug code?
> 
> 
> BR Andreas
> 

`refcount_inc` will WARN and saturate the refcount when trying to
increment from zero. This is true regardless of config.

I've already captured this in `Refcount::inc` documentation.

Best,
Gary

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Fri, 04 Oct 2024 20:34:01 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> Hi Gary,
>>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>> [...]
>>
>> >  // SAFETY: All instances of `Request<T>` are reference counted. This
>> >  // implementation of `AlwaysRefCounted` ensure that increments to the ref count
>> >  // keeps the object alive in memory at least until a matching reference count
>> >  // decrement is executed.
>> >  unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
>> >      fn inc_ref(&self) {
>> > -        let refcount = &self.wrapper_ref().refcount();
>> > -
>> > -        #[cfg_attr(not(CONFIG_DEBUG_MISC), allow(unused_variables))]
>> > -        let updated = atomic_relaxed_op_unless(refcount, |x| x + 1, 0);
>> > -
>> > -        #[cfg(CONFIG_DEBUG_MISC)]
>> > -        if !updated {
>> > -            panic!("Request refcount zero on clone")
>> > -        }
>> > +        self.wrapper_ref().refcount().inc();
>>
>> What happened to the debug code?
>>
>>
>> BR Andreas
>>
>
> `refcount_inc` will WARN and saturate the refcount when trying to
> increment from zero. This is true regardless of config.
>
> I've already captured this in `Refcount::inc` documentation.

I did not know. Thanks!

BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Greg KH]

On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> There is an operation needed by `block::mq`, atomically decreasing
> refcount from 2 to 0, which is not available through refcount.h, so
> I exposed `Refcount::as_atomic` which allows accessing the refcount
> directly.

That's scary, and of course feels wrong on many levels, but:


> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>      /// C `struct request`. If the operation fails, `this` is returned in the
>      /// `Err` variant.
>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> -        // We can race with `TagSet::tag_to_rq`
> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> -            2,
> -            0,
> -            Ordering::Relaxed,
> -            Ordering::Relaxed,
> -        ) {
> +        // To hand back the ownership, we need the current refcount to be 2.
> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> +        // atomics directly.
> +        if this
> +            .wrapper_ref()
> +            .refcount()
> +            .as_atomic()
> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> +            .is_err()

Why not just call rust_helper_refcount_set()?  Or is the issue that you
think you might not be 2 here?  And if you HAVE to be 2, why that magic
value (i.e. why not just always be 1 and rely on normal
increment/decrement?)

I know some refcounts are odd in the kernel, but I don't see where the
block layer is caring about 2 as a refcount anywhere, what am I missing?

thanks,

greg k-h

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

Hi Greg,

"Greg KH" <gregkh@linuxfoundation.org> writes:

> On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>> There is an operation needed by `block::mq`, atomically decreasing
>> refcount from 2 to 0, which is not available through refcount.h, so
>> I exposed `Refcount::as_atomic` which allows accessing the refcount
>> directly.
>
> That's scary, and of course feels wrong on many levels, but:
>
>
>> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>>      /// C `struct request`. If the operation fails, `this` is returned in the
>>      /// `Err` variant.
>>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>> -        // We can race with `TagSet::tag_to_rq`
>> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>> -            2,
>> -            0,
>> -            Ordering::Relaxed,
>> -            Ordering::Relaxed,
>> -        ) {
>> +        // To hand back the ownership, we need the current refcount to be 2.
>> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>> +        // atomics directly.
>> +        if this
>> +            .wrapper_ref()
>> +            .refcount()
>> +            .as_atomic()
>> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>> +            .is_err()
>
> Why not just call rust_helper_refcount_set()?  Or is the issue that you
> think you might not be 2 here?  And if you HAVE to be 2, why that magic
> value (i.e. why not just always be 1 and rely on normal
> increment/decrement?)
>
> I know some refcounts are odd in the kernel, but I don't see where the
> block layer is caring about 2 as a refcount anywhere, what am I missing?

It is in the documentation, rendered version available here [1]. Let me
know if it is still unclear, then I guess we need to update the docs.

Also, my session from Recipes has a little bit of discussion regarding
this refcount and it's use [2].

Best regards,
Andreas


[1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
[2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Greg KH]

On Sat, Oct 05, 2024 at 11:48:53AM +0200, Andreas Hindborg wrote:
> Hi Greg,
> 
> "Greg KH" <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> >> There is an operation needed by `block::mq`, atomically decreasing
> >> refcount from 2 to 0, which is not available through refcount.h, so
> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> >> directly.
> >
> > That's scary, and of course feels wrong on many levels, but:
> >
> >
> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> >>      /// C `struct request`. If the operation fails, `this` is returned in the
> >>      /// `Err` variant.
> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> >> -        // We can race with `TagSet::tag_to_rq`
> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> >> -            2,
> >> -            0,
> >> -            Ordering::Relaxed,
> >> -            Ordering::Relaxed,
> >> -        ) {
> >> +        // To hand back the ownership, we need the current refcount to be 2.
> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> >> +        // atomics directly.
> >> +        if this
> >> +            .wrapper_ref()
> >> +            .refcount()
> >> +            .as_atomic()
> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> >> +            .is_err()
> >
> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > value (i.e. why not just always be 1 and rely on normal
> > increment/decrement?)
> >
> > I know some refcounts are odd in the kernel, but I don't see where the
> > block layer is caring about 2 as a refcount anywhere, what am I missing?
> 
> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.
> 
> Also, my session from Recipes has a little bit of discussion regarding
> this refcount and it's use [2].

Ah, ick, that's crazy, ok, good luck!

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Peter Zijlstra]

On Sat, Oct 05, 2024 at 11:48:53AM +0200, Andreas Hindborg wrote:

> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.

> 
> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details

So I clicked on the link for shits and giggles, and OMG that's
unreadable garbage :/ Is there a plain text form that a normal person
can read?

There's just too much 'layout' and fonts and colours and URGH.

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Peter Zijlstra" <peterz@infradead.org> writes:

> On Sat, Oct 05, 2024 at 11:48:53AM +0200, Andreas Hindborg wrote:
>
>> It is in the documentation, rendered version available here [1]. Let me
>> know if it is still unclear, then I guess we need to update the docs.
>
>>
>> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>
> So I clicked on the link for shits and giggles, and OMG that's
> unreadable garbage :/ Is there a plain text form that a normal person
> can read?

To each his own, I guess. It is rendered from the source code. There is
a source link at the top of the page that will take you to a rendered
version of the source code, line numbers, syntax highlighting and all.

You can open up the source file in your favorite pager and read it there
as well. This particular text is from rust/kernel/block/mq/request.rs.

BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Miguel Ojeda]

On Sat, Oct 5, 2024 at 12:10 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> So I clicked on the link for shits and giggles, and OMG that's
> unreadable garbage :/ Is there a plain text form that a normal person
> can read?
>
> There's just too much 'layout' and fonts and colours and URGH.

If fonts and colors are the only issue, then it can easily be fixed
with a bit of CSS client-side or we can perhaps add it to a new theme.

Otherwise, people have implemented other renderers and viewers in the
past, including text / terminal-based ones. Nowadays there is unstable
JSON output support that can be used for that without dealing with
HTML:

    https://rust-lang.github.io/rfcs/2963-rustdoc-json.html

If you want to use rust.docs.kernel.org, you can also use the "source"
view at the top-right. It is still syntax highlighted a bit -- not
sure if you like that, but you may find it "less busy".

Having said that, there is some logic in the layout (in the non-source
view, I mean) being the way it is in the HTML view -- it may take time
to get used to, but it is quite useful when you know where to look /
click.

Cheers,
Miguel

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Alice Ryhl]

On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Hi Greg,
>
> "Greg KH" <gregkh@linuxfoundation.org> writes:
>
> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> >> There is an operation needed by `block::mq`, atomically decreasing
> >> refcount from 2 to 0, which is not available through refcount.h, so
> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> >> directly.
> >
> > That's scary, and of course feels wrong on many levels, but:
> >
> >
> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> >>      /// C `struct request`. If the operation fails, `this` is returned in the
> >>      /// `Err` variant.
> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> >> -        // We can race with `TagSet::tag_to_rq`
> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> >> -            2,
> >> -            0,
> >> -            Ordering::Relaxed,
> >> -            Ordering::Relaxed,
> >> -        ) {
> >> +        // To hand back the ownership, we need the current refcount to be 2.
> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> >> +        // atomics directly.
> >> +        if this
> >> +            .wrapper_ref()
> >> +            .refcount()
> >> +            .as_atomic()
> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> >> +            .is_err()
> >
> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > value (i.e. why not just always be 1 and rely on normal
> > increment/decrement?)
> >
> > I know some refcounts are odd in the kernel, but I don't see where the
> > block layer is caring about 2 as a refcount anywhere, what am I missing?
>
> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.
>
> Also, my session from Recipes has a little bit of discussion regarding
> this refcount and it's use [2].
>
> Best regards,
> Andreas
>
>
> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685

So it sounds like there is one refcount from the C side, and some
number of references from the Rust side. The function checks whether
there's only one Rust reference left, and if so, takes ownership of
the value, correct?

In that case, the CAS should have an acquire ordering to synchronize
with dropping the refcount 3->2 on another thread. Otherwise, you
might have a data race with the operations that happened just before
the 3->2 refcount drop.

Alice

On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Hi Greg,
>
> "Greg KH" <gregkh@linuxfoundation.org> writes:
>
> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> >> There is an operation needed by `block::mq`, atomically decreasing
> >> refcount from 2 to 0, which is not available through refcount.h, so
> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> >> directly.
> >
> > That's scary, and of course feels wrong on many levels, but:
> >
> >
> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> >>      /// C `struct request`. If the operation fails, `this` is returned in the
> >>      /// `Err` variant.
> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> >> -        // We can race with `TagSet::tag_to_rq`
> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> >> -            2,
> >> -            0,
> >> -            Ordering::Relaxed,
> >> -            Ordering::Relaxed,
> >> -        ) {
> >> +        // To hand back the ownership, we need the current refcount to be 2.
> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> >> +        // atomics directly.
> >> +        if this
> >> +            .wrapper_ref()
> >> +            .refcount()
> >> +            .as_atomic()
> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> >> +            .is_err()
> >
> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > value (i.e. why not just always be 1 and rely on normal
> > increment/decrement?)
> >
> > I know some refcounts are odd in the kernel, but I don't see where the
> > block layer is caring about 2 as a refcount anywhere, what am I missing?
>
> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.
>
> Also, my session from Recipes has a little bit of discussion regarding
> this refcount and it's use [2].
>
> Best regards,
> Andreas
>
>
> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Gary Guo]

On Sat, 5 Oct 2024 13:59:44 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >
> > Hi Greg,
> >
> > "Greg KH" <gregkh@linuxfoundation.org> writes:
> >  
> > > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:  
> > >> There is an operation needed by `block::mq`, atomically decreasing
> > >> refcount from 2 to 0, which is not available through refcount.h, so
> > >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> > >> directly.  
> > >
> > > That's scary, and of course feels wrong on many levels, but:
> > >
> > >  
> > >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> > >>      /// C `struct request`. If the operation fails, `this` is returned in the
> > >>      /// `Err` variant.
> > >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> > >> -        // We can race with `TagSet::tag_to_rq`
> > >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> > >> -            2,
> > >> -            0,
> > >> -            Ordering::Relaxed,
> > >> -            Ordering::Relaxed,
> > >> -        ) {
> > >> +        // To hand back the ownership, we need the current refcount to be 2.
> > >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> > >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> > >> +        // atomics directly.
> > >> +        if this
> > >> +            .wrapper_ref()
> > >> +            .refcount()
> > >> +            .as_atomic()
> > >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> > >> +            .is_err()  
> > >
> > > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > > value (i.e. why not just always be 1 and rely on normal
> > > increment/decrement?)
> > >
> > > I know some refcounts are odd in the kernel, but I don't see where the
> > > block layer is caring about 2 as a refcount anywhere, what am I missing?  
> >
> > It is in the documentation, rendered version available here [1]. Let me
> > know if it is still unclear, then I guess we need to update the docs.
> >
> > Also, my session from Recipes has a little bit of discussion regarding
> > this refcount and it's use [2].
> >
> > Best regards,
> > Andreas
> >
> >
> > [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
> > [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685  
> 
> So it sounds like there is one refcount from the C side, and some
> number of references from the Rust side. The function checks whether
> there's only one Rust reference left, and if so, takes ownership of
> the value, correct?
> 
> In that case, the CAS should have an acquire ordering to synchronize
> with dropping the refcount 3->2 on another thread. Otherwise, you
> might have a data race with the operations that happened just before
> the 3->2 refcount drop.
> 
> Alice

The code as is is fine since there's no data protected in
`RequestDataWrapper` yet (in fact it's not even generic yet). I know
Andreas does want to introduce driver-specific data into that, so in
the long term the acquire would be necessary.

Andreas, please let me know if you want me to make the change now, or
you'd rather change the ordering when you introduce data to
`RequestDataWrapper`.

Best,
Gary

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> On Sat, 5 Oct 2024 13:59:44 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >
>> > Hi Greg,
>> >
>> > "Greg KH" <gregkh@linuxfoundation.org> writes:
>> >
>> > > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>> > >> There is an operation needed by `block::mq`, atomically decreasing
>> > >> refcount from 2 to 0, which is not available through refcount.h, so
>> > >> I exposed `Refcount::as_atomic` which allows accessing the refcount
>> > >> directly.
>> > >
>> > > That's scary, and of course feels wrong on many levels, but:
>> > >
>> > >
>> > >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>> > >>      /// C `struct request`. If the operation fails, `this` is returned in the
>> > >>      /// `Err` variant.
>> > >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>> > >> -        // We can race with `TagSet::tag_to_rq`
>> > >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>> > >> -            2,
>> > >> -            0,
>> > >> -            Ordering::Relaxed,
>> > >> -            Ordering::Relaxed,
>> > >> -        ) {
>> > >> +        // To hand back the ownership, we need the current refcount to be 2.
>> > >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>> > >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>> > >> +        // atomics directly.
>> > >> +        if this
>> > >> +            .wrapper_ref()
>> > >> +            .refcount()
>> > >> +            .as_atomic()
>> > >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>> > >> +            .is_err()
>> > >
>> > > Why not just call rust_helper_refcount_set()?  Or is the issue that you
>> > > think you might not be 2 here?  And if you HAVE to be 2, why that magic
>> > > value (i.e. why not just always be 1 and rely on normal
>> > > increment/decrement?)
>> > >
>> > > I know some refcounts are odd in the kernel, but I don't see where the
>> > > block layer is caring about 2 as a refcount anywhere, what am I missing?
>> >
>> > It is in the documentation, rendered version available here [1]. Let me
>> > know if it is still unclear, then I guess we need to update the docs.
>> >
>> > Also, my session from Recipes has a little bit of discussion regarding
>> > this refcount and it's use [2].
>> >
>> > Best regards,
>> > Andreas
>> >
>> >
>> > [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>> > [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>>
>> So it sounds like there is one refcount from the C side, and some
>> number of references from the Rust side. The function checks whether
>> there's only one Rust reference left, and if so, takes ownership of
>> the value, correct?
>>
>> In that case, the CAS should have an acquire ordering to synchronize
>> with dropping the refcount 3->2 on another thread. Otherwise, you
>> might have a data race with the operations that happened just before
>> the 3->2 refcount drop.
>>
>> Alice
>
> The code as is is fine since there's no data protected in
> `RequestDataWrapper` yet (in fact it's not even generic yet). I know
> Andreas does want to introduce driver-specific data into that, so in
> the long term the acquire would be necessary.
>
> Andreas, please let me know if you want me to make the change now, or
> you'd rather change the ordering when you introduce data to
> `RequestDataWrapper`.

I guess we will have said data dependencies when we are going to run
drop for fields in the private data area. Thanks for pointing that out.
I will update the ordering when I submit that patch.

As I mentioned before, I would rather we do not apply this patch before
we get a way to inline helpers.


BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Gary Guo" <gary@garyguo.net> writes:
>
>> On Sat, 5 Oct 2024 13:59:44 +0200
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>>> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>> >
>>> > Hi Greg,
>>> >
>>> > "Greg KH" <gregkh@linuxfoundation.org> writes:
>>> >
>>> > > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>>> > >> There is an operation needed by `block::mq`, atomically decreasing
>>> > >> refcount from 2 to 0, which is not available through refcount.h, so
>>> > >> I exposed `Refcount::as_atomic` which allows accessing the refcount
>>> > >> directly.
>>> > >
>>> > > That's scary, and of course feels wrong on many levels, but:
>>> > >
>>> > >
>>> > >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>>> > >>      /// C `struct request`. If the operation fails, `this` is returned in the
>>> > >>      /// `Err` variant.
>>> > >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>>> > >> -        // We can race with `TagSet::tag_to_rq`
>>> > >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>>> > >> -            2,
>>> > >> -            0,
>>> > >> -            Ordering::Relaxed,
>>> > >> -            Ordering::Relaxed,
>>> > >> -        ) {
>>> > >> +        // To hand back the ownership, we need the current refcount to be 2.
>>> > >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>>> > >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>>> > >> +        // atomics directly.
>>> > >> +        if this
>>> > >> +            .wrapper_ref()
>>> > >> +            .refcount()
>>> > >> +            .as_atomic()
>>> > >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>>> > >> +            .is_err()
>>> > >
>>> > > Why not just call rust_helper_refcount_set()?  Or is the issue that you
>>> > > think you might not be 2 here?  And if you HAVE to be 2, why that magic
>>> > > value (i.e. why not just always be 1 and rely on normal
>>> > > increment/decrement?)
>>> > >
>>> > > I know some refcounts are odd in the kernel, but I don't see where the
>>> > > block layer is caring about 2 as a refcount anywhere, what am I missing?
>>> >
>>> > It is in the documentation, rendered version available here [1]. Let me
>>> > know if it is still unclear, then I guess we need to update the docs.
>>> >
>>> > Also, my session from Recipes has a little bit of discussion regarding
>>> > this refcount and it's use [2].
>>> >
>>> > Best regards,
>>> > Andreas
>>> >
>>> >
>>> > [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>>> > [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>>>
>>> So it sounds like there is one refcount from the C side, and some
>>> number of references from the Rust side. The function checks whether
>>> there's only one Rust reference left, and if so, takes ownership of
>>> the value, correct?
>>>
>>> In that case, the CAS should have an acquire ordering to synchronize
>>> with dropping the refcount 3->2 on another thread. Otherwise, you
>>> might have a data race with the operations that happened just before
>>> the 3->2 refcount drop.
>>>
>>> Alice
>>
>> The code as is is fine since there's no data protected in
>> `RequestDataWrapper` yet (in fact it's not even generic yet). I know
>> Andreas does want to introduce driver-specific data into that, so in
>> the long term the acquire would be necessary.
>>
>> Andreas, please let me know if you want me to make the change now, or
>> you'd rather change the ordering when you introduce data to
>> `RequestDataWrapper`.
>
> I guess we will have said data dependencies when we are going to run
> drop for fields in the private data area. Thanks for pointing that out.
> I will update the ordering when I submit that patch.
>
> As I mentioned before, I would rather we do not apply this patch before
> we get a way to inline helpers.

As discussed offline, the code that suffers the performance regression
is downstream, and since this change seems to be important, I can apply
the helper LTO patch downstream as well.

Since the plan for the downstream code _is_ to move upstream, I really
hope to see the helper LTO patch upstream, so we don't get a performance
regression because of these refcounts.

If we cannot figure out a way to get the LTO patches (or an alternative
solution) upstream, we can always revert back to a more performant
solution in block.


BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>>> On Sat, 5 Oct 2024 13:59:44 +0200
>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>
>>>> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>> >
>>>> > Hi Greg,
>>>> >
>>>> > "Greg KH" <gregkh@linuxfoundation.org> writes:
>>>> >
>>>> > > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>>>> > >> There is an operation needed by `block::mq`, atomically decreasing
>>>> > >> refcount from 2 to 0, which is not available through refcount.h, so
>>>> > >> I exposed `Refcount::as_atomic` which allows accessing the refcount
>>>> > >> directly.
>>>> > >
>>>> > > That's scary, and of course feels wrong on many levels, but:
>>>> > >
>>>> > >
>>>> > >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>>>> > >>      /// C `struct request`. If the operation fails, `this` is returned in the
>>>> > >>      /// `Err` variant.
>>>> > >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>>>> > >> -        // We can race with `TagSet::tag_to_rq`
>>>> > >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>>>> > >> -            2,
>>>> > >> -            0,
>>>> > >> -            Ordering::Relaxed,
>>>> > >> -            Ordering::Relaxed,
>>>> > >> -        ) {
>>>> > >> +        // To hand back the ownership, we need the current refcount to be 2.
>>>> > >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>>>> > >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>>>> > >> +        // atomics directly.
>>>> > >> +        if this
>>>> > >> +            .wrapper_ref()
>>>> > >> +            .refcount()
>>>> > >> +            .as_atomic()
>>>> > >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>>>> > >> +            .is_err()
>>>> > >
>>>> > > Why not just call rust_helper_refcount_set()?  Or is the issue that you
>>>> > > think you might not be 2 here?  And if you HAVE to be 2, why that magic
>>>> > > value (i.e. why not just always be 1 and rely on normal
>>>> > > increment/decrement?)
>>>> > >
>>>> > > I know some refcounts are odd in the kernel, but I don't see where the
>>>> > > block layer is caring about 2 as a refcount anywhere, what am I missing?
>>>> >
>>>> > It is in the documentation, rendered version available here [1]. Let me
>>>> > know if it is still unclear, then I guess we need to update the docs.
>>>> >
>>>> > Also, my session from Recipes has a little bit of discussion regarding
>>>> > this refcount and it's use [2].
>>>> >
>>>> > Best regards,
>>>> > Andreas
>>>> >
>>>> >
>>>> > [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>>>> > [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>>>>
>>>> So it sounds like there is one refcount from the C side, and some
>>>> number of references from the Rust side. The function checks whether
>>>> there's only one Rust reference left, and if so, takes ownership of
>>>> the value, correct?
>>>>
>>>> In that case, the CAS should have an acquire ordering to synchronize
>>>> with dropping the refcount 3->2 on another thread. Otherwise, you
>>>> might have a data race with the operations that happened just before
>>>> the 3->2 refcount drop.
>>>>
>>>> Alice
>>>
>>> The code as is is fine since there's no data protected in
>>> `RequestDataWrapper` yet (in fact it's not even generic yet). I know
>>> Andreas does want to introduce driver-specific data into that, so in
>>> the long term the acquire would be necessary.
>>>
>>> Andreas, please let me know if you want me to make the change now, or
>>> you'd rather change the ordering when you introduce data to
>>> `RequestDataWrapper`.
>>
>> I guess we will have said data dependencies when we are going to run
>> drop for fields in the private data area. Thanks for pointing that out.
>> I will update the ordering when I submit that patch.
>>
>> As I mentioned before, I would rather we do not apply this patch before
>> we get a way to inline helpers.
>
> As discussed offline, the code that suffers the performance regression
> is downstream, and since this change seems to be important, I can apply
> the helper LTO patch downstream as well.
>
> Since the plan for the downstream code _is_ to move upstream, I really
> hope to see the helper LTO patch upstream, so we don't get a performance
> regression because of these refcounts.
>
> If we cannot figure out a way to get the LTO patches (or an alternative
> solution) upstream, we can always revert back to a more performant
> solution in block.

I forgot to report the result of the benchmarks. Over the usual
benchmark workload that I run for `rnull` I see an average 0.8 percent
performance penalty with this patch. For some configurations
I see 95% CI N=40 [-18%;-5%]. So it is not insignificant.


BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Benno Lossin]

On 10.10.24 11:06, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>>> "Gary Guo" <gary@garyguo.net> writes:
>>>
>>>> On Sat, 5 Oct 2024 13:59:44 +0200
>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>
>>>>> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>>
>>>>>> Hi Greg,
>>>>>>
>>>>>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>>>>>
>>>>>>> On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>>>>>>>> There is an operation needed by `block::mq`, atomically decreasing
>>>>>>>> refcount from 2 to 0, which is not available through refcount.h, so
>>>>>>>> I exposed `Refcount::as_atomic` which allows accessing the refcount
>>>>>>>> directly.
>>>>>>>
>>>>>>> That's scary, and of course feels wrong on many levels, but:
>>>>>>>
>>>>>>>
>>>>>>>> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>>>>>>>>      /// C `struct request`. If the operation fails, `this` is returned in the
>>>>>>>>      /// `Err` variant.
>>>>>>>>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>>>>>>>> -        // We can race with `TagSet::tag_to_rq`
>>>>>>>> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>>>>>>>> -            2,
>>>>>>>> -            0,
>>>>>>>> -            Ordering::Relaxed,
>>>>>>>> -            Ordering::Relaxed,
>>>>>>>> -        ) {
>>>>>>>> +        // To hand back the ownership, we need the current refcount to be 2.
>>>>>>>> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>>>>>>>> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>>>>>>>> +        // atomics directly.
>>>>>>>> +        if this
>>>>>>>> +            .wrapper_ref()
>>>>>>>> +            .refcount()
>>>>>>>> +            .as_atomic()
>>>>>>>> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>>>>>>>> +            .is_err()
>>>>>>>
>>>>>>> Why not just call rust_helper_refcount_set()?  Or is the issue that you
>>>>>>> think you might not be 2 here?  And if you HAVE to be 2, why that magic
>>>>>>> value (i.e. why not just always be 1 and rely on normal
>>>>>>> increment/decrement?)
>>>>>>>
>>>>>>> I know some refcounts are odd in the kernel, but I don't see where the
>>>>>>> block layer is caring about 2 as a refcount anywhere, what am I missing?
>>>>>>
>>>>>> It is in the documentation, rendered version available here [1]. Let me
>>>>>> know if it is still unclear, then I guess we need to update the docs.
>>>>>>
>>>>>> Also, my session from Recipes has a little bit of discussion regarding
>>>>>> this refcount and it's use [2].
>>>>>>
>>>>>> Best regards,
>>>>>> Andreas
>>>>>>
>>>>>>
>>>>>> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>>>>>> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>>>>>
>>>>> So it sounds like there is one refcount from the C side, and some
>>>>> number of references from the Rust side. The function checks whether
>>>>> there's only one Rust reference left, and if so, takes ownership of
>>>>> the value, correct?
>>>>>
>>>>> In that case, the CAS should have an acquire ordering to synchronize
>>>>> with dropping the refcount 3->2 on another thread. Otherwise, you
>>>>> might have a data race with the operations that happened just before
>>>>> the 3->2 refcount drop.
>>>>>
>>>>> Alice
>>>>
>>>> The code as is is fine since there's no data protected in
>>>> `RequestDataWrapper` yet (in fact it's not even generic yet). I know
>>>> Andreas does want to introduce driver-specific data into that, so in
>>>> the long term the acquire would be necessary.
>>>>
>>>> Andreas, please let me know if you want me to make the change now, or
>>>> you'd rather change the ordering when you introduce data to
>>>> `RequestDataWrapper`.
>>>
>>> I guess we will have said data dependencies when we are going to run
>>> drop for fields in the private data area. Thanks for pointing that out.
>>> I will update the ordering when I submit that patch.
>>>
>>> As I mentioned before, I would rather we do not apply this patch before
>>> we get a way to inline helpers.
>>
>> As discussed offline, the code that suffers the performance regression
>> is downstream, and since this change seems to be important, I can apply
>> the helper LTO patch downstream as well.
>>
>> Since the plan for the downstream code _is_ to move upstream, I really
>> hope to see the helper LTO patch upstream, so we don't get a performance
>> regression because of these refcounts.
>>
>> If we cannot figure out a way to get the LTO patches (or an alternative
>> solution) upstream, we can always revert back to a more performant
>> solution in block.
> 
> I forgot to report the result of the benchmarks. Over the usual
> benchmark workload that I run for `rnull` I see an average 0.8 percent
> performance penalty with this patch. For some configurations
> I see 95% CI N=40 [-18%;-5%]. So it is not insignificant.

Was the benchmark run together with the LTO helper patches?

---
Cheers,
Benno

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 10.10.24 11:06, Andreas Hindborg wrote:
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>
>>>> "Gary Guo" <gary@garyguo.net> writes:
>>>>
>>>>> On Sat, 5 Oct 2024 13:59:44 +0200
>>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>
>>>>>> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>>>
>>>>>>> Hi Greg,
>>>>>>>
>>>>>>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>>>>>>
>>>>>>>> On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>>>>>>>>> There is an operation needed by `block::mq`, atomically decreasing
>>>>>>>>> refcount from 2 to 0, which is not available through refcount.h, so
>>>>>>>>> I exposed `Refcount::as_atomic` which allows accessing the refcount
>>>>>>>>> directly.
>>>>>>>>
>>>>>>>> That's scary, and of course feels wrong on many levels, but:
>>>>>>>>
>>>>>>>>
>>>>>>>>> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>>>>>>>>>      /// C `struct request`. If the operation fails, `this` is returned in the
>>>>>>>>>      /// `Err` variant.
>>>>>>>>>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>>>>>>>>> -        // We can race with `TagSet::tag_to_rq`
>>>>>>>>> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>>>>>>>>> -            2,
>>>>>>>>> -            0,
>>>>>>>>> -            Ordering::Relaxed,
>>>>>>>>> -            Ordering::Relaxed,
>>>>>>>>> -        ) {
>>>>>>>>> +        // To hand back the ownership, we need the current refcount to be 2.
>>>>>>>>> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>>>>>>>>> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>>>>>>>>> +        // atomics directly.
>>>>>>>>> +        if this
>>>>>>>>> +            .wrapper_ref()
>>>>>>>>> +            .refcount()
>>>>>>>>> +            .as_atomic()
>>>>>>>>> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>>>>>>>>> +            .is_err()
>>>>>>>>
>>>>>>>> Why not just call rust_helper_refcount_set()?  Or is the issue that you
>>>>>>>> think you might not be 2 here?  And if you HAVE to be 2, why that magic
>>>>>>>> value (i.e. why not just always be 1 and rely on normal
>>>>>>>> increment/decrement?)
>>>>>>>>
>>>>>>>> I know some refcounts are odd in the kernel, but I don't see where the
>>>>>>>> block layer is caring about 2 as a refcount anywhere, what am I missing?
>>>>>>>
>>>>>>> It is in the documentation, rendered version available here [1]. Let me
>>>>>>> know if it is still unclear, then I guess we need to update the docs.
>>>>>>>
>>>>>>> Also, my session from Recipes has a little bit of discussion regarding
>>>>>>> this refcount and it's use [2].
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Andreas
>>>>>>>
>>>>>>>
>>>>>>> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>>>>>>> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>>>>>>
>>>>>> So it sounds like there is one refcount from the C side, and some
>>>>>> number of references from the Rust side. The function checks whether
>>>>>> there's only one Rust reference left, and if so, takes ownership of
>>>>>> the value, correct?
>>>>>>
>>>>>> In that case, the CAS should have an acquire ordering to synchronize
>>>>>> with dropping the refcount 3->2 on another thread. Otherwise, you
>>>>>> might have a data race with the operations that happened just before
>>>>>> the 3->2 refcount drop.
>>>>>>
>>>>>> Alice
>>>>>
>>>>> The code as is is fine since there's no data protected in
>>>>> `RequestDataWrapper` yet (in fact it's not even generic yet). I know
>>>>> Andreas does want to introduce driver-specific data into that, so in
>>>>> the long term the acquire would be necessary.
>>>>>
>>>>> Andreas, please let me know if you want me to make the change now, or
>>>>> you'd rather change the ordering when you introduce data to
>>>>> `RequestDataWrapper`.
>>>>
>>>> I guess we will have said data dependencies when we are going to run
>>>> drop for fields in the private data area. Thanks for pointing that out.
>>>> I will update the ordering when I submit that patch.
>>>>
>>>> As I mentioned before, I would rather we do not apply this patch before
>>>> we get a way to inline helpers.
>>>
>>> As discussed offline, the code that suffers the performance regression
>>> is downstream, and since this change seems to be important, I can apply
>>> the helper LTO patch downstream as well.
>>>
>>> Since the plan for the downstream code _is_ to move upstream, I really
>>> hope to see the helper LTO patch upstream, so we don't get a performance
>>> regression because of these refcounts.
>>>
>>> If we cannot figure out a way to get the LTO patches (or an alternative
>>> solution) upstream, we can always revert back to a more performant
>>> solution in block.
>>
>> I forgot to report the result of the benchmarks. Over the usual
>> benchmark workload that I run for `rnull` I see an average 0.8 percent
>> performance penalty with this patch. For some configurations
>> I see 95% CI N=40 [-18%;-5%]. So it is not insignificant.
>
> Was the benchmark run together with the LTO helper patches?

No, that the effect of applying only this patch set alone. I did apply
the helper LTO patches downstream a few times, but I don't carry them in
my default tree. But I guess I can start doing that now.

Best regards,
Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Hi Greg,
>>
>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>
>> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>> >> There is an operation needed by `block::mq`, atomically decreasing
>> >> refcount from 2 to 0, which is not available through refcount.h, so
>> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
>> >> directly.
>> >
>> > That's scary, and of course feels wrong on many levels, but:
>> >
>> >
>> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>> >>      /// C `struct request`. If the operation fails, `this` is returned in the
>> >>      /// `Err` variant.
>> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>> >> -        // We can race with `TagSet::tag_to_rq`
>> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>> >> -            2,
>> >> -            0,
>> >> -            Ordering::Relaxed,
>> >> -            Ordering::Relaxed,
>> >> -        ) {
>> >> +        // To hand back the ownership, we need the current refcount to be 2.
>> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>> >> +        // atomics directly.
>> >> +        if this
>> >> +            .wrapper_ref()
>> >> +            .refcount()
>> >> +            .as_atomic()
>> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>> >> +            .is_err()
>> >
>> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
>> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
>> > value (i.e. why not just always be 1 and rely on normal
>> > increment/decrement?)
>> >
>> > I know some refcounts are odd in the kernel, but I don't see where the
>> > block layer is caring about 2 as a refcount anywhere, what am I missing?
>>
>> It is in the documentation, rendered version available here [1]. Let me
>> know if it is still unclear, then I guess we need to update the docs.
>>
>> Also, my session from Recipes has a little bit of discussion regarding
>> this refcount and it's use [2].
>>
>> Best regards,
>> Andreas
>>
>>
>> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>
> So it sounds like there is one refcount from the C side, and some
> number of references from the Rust side.

C side uses a different refcount field. That refcount never read by
Rust, but it is guaranteed to be greater or equal to one while the
driver owns the request.

Rust uses a different refcount field to keep track of how many Rust
references there is to a request. There is an implicit count while the
driver owns the request. This count is not materialized as an `ARef`
instance.

> The function checks whether there's only one Rust reference left, and
> if so, takes ownership of the value, correct?

It checks if the `ARef` passed to the function is the last one in
existence. If it is, it takes ownership of the `Request` object.

> In that case, the CAS should have an acquire ordering to synchronize
> with dropping the refcount 3->2 on another thread. Otherwise, you
> might have a data race with the operations that happened just before
> the 3->2 refcount drop.

I am not sure. I don't think that the thread that does the CAS 2 -> 0
has any data dependencies to any thread that does the atomic decrement 3
-> 2. Any data dependencies between operations on the underlying C
`struct request` would be synchronized by operations on the `ref` field
of `struct request`, which is entirely managed block layer C code and
the C functions called by the Rust abstractions.


BR Andreas

Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

[Andreas Hindborg]

"Gary Guo" <gary@garyguo.net> writes:

> Currently there's a custom reference counting in `block::mq`, which uses
> `AtomicU64` Rust atomics, and this type doesn't exist on some 32-bit
> architectures. We cannot just change it to use 32-bit atomics, because
> doing so will make it vulnerable to refcount overflow. So switch it to
> use the kernel refcount `kernel::sync::Refcount` instead.
>
> There is an operation needed by `block::mq`, atomically decreasing
> refcount from 2 to 0, which is not available through refcount.h, so
> I exposed `Refcount::as_atomic` which allows accessing the refcount
> directly.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Gary Guo <gary@garyguo.net>


Acked-by: Andreas Hindborg <a.hindborg@kernel.org>