[RFC 0/5] rust: sync: Arc: Implement Debug and Display

[RFC 0/5] rust: sync: Arc: Implement Debug and Display

[Boqun Feng]

I found that our Arc doesn't implement `Debug` or `Display` when I tried
to play with them, therefore add these implementation.

With these changes, I could get the following print with the sample code
in patch #5:

	[..] rust_print: 1
	[..] rust_print: UniqueArc { inner: Arc { refcount: 1, data: "hello, world" } }
	[..] rust_print: [samples/rust/rust_print.rs:34] c = Arc {
	[..]     refcount: 2,
	[..]     data: "hello, world",
	[..] }
	[..] rust_print: Arc {
	[..]     refcount: 0x1,
	[..]     data: "hello, world",
	[..] }

Note that I make the `Debug` implementation of `Arc` also print the
current reference count, which I think may be useful: myself sometimes
wonder "how many references exist at this point" during my own
development. But I'm open to suggestions and changes.

Wedson, I know that you are considering to get rid of `ArcBorrow`, so
the patch #3 may have some conflicts with what you may be working on.
I'm happy to wait and rebase since this series is not something urgent
;-)

Suggestions and comments are welcome!

Regards,
Boqun

[RFC 1/5] rust: sync: impl Display for {Unique,}Arc

[Boqun Feng]

This allows printing the inner data of `Arc` and its friends if the
inner data implements `Display`. It's useful for logging and debugging
purpose.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/arc.rs | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 519a6ec43644..fc680a4a795c 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -22,6 +22,7 @@ use crate::{
 };
 use alloc::boxed::Box;
 use core::{
+    fmt,
     marker::{PhantomData, Unsize},
     mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
@@ -522,3 +523,15 @@ impl<T: ?Sized> DerefMut for UniqueArc<T> {
         unsafe { &mut self.inner.ptr.as_mut().data }
     }
 }
+
+impl<T: fmt::Display + ?Sized> fmt::Display for UniqueArc<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt::Display::fmt(self.deref(), f)
+    }
+}
+
+impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt::Display::fmt(self.deref(), f)
+    }
+}
-- 
2.39.1

[RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

This allows reading the current count of a refcount in an `ArcInner`.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/helpers.c          | 6 ++++++
 rust/kernel/sync/arc.rs | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/rust/helpers.c b/rust/helpers.c
index 09a4d93f9d62..afc5f1a39fef 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
 }
 EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
 
+unsigned int rust_helper_refcount_read(refcount_t *r)
+{
+	return refcount_read(r);
+}
+EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index fc680a4a795c..fbfceaa3096e 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
     data: T,
 }
 
+impl<T: ?Sized> ArcInner<T> {
+    /// Returns the current reference count of [`ArcInner`].
+    fn count(&self) -> u32 {
+        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
+        // normal atomic read (i.e. no data race) only requiring on the address is valid.
+        unsafe { bindings::refcount_read(self.refcount.get()) }
+    }
+}
+
 // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
 impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
 
-- 
2.39.1

[RFC 3/5] rust: sync: Arc: Introduces Arc::get_inner() helper

[Boqun Feng]

Getting a `&ArcInner<T>` should always be safe from a `Arc<T>`,
therefore add this helper function to avoid duplicate unsafe
`ptr.as_ref()`.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/arc.rs | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index fbfceaa3096e..4d8de20c996f 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -201,6 +201,13 @@ impl<T: ?Sized> Arc<T> {
         // reference can be created.
         unsafe { ArcBorrow::new(self.ptr) }
     }
+
+    /// Returns a reference to the internal [`ArcInner`].
+    fn get_inner(&self) -> &ArcInner<T> {
+        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
+        // safe to dereference it.
+        unsafe { self.ptr.as_ref() }
+    }
 }
 
 impl<T: 'static> ForeignOwnable for Arc<T> {
@@ -233,9 +240,7 @@ impl<T: ?Sized> Deref for Arc<T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {
-        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
-        // safe to dereference it.
-        unsafe { &self.ptr.as_ref().data }
+        &self.get_inner().data
     }
 }
 
@@ -244,7 +249,7 @@ impl<T: ?Sized> Clone for Arc<T> {
         // INVARIANT: C `refcount_inc` 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 { bindings::refcount_inc(self.get_inner().refcount.get()) };
 
         // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
         unsafe { Self::from_inner(self.ptr) }
@@ -253,11 +258,7 @@ impl<T: ?Sized> Clone for Arc<T> {
 
 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();
+        let refcount = self.get_inner().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.
-- 
2.39.1

[RFC 4/5] rust: sync: impl Debug for {Unique,}Arc

[Boqun Feng]

This allows printing macros to print the data and the refcount nubmer
of these struct for debugging purposes.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/sync/arc.rs | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 4d8de20c996f..f143d8305c36 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -474,6 +474,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
 ///
 /// # test().unwrap();
 /// ```
+#[derive(Debug)]
 pub struct UniqueArc<T: ?Sized> {
     inner: Arc<T>,
 }
@@ -545,3 +546,15 @@ impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
         fmt::Display::fmt(self.deref(), f)
     }
 }
+
+impl<T: fmt::Debug + ?Sized> fmt::Debug for Arc<T> {
+    /// Formats debug output for [`Arc`].
+    ///
+    /// Refcount is also printed for debugging purpose.
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Arc")
+            .field("refcount", &self.get_inner().count())
+            .field("data", &self.deref())
+            .finish()
+    }
+}
-- 
2.39.1

[RFC 5/5] sample: rust: print: Add sampe code for Arc printing

[Boqun Feng]

This both demonstrates the usage of different print format in Rust and
serves as a selftest for the `Display` and `Debug` implementation of
`Arc` and its friends.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 samples/rust/rust_print.rs | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
index 8b39d9cef6d1..165a8d7b1c07 100644
--- a/samples/rust/rust_print.rs
+++ b/samples/rust/rust_print.rs
@@ -15,6 +15,30 @@ module! {
 
 struct RustPrint;
 
+fn arc_print() -> Result {
+    use kernel::sync::*;
+
+    let a = Arc::try_new(1)?;
+    let b = UniqueArc::try_new("hello, world")?;
+
+    // Prints the value of data in `a`.
+    pr_info!("{}", a);
+
+    // Uses ":?" to print debug fmt of `b`.
+    pr_info!("{:?}", b);
+
+    let a: Arc<&str> = b.into();
+    let c = a.clone();
+
+    // Uses `dbg` to print, will move `c`.
+    dbg!(c);
+
+    // Prints debug fmt with pretty-print "#" and number-in-hex "x".
+    pr_info!("{:#x?}", a);
+
+    Ok(())
+}
+
 impl kernel::Module for RustPrint {
     fn init(_module: &'static ThisModule) -> Result<Self> {
         pr_info!("Rust printing macros sample (init)\n");
@@ -43,6 +67,8 @@ impl kernel::Module for RustPrint {
         pr_cont!(" is {}", "continued");
         pr_cont!(" with {}\n", "args");
 
+        arc_print()?;
+
         Ok(RustPrint)
     }
 }
-- 
2.39.1

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Peter Zijlstra]

On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> This allows reading the current count of a refcount in an `ArcInner`.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/helpers.c          | 6 ++++++
>  rust/kernel/sync/arc.rs | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 09a4d93f9d62..afc5f1a39fef 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
>  
> +unsigned int rust_helper_refcount_read(refcount_t *r)
> +{
> +	return refcount_read(r);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fc680a4a795c..fbfceaa3096e 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
>      data: T,
>  }
>  
> +impl<T: ?Sized> ArcInner<T> {
> +    /// Returns the current reference count of [`ArcInner`].
> +    fn count(&self) -> u32 {
> +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> +        unsafe { bindings::refcount_read(self.refcount.get()) }
> +    }
> +}

This is completely unsafe vs concurrency. In order to enable correct
tracing of refcount manipulations we have the __refcount_*(.oldp) API.

Admittedly, I have no idea how to sanely wire that up in this thing, but
it seems odd to me to have this 'safe' language display fundamentally
unsafe data like this.

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

On Thu, Feb 02, 2023 at 10:14:06AM +0100, Peter Zijlstra wrote:
> On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > This allows reading the current count of a refcount in an `ArcInner`.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/helpers.c          | 6 ++++++
> >  rust/kernel/sync/arc.rs | 9 +++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 09a4d93f9d62..afc5f1a39fef 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> >  
> > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > +{
> > +	return refcount_read(r);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > +
> >  /*
> >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index fc680a4a795c..fbfceaa3096e 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> >      data: T,
> >  }
> >  
> > +impl<T: ?Sized> ArcInner<T> {
> > +    /// Returns the current reference count of [`ArcInner`].
> > +    fn count(&self) -> u32 {
> > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > +    }
> > +}
> 
> This is completely unsafe vs concurrency. In order to enable correct
> tracing of refcount manipulations we have the __refcount_*(.oldp) API.
> 

Interesting.

> Admittedly, I have no idea how to sanely wire that up in this thing, but
> it seems odd to me to have this 'safe' language display fundamentally
> unsafe data like this.

Right now the only use of this "data" is for debugging display, the
"count" function is private, so no one outside Arc can mis-use it, in
that sense, I think it's still safe?

Regards,
Boqun

Re: [RFC 1/5] rust: sync: impl Display for {Unique,}Arc

[Gary Guo]

On Wed,  1 Feb 2023 15:22:40 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> This allows printing the inner data of `Arc` and its friends if the
> inner data implements `Display`. It's useful for logging and debugging
> purpose.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/sync/arc.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 519a6ec43644..fc680a4a795c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -22,6 +22,7 @@ use crate::{
>  };
>  use alloc::boxed::Box;
>  use core::{
> +    fmt,
>      marker::{PhantomData, Unsize},
>      mem::{ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
> @@ -522,3 +523,15 @@ impl<T: ?Sized> DerefMut for UniqueArc<T> {
>          unsafe { &mut self.inner.ptr.as_mut().data }
>      }
>  }
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for UniqueArc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Gary Guo]

On Thu, 2 Feb 2023 10:14:06 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > This allows reading the current count of a refcount in an `ArcInner`.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/helpers.c          | 6 ++++++
> >  rust/kernel/sync/arc.rs | 9 +++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/rust/helpers.c b/rust/helpers.c
> > index 09a4d93f9d62..afc5f1a39fef 100644
> > --- a/rust/helpers.c
> > +++ b/rust/helpers.c
> > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> >  }
> >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> >  
> > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > +{
> > +	return refcount_read(r);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > +
> >  /*
> >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index fc680a4a795c..fbfceaa3096e 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> >      data: T,
> >  }
> >  
> > +impl<T: ?Sized> ArcInner<T> {
> > +    /// Returns the current reference count of [`ArcInner`].
> > +    fn count(&self) -> u32 {
> > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > +    }
> > +}  
> 
> This is completely unsafe vs concurrency. In order to enable correct
> tracing of refcount manipulations we have the __refcount_*(.oldp) API.

Retrieving the reference count is safe. It's just that in many
scenarios it's very hard to use the retrieved reference count
correctly, because it might be concurrently changed.

But there are correct ways to use a refcount, e.g. if you own
`Arc` and `.count()` returns 1, then you know that you are the
exclusive owner of the `Arc` and nobody else is going to touch it.
In this case we could get a `&mut` of the data (Rust standard library's
`Arc` provides such an API, although we don't have it yet).

The Rust standard library's `Arc` also expose a `strong_count`
function, with this doc:

```
Gets the number of strong (Arc) pointers to this allocation.

# Safety
This method by itself is safe, but using it correctly requires extra
care. Another thread can change the strong count at any time, including
potentially between calling this method and acting on the result.
```

Best,
Gary

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Gary Guo]

On Wed,  1 Feb 2023 15:22:41 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> This allows reading the current count of a refcount in an `ArcInner`.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/helpers.c          | 6 ++++++
>  rust/kernel/sync/arc.rs | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 09a4d93f9d62..afc5f1a39fef 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
>  
> +unsigned int rust_helper_refcount_read(refcount_t *r)
> +{
> +	return refcount_read(r);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fc680a4a795c..fbfceaa3096e 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
>      data: T,
>  }
>  
> +impl<T: ?Sized> ArcInner<T> {
> +    /// Returns the current reference count of [`ArcInner`].
> +    fn count(&self) -> u32 {
> +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> +        unsafe { bindings::refcount_read(self.refcount.get()) }
> +    }
> +}
> +
>  // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
>  impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
>  

Re: [RFC 3/5] rust: sync: Arc: Introduces Arc::get_inner() helper

[Gary Guo]

On Wed,  1 Feb 2023 15:22:42 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> Getting a `&ArcInner<T>` should always be safe from a `Arc<T>`,
> therefore add this helper function to avoid duplicate unsafe
> `ptr.as_ref()`.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/sync/arc.rs | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fbfceaa3096e..4d8de20c996f 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -201,6 +201,13 @@ impl<T: ?Sized> Arc<T> {
>          // reference can be created.
>          unsafe { ArcBorrow::new(self.ptr) }
>      }
> +
> +    /// Returns a reference to the internal [`ArcInner`].
> +    fn get_inner(&self) -> &ArcInner<T> {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
>  }
>  
>  impl<T: 'static> ForeignOwnable for Arc<T> {
> @@ -233,9 +240,7 @@ impl<T: ?Sized> Deref for Arc<T> {
>      type Target = T;
>  
>      fn deref(&self) -> &Self::Target {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> -        // safe to dereference it.
> -        unsafe { &self.ptr.as_ref().data }
> +        &self.get_inner().data
>      }
>  }
>  
> @@ -244,7 +249,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>          // INVARIANT: C `refcount_inc` 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 { bindings::refcount_inc(self.get_inner().refcount.get()) };
>  
>          // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>          unsafe { Self::from_inner(self.ptr) }
> @@ -253,11 +258,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>  
>  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();
> +        let refcount = self.get_inner().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.

Re: [RFC 4/5] rust: sync: impl Debug for {Unique,}Arc

[Gary Guo]

On Wed,  1 Feb 2023 15:22:43 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> This allows printing macros to print the data and the refcount nubmer
> of these struct for debugging purposes.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/sync/arc.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 4d8de20c996f..f143d8305c36 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -474,6 +474,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
>  ///
>  /// # test().unwrap();
>  /// ```
> +#[derive(Debug)]

I don't think this should be a `#[derive(Debug)]`. For `UniqueArc` the
refcount field in `Arc` is useless, and we should just delegate the
`Debug` impl to that of deref, just like `Display` does.

Best,
Gary

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Greg KH]

On Thu, Feb 02, 2023 at 02:21:53PM +0000, Gary Guo wrote:
> On Thu, 2 Feb 2023 10:14:06 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > > This allows reading the current count of a refcount in an `ArcInner`.
> > > 
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > ---
> > >  rust/helpers.c          | 6 ++++++
> > >  rust/kernel/sync/arc.rs | 9 +++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > index 09a4d93f9d62..afc5f1a39fef 100644
> > > --- a/rust/helpers.c
> > > +++ b/rust/helpers.c
> > > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> > >  }
> > >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> > >  
> > > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > > +{
> > > +	return refcount_read(r);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > > +
> > >  /*
> > >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> > >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > > index fc680a4a795c..fbfceaa3096e 100644
> > > --- a/rust/kernel/sync/arc.rs
> > > +++ b/rust/kernel/sync/arc.rs
> > > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> > >      data: T,
> > >  }
> > >  
> > > +impl<T: ?Sized> ArcInner<T> {
> > > +    /// Returns the current reference count of [`ArcInner`].
> > > +    fn count(&self) -> u32 {
> > > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > > +    }
> > > +}  
> > 
> > This is completely unsafe vs concurrency. In order to enable correct
> > tracing of refcount manipulations we have the __refcount_*(.oldp) API.
> 
> Retrieving the reference count is safe. It's just that in many
> scenarios it's very hard to use the retrieved reference count
> correctly, because it might be concurrently changed.

Yes, so you really should never ever ever care about the value, and that
includes printing it out as it will be wrong the instant you read it.

> But there are correct ways to use a refcount, e.g. if you own
> `Arc` and `.count()` returns 1, then you know that you are the
> exclusive owner of the `Arc` and nobody else is going to touch it.

But you should never know this, as it is not relevant.

So no, please don't allow printing out of a reference count, that will
only cause problems and allow people to think it is safe to do so.

Peter is right, please don't do this.

thanks,

greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

On Thu, Feb 02, 2023 at 04:41:47PM +0100, Greg KH wrote:
> On Thu, Feb 02, 2023 at 02:21:53PM +0000, Gary Guo wrote:
> > On Thu, 2 Feb 2023 10:14:06 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > > > This allows reading the current count of a refcount in an `ArcInner`.
> > > > 
> > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > ---
> > > >  rust/helpers.c          | 6 ++++++
> > > >  rust/kernel/sync/arc.rs | 9 +++++++++
> > > >  2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > > index 09a4d93f9d62..afc5f1a39fef 100644
> > > > --- a/rust/helpers.c
> > > > +++ b/rust/helpers.c
> > > > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> > > >  
> > > > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > > > +{
> > > > +	return refcount_read(r);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > > > +
> > > >  /*
> > > >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> > > >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > > > index fc680a4a795c..fbfceaa3096e 100644
> > > > --- a/rust/kernel/sync/arc.rs
> > > > +++ b/rust/kernel/sync/arc.rs
> > > > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> > > >      data: T,
> > > >  }
> > > >  
> > > > +impl<T: ?Sized> ArcInner<T> {
> > > > +    /// Returns the current reference count of [`ArcInner`].
> > > > +    fn count(&self) -> u32 {
> > > > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > > > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > > > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > > > +    }
> > > > +}  
> > > 
> > > This is completely unsafe vs concurrency. In order to enable correct
> > > tracing of refcount manipulations we have the __refcount_*(.oldp) API.
> > 
> > Retrieving the reference count is safe. It's just that in many
> > scenarios it's very hard to use the retrieved reference count
> > correctly, because it might be concurrently changed.
> 
> Yes, so you really should never ever ever care about the value, and that
> includes printing it out as it will be wrong the instant you read it.
> 

Agreed.

> > But there are correct ways to use a refcount, e.g. if you own
> > `Arc` and `.count()` returns 1, then you know that you are the
> > exclusive owner of the `Arc` and nobody else is going to touch it.
> 
> But you should never know this, as it is not relevant.
> 
> So no, please don't allow printing out of a reference count, that will
> only cause problems and allow people to think it is safe to do so.
> 

People already do it, even in *security* code,

security/keys/keyring.c:

	int key_link(struct key *keyring, struct key *key)
	{
		...
		kenter("{%d,%d}", keyring->serial, refcount_read(&keyring->usage));
		...
	}

Should we fix that?

Actually It is *safe* to do it, the existence of `ArcInner` proves the
object can not be freed while reading the refcount. This is somewhat
similar to the data_race() macro, we know there is a data race, but we
don't care because of the way that we are going to use the value won't
cause a problem.

What I propose is to print it in debug fmt only, anyway I'm OK to remove
it, but I'm really confused about the reasoning here.

Regards,
Boqun

> Peter is right, please don't do this.
> 
> thanks,
> 
> greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Greg KH]

On Thu, Feb 02, 2023 at 08:10:19AM -0800, Boqun Feng wrote:
> On Thu, Feb 02, 2023 at 04:41:47PM +0100, Greg KH wrote:
> > On Thu, Feb 02, 2023 at 02:21:53PM +0000, Gary Guo wrote:
> > > On Thu, 2 Feb 2023 10:14:06 +0100
> > > Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > On Wed, Feb 01, 2023 at 03:22:41PM -0800, Boqun Feng wrote:
> > > > > This allows reading the current count of a refcount in an `ArcInner`.
> > > > > 
> > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > > ---
> > > > >  rust/helpers.c          | 6 ++++++
> > > > >  rust/kernel/sync/arc.rs | 9 +++++++++
> > > > >  2 files changed, 15 insertions(+)
> > > > > 
> > > > > diff --git a/rust/helpers.c b/rust/helpers.c
> > > > > index 09a4d93f9d62..afc5f1a39fef 100644
> > > > > --- a/rust/helpers.c
> > > > > +++ b/rust/helpers.c
> > > > > @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> > > > >  
> > > > > +unsigned int rust_helper_refcount_read(refcount_t *r)
> > > > > +{
> > > > > +	return refcount_read(r);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> > > > > +
> > > > >  /*
> > > > >   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> > > > >   * as the Rust `usize` type, so we can use it in contexts where Rust
> > > > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > > > > index fc680a4a795c..fbfceaa3096e 100644
> > > > > --- a/rust/kernel/sync/arc.rs
> > > > > +++ b/rust/kernel/sync/arc.rs
> > > > > @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
> > > > >      data: T,
> > > > >  }
> > > > >  
> > > > > +impl<T: ?Sized> ArcInner<T> {
> > > > > +    /// Returns the current reference count of [`ArcInner`].
> > > > > +    fn count(&self) -> u32 {
> > > > > +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> > > > > +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> > > > > +        unsafe { bindings::refcount_read(self.refcount.get()) }
> > > > > +    }
> > > > > +}  
> > > > 
> > > > This is completely unsafe vs concurrency. In order to enable correct
> > > > tracing of refcount manipulations we have the __refcount_*(.oldp) API.
> > > 
> > > Retrieving the reference count is safe. It's just that in many
> > > scenarios it's very hard to use the retrieved reference count
> > > correctly, because it might be concurrently changed.
> > 
> > Yes, so you really should never ever ever care about the value, and that
> > includes printing it out as it will be wrong the instant you read it.
> > 
> 
> Agreed.
> 
> > > But there are correct ways to use a refcount, e.g. if you own
> > > `Arc` and `.count()` returns 1, then you know that you are the
> > > exclusive owner of the `Arc` and nobody else is going to touch it.
> > 
> > But you should never know this, as it is not relevant.
> > 
> > So no, please don't allow printing out of a reference count, that will
> > only cause problems and allow people to think it is safe to do so.
> > 
> 
> People already do it, even in *security* code,
> 
> security/keys/keyring.c:
> 
> 	int key_link(struct key *keyring, struct key *key)
> 	{
> 		...
> 		kenter("{%d,%d}", keyring->serial, refcount_read(&keyring->usage));
> 		...
> 	}
> 
> Should we fix that?

Yes.  But really, that's debugging code, it probably should all be
removed now.

thanks,

greg k-h

Re: [RFC 1/5] rust: sync: impl Display for {Unique,}Arc

[Björn Roy Baron]

On Thursday, February 2nd, 2023 at 00:22, Boqun Feng <boqun.feng@gmail.com> wrote:

> This allows printing the inner data of `Arc` and its friends if the
> inner data implements `Display`. It's useful for logging and debugging
> purpose.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>

> 
> ---
>  rust/kernel/sync/arc.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 519a6ec43644..fc680a4a795c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -22,6 +22,7 @@ use crate::{
>  };
>  use alloc::boxed::Box;
>  use core::{
> +    fmt,
>      marker::{PhantomData, Unsize},
>      mem::{ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
> @@ -522,3 +523,15 @@ impl<T: ?Sized> DerefMut for UniqueArc<T> {
>          unsafe { &mut self.inner.ptr.as_mut().data }
>      }
>  }
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for UniqueArc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> --
> 2.39.1

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

On Thu, Feb 02, 2023 at 05:17:55PM +0100, Greg KH wrote:
[...]
> > > > But there are correct ways to use a refcount, e.g. if you own
> > > > `Arc` and `.count()` returns 1, then you know that you are the
> > > > exclusive owner of the `Arc` and nobody else is going to touch it.
> > > 
> > > But you should never know this, as it is not relevant.
> > > 
> > > So no, please don't allow printing out of a reference count, that will
> > > only cause problems and allow people to think it is safe to do so.
> > > 
> > 
> > People already do it, even in *security* code,
> > 
> > security/keys/keyring.c:
> > 
> > 	int key_link(struct key *keyring, struct key *key)
> > 	{
> > 		...
> > 		kenter("{%d,%d}", keyring->serial, refcount_read(&keyring->usage));
> > 		...
> > 	}
> > 
> > Should we fix that?
> 
> Yes.  But really, that's debugging code, it probably should all be
> removed now.
> 

Well, there are also printings in proc_keys_show() and
proc_key_users_show() in security/keys/proc.c, and I think it's hard to
remove these since they are userspace related.

Anyway I realise I could have done a better job explaining what I'm
doing here:

I want to read refcount in a later patch, which make Arc<T> implement
Debug trait/interface, and that allows user to print Arc<T> for debug
purposes, e.g.

	pr_info!("{:#x?}", a); // a is an "Arc<T">

that's the only usage of the reading from refcount. I could open code an
FFI call in that implementation, but I thought maybe I could add a
helper function, hence the "count" function. And since "count" is a
private function, so no one can use it outside this
rust/kernel/sync/arc.rs file, therefore mis-usage by outsiders are
impossible.

Maybe I made things confusing because I just learned the language and
wanted to try out a few things which made things complicated (for
review), hope the above explanation undo some of the confusion I
created.

As I said, I'm open to remove the printing of the refcount, and if you
and Peter think maybe it's OK to do that after the explanation above,
I will improve the patch to make things clear ;-)

Regards,
Boqun

> thanks,
> 
> greg k-h

Re: [RFC 3/5] rust: sync: Arc: Introduces Arc::get_inner() helper

[Björn Roy Baron]

On Thursday, February 2nd, 2023 at 00:22, Boqun Feng <boqun.feng@gmail.com> wrote:


> Getting a `&ArcInner<T>` should always be safe from a `Arc<T>`,
> 
> therefore add this helper function to avoid duplicate unsafe
> `ptr.as_ref()`.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>

> ---
>  rust/kernel/sync/arc.rs | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fbfceaa3096e..4d8de20c996f 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -201,6 +201,13 @@ impl<T: ?Sized> Arc<T> {
>          // reference can be created.
>          unsafe { ArcBorrow::new(self.ptr) }
>      }
> +
> +    /// Returns a reference to the internal [`ArcInner`].
> +    fn get_inner(&self) -> &ArcInner<T> {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
>  }
> 
>  impl<T: 'static> ForeignOwnable for Arc<T> {
> @@ -233,9 +240,7 @@ impl<T: ?Sized> Deref for Arc<T> {
>      type Target = T;
> 
>      fn deref(&self) -> &Self::Target {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> -        // safe to dereference it.
> -        unsafe { &self.ptr.as_ref().data }
> +        &self.get_inner().data
>      }
>  }
> 
> @@ -244,7 +249,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>          // INVARIANT: C `refcount_inc` 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 { bindings::refcount_inc(self.get_inner().refcount.get()) };
> 
>          // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>          unsafe { Self::from_inner(self.ptr) }
> @@ -253,11 +258,7 @@ impl<T: ?Sized> Clone for Arc<T> {
> 
>  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();
> +        let refcount = self.get_inner().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.
> --
> 2.39.1

Re: [RFC 5/5] sample: rust: print: Add sampe code for Arc printing

[Björn Roy Baron]

On Thursday, February 2nd, 2023 at 00:22, Boqun Feng <boqun.feng@gmail.com> wrote:

> This both demonstrates the usage of different print format in Rust and
> serves as a selftest for the `Display` and `Debug` implementation of
> `Arc` and its friends.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>

> ---
>  samples/rust/rust_print.rs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
> index 8b39d9cef6d1..165a8d7b1c07 100644
> --- a/samples/rust/rust_print.rs
> +++ b/samples/rust/rust_print.rs
> @@ -15,6 +15,30 @@ module! {
> 
>  struct RustPrint;
> 
> +fn arc_print() -> Result {
> +    use kernel::sync::*;
> +
> +    let a = Arc::try_new(1)?;
> +    let b = UniqueArc::try_new("hello, world")?;
> +
> +    // Prints the value of data in `a`.
> +    pr_info!("{}", a);
> +
> +    // Uses ":?" to print debug fmt of `b`.
> +    pr_info!("{:?}", b);
> +
> +    let a: Arc<&str> = b.into();
> +    let c = a.clone();
> +
> +    // Uses `dbg` to print, will move `c`.
> +    dbg!(c);
> +
> +    // Prints debug fmt with pretty-print "#" and number-in-hex "x".
> +    pr_info!("{:#x?}", a);
> +
> +    Ok(())
> +}
> +
>  impl kernel::Module for RustPrint {
>      fn init(_module: &'static ThisModule) -> Result<Self> {
>          pr_info!("Rust printing macros sample (init)\n");
> @@ -43,6 +67,8 @@ impl kernel::Module for RustPrint {
>          pr_cont!(" is {}", "continued");
>          pr_cont!(" with {}\n", "args");
> 
> +        arc_print()?;
> +
>          Ok(RustPrint)
>      }
>  }
> --
> 2.39.1

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Miguel Ojeda]

On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> As I said, I'm open to remove the printing of the refcount, and if you
> and Peter think maybe it's OK to do that after the explanation above,

Perhaps part of the confusion came from the overloaded "safe" term.

When Gary and Boqun used the term "safe", they meant it in the Rust
sense, i.e. calling the method will not allow to introduce undefined
behavior. While I think Peter and Greg are using the term to mean
something different.

In particular, "safe" in Rust does not mean "hard to misuse" or "OK to
use in production" or "avoids functional bugs" or "prevents crashes"
and so on. And, yeah, this is a common source of misunderstandings,
and some argue a better term could have been chosen.

So it is possible to have perfectly safe Rust functions that are very
tricky to use or unintended for common usage. Now, of course, whether
having a particular function is a good idea or not is a different
story.

Boqun: maybe appending a small paragraph to the doc comments of the
function would alleviate concerns.

Cheers,
Miguel

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Greg KH]

On Thu, Feb 02, 2023 at 10:47:12PM +0100, Miguel Ojeda wrote:
> On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > As I said, I'm open to remove the printing of the refcount, and if you
> > and Peter think maybe it's OK to do that after the explanation above,
> 
> Perhaps part of the confusion came from the overloaded "safe" term.
> 
> When Gary and Boqun used the term "safe", they meant it in the Rust
> sense, i.e. calling the method will not allow to introduce undefined
> behavior. While I think Peter and Greg are using the term to mean
> something different.

Yes, I mean it in a "this is not giving you the value you think you are
getting and you can not rely on it for anything at all as it is going to
be incorrect" meaning.

Which in kernel code means "this is not something you should do".

thanks,

greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

On Fri, Feb 03, 2023 at 06:22:15AM +0100, Greg KH wrote:
> On Thu, Feb 02, 2023 at 10:47:12PM +0100, Miguel Ojeda wrote:
> > On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > As I said, I'm open to remove the printing of the refcount, and if you
> > > and Peter think maybe it's OK to do that after the explanation above,
> > 
> > Perhaps part of the confusion came from the overloaded "safe" term.
> > 
> > When Gary and Boqun used the term "safe", they meant it in the Rust
> > sense, i.e. calling the method will not allow to introduce undefined
> > behavior. While I think Peter and Greg are using the term to mean
> > something different.
> 
> Yes, I mean it in a "this is not giving you the value you think you are
> getting and you can not rely on it for anything at all as it is going to
> be incorrect" meaning.
> 
> Which in kernel code means "this is not something you should do".
> 

Now what really confuses me is why kref_read() is safe.. or how this is
different than kref_read(). Needless to say that ArcInner::count() can
guarantee not reading 0 (because of the type invariants) but kref_read()
cannot..

Regards,
Boqun

> thanks,
> 
> greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Greg KH]

On Thu, Feb 02, 2023 at 11:25:08PM -0800, Boqun Feng wrote:
> On Fri, Feb 03, 2023 at 06:22:15AM +0100, Greg KH wrote:
> > On Thu, Feb 02, 2023 at 10:47:12PM +0100, Miguel Ojeda wrote:
> > > On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > As I said, I'm open to remove the printing of the refcount, and if you
> > > > and Peter think maybe it's OK to do that after the explanation above,
> > > 
> > > Perhaps part of the confusion came from the overloaded "safe" term.
> > > 
> > > When Gary and Boqun used the term "safe", they meant it in the Rust
> > > sense, i.e. calling the method will not allow to introduce undefined
> > > behavior. While I think Peter and Greg are using the term to mean
> > > something different.
> > 
> > Yes, I mean it in a "this is not giving you the value you think you are
> > getting and you can not rely on it for anything at all as it is going to
> > be incorrect" meaning.
> > 
> > Which in kernel code means "this is not something you should do".
> > 
> 
> Now what really confuses me is why kref_read() is safe..

It isn't, and I hate it and it should be removed from the kernel
entirely.  But the scsi and drm developers seem to insist that "their
locking model ensures it will be safe to use" and I lost that argument
:(

> or how this is different than kref_read().

It isn't, but again, I don't like that and do not agree it should be
used as it is almost always a sign that the logic in the code is
incorrect.

> Needless to say that ArcInner::count() can guarantee not reading 0

How?  Because you have an implicit reference on it already?  If so, then
why does reading from it matter at all, as if you have a reference, you
know it isn't 0, and that's all that you can really care about.  You
don't care about any number other than 0 for a reference count, as by
definition, that's what a reference count does :)

> (because of the type invariants) but kref_read() cannot..

I totally agree with you.  Let's not mirror bad decisions of legacy
subsystems in the kernel written in C with new designs in Rust please.

thanks,

greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

On Fri, Feb 03, 2023 at 08:38:25AM +0100, Greg KH wrote:
> On Thu, Feb 02, 2023 at 11:25:08PM -0800, Boqun Feng wrote:
> > On Fri, Feb 03, 2023 at 06:22:15AM +0100, Greg KH wrote:
> > > On Thu, Feb 02, 2023 at 10:47:12PM +0100, Miguel Ojeda wrote:
> > > > On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > >
> > > > > As I said, I'm open to remove the printing of the refcount, and if you
> > > > > and Peter think maybe it's OK to do that after the explanation above,
> > > > 
> > > > Perhaps part of the confusion came from the overloaded "safe" term.
> > > > 
> > > > When Gary and Boqun used the term "safe", they meant it in the Rust
> > > > sense, i.e. calling the method will not allow to introduce undefined
> > > > behavior. While I think Peter and Greg are using the term to mean
> > > > something different.
> > > 
> > > Yes, I mean it in a "this is not giving you the value you think you are
> > > getting and you can not rely on it for anything at all as it is going to
> > > be incorrect" meaning.
> > > 
> > > Which in kernel code means "this is not something you should do".
> > > 
> > 
> > Now what really confuses me is why kref_read() is safe..
> 
> It isn't, and I hate it and it should be removed from the kernel
> entirely.  But the scsi and drm developers seem to insist that "their
> locking model ensures it will be safe to use" and I lost that argument
> :(
> 
> > or how this is different than kref_read().
> 
> It isn't, but again, I don't like that and do not agree it should be
> used as it is almost always a sign that the logic in the code is
> incorrect.
> 
> > Needless to say that ArcInner::count() can guarantee not reading 0
> 
> How?  Because you have an implicit reference on it already?  If so, then
> why does reading from it matter at all, as if you have a reference, you
> know it isn't 0, and that's all that you can really care about.  You
> don't care about any number other than 0 for a reference count, as by
> definition, that's what a reference count does :)
> 

Fair enough!

> > (because of the type invariants) but kref_read() cannot..
> 
> I totally agree with you.  Let's not mirror bad decisions of legacy
> subsystems in the kernel written in C with new designs in Rust please.
> 

Roger that, will remove this in the next version ;-)

Regards,
Boqun

> thanks,
> 
> greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Boqun Feng]

On Fri, Feb 03, 2023 at 08:38:25AM +0100, Greg KH wrote:
[...]
> > Needless to say that ArcInner::count() can guarantee not reading 0
> 
> How?  Because you have an implicit reference on it already?  If so, then

Yes, roughly a reference ("&") in Rust can be treated as a
compile-time reference count, so the existence of "&" in fact prevents
the underlying data from going away, or in Rust term, being "drop"ped.

To get a "&ArcInner<T>", we need a "&Arc<T>", and as long as there is
a reference to an "Arc<T>", the object won't be dropped, that's the
proof of the underly object still being referenced.

Other folks may explain this better and accurate, but that's the basic
idea ;-)

Regards,
Boqun

> why does reading from it matter at all, as if you have a reference, you
> know it isn't 0, and that's all that you can really care about.  You
> don't care about any number other than 0 for a reference count, as by
> definition, that's what a reference count does :)
> 

[...]

> thanks,
> 
> greg k-h

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Josh Stone]

On 2/2/23 11:38 PM, Greg KH wrote:
> How?  Because you have an implicit reference on it already?  If so, then
> why does reading from it matter at all, as if you have a reference, you
> know it isn't 0, and that's all that you can really care about.  You
> don't care about any number other than 0 for a reference count, as by
> definition, that's what a reference count does :)

There is an additional ability for 1, mentioned up thread -- if you have
&mut Arc<T>, and the inner count is 1, then you *know* there aren't any
other Arc<T> handles anywhere else. Then it is safe to return an
exclusive &mut T, like the upstream Arc::get_mut and Arc::make_mut. This
can also be used for owned Arc<T> like the upstream Arc::try_unwrap.

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Wedson Almeida Filho]

On Fri, 3 Feb 2023 at 16:17, Josh Stone <jistone@redhat.com> wrote:
>
> On 2/2/23 11:38 PM, Greg KH wrote:
> > How?  Because you have an implicit reference on it already?  If so, then
> > why does reading from it matter at all, as if you have a reference, you
> > know it isn't 0, and that's all that you can really care about.  You
> > don't care about any number other than 0 for a reference count, as by
> > definition, that's what a reference count does :)
>
> There is an additional ability for 1, mentioned up thread -- if you have
> &mut Arc<T>, and the inner count is 1, then you *know* there aren't any
> other Arc<T> handles anywhere else. Then it is safe to return an
> exclusive &mut T, like the upstream Arc::get_mut and Arc::make_mut. This
> can also be used for owned Arc<T> like the upstream Arc::try_unwrap.

The `Arc<T>` in the kernel is pinned so we can't return an `&mut T`,
though a `Pin<&mut T>` would be ok.

Re: [RFC 4/5] rust: sync: impl Debug for {Unique,}Arc

[Boqun Feng]

On Thu, Feb 02, 2023 at 02:28:04PM +0000, Gary Guo wrote:
> On Wed,  1 Feb 2023 15:22:43 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > This allows printing macros to print the data and the refcount nubmer
> > of these struct for debugging purposes.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/sync/arc.rs | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index 4d8de20c996f..f143d8305c36 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -474,6 +474,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> >  ///
> >  /// # test().unwrap();
> >  /// ```
> > +#[derive(Debug)]
> 
> I don't think this should be a `#[derive(Debug)]`. For `UniqueArc` the
> refcount field in `Arc` is useless, and we should just delegate the
> `Debug` impl to that of deref, just like `Display` does.
> 

I was just being lazy ;-) Will change this in v2.

Regards,
Boqun

> Best,
> Gary

Re: [RFC 1/5] rust: sync: impl Display for {Unique,}Arc

[Finn Behrens]

On 2 Feb 2023, at 0:22, Boqun Feng wrote:

> This allows printing the inner data of `Arc` and its friends if the
> inner data implements `Display`. It's useful for logging and debugging
> purpose.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Finn Behrens <fin@nyantec.com>

> ---
>  rust/kernel/sync/arc.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 519a6ec43644..fc680a4a795c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -22,6 +22,7 @@ use crate::{
>  };
>  use alloc::boxed::Box;
>  use core::{
> +    fmt,
>      marker::{PhantomData, Unsize},
>      mem::{ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
> @@ -522,3 +523,15 @@ impl<T: ?Sized> DerefMut for UniqueArc<T> {
>          unsafe { &mut self.inner.ptr.as_mut().data }
>      }
>  }
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for UniqueArc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> -- 
> 2.39.1

Re: [RFC 5/5] sample: rust: print: Add sampe code for Arc printing

[Finn Behrens]

On 2 Feb 2023, at 0:22, Boqun Feng wrote:

> This both demonstrates the usage of different print format in Rust and
> serves as a selftest for the `Display` and `Debug` implementation of
> `Arc` and its friends.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Finn Behrens <fin@nyantec.com>

> ---
>  samples/rust/rust_print.rs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
> index 8b39d9cef6d1..165a8d7b1c07 100644
> --- a/samples/rust/rust_print.rs
> +++ b/samples/rust/rust_print.rs
> @@ -15,6 +15,30 @@ module! {
>
>  struct RustPrint;
>
> +fn arc_print() -> Result {
> +    use kernel::sync::*;
> +
> +    let a = Arc::try_new(1)?;
> +    let b = UniqueArc::try_new("hello, world")?;
> +
> +    // Prints the value of data in `a`.
> +    pr_info!("{}", a);
> +
> +    // Uses ":?" to print debug fmt of `b`.
> +    pr_info!("{:?}", b);
> +
> +    let a: Arc<&str> = b.into();
> +    let c = a.clone();
> +
> +    // Uses `dbg` to print, will move `c`.
> +    dbg!(c);
> +
> +    // Prints debug fmt with pretty-print "#" and number-in-hex "x".
> +    pr_info!("{:#x?}", a);
> +
> +    Ok(())
> +}
> +
>  impl kernel::Module for RustPrint {
>      fn init(_module: &'static ThisModule) -> Result<Self> {
>          pr_info!("Rust printing macros sample (init)\n");
> @@ -43,6 +67,8 @@ impl kernel::Module for RustPrint {
>          pr_cont!(" is {}", "continued");
>          pr_cont!(" with {}\n", "args");
>
> +        arc_print()?;
Wonder if it makes sense to also extract the other printing functions to group usage.
> +
>          Ok(RustPrint)
>      }
>  }
> -- 
> 2.39.1

Re: [RFC 1/5] rust: sync: impl Display for {Unique,}Arc

[Vincenzo Palazzo]

On Thu Feb 2, 2023 at 12:22 AM CET, Boqun Feng wrote:
> This allows printing the inner data of `Arc` and its friends if the
> inner data implements `Display`. It's useful for logging and debugging
> purpose.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---

Reviwed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

>  rust/kernel/sync/arc.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 519a6ec43644..fc680a4a795c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -22,6 +22,7 @@ use crate::{
>  };
>  use alloc::boxed::Box;
>  use core::{
> +    fmt,
>      marker::{PhantomData, Unsize},
>      mem::{ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
> @@ -522,3 +523,15 @@ impl<T: ?Sized> DerefMut for UniqueArc<T> {
>          unsafe { &mut self.inner.ptr.as_mut().data }
>      }
>  }
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for UniqueArc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> +
> +impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(self.deref(), f)
> +    }
> +}
> -- 
> 2.39.1

Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()

[Vincenzo Palazzo]

On Thu Feb 2, 2023 at 12:22 AM CET, Boqun Feng wrote:
> This allows reading the current count of a refcount in an `ArcInner`.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
Reviwed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

>  rust/helpers.c          | 6 ++++++
>  rust/kernel/sync/arc.rs | 9 +++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 09a4d93f9d62..afc5f1a39fef 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -46,6 +46,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
>  
> +unsigned int rust_helper_refcount_read(refcount_t *r)
> +{
> +	return refcount_read(r);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_read);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fc680a4a795c..fbfceaa3096e 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -127,6 +127,15 @@ struct ArcInner<T: ?Sized> {
>      data: T,
>  }
>  
> +impl<T: ?Sized> ArcInner<T> {
> +    /// Returns the current reference count of [`ArcInner`].
> +    fn count(&self) -> u32 {
> +        // SAFETY: `self.refcount.get()` is always a valid pointer, and `refcount_read()` is a
> +        // normal atomic read (i.e. no data race) only requiring on the address is valid.
> +        unsafe { bindings::refcount_read(self.refcount.get()) }
> +    }
> +}
> +
>  // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
>  impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
>  
> -- 
> 2.39.1

Re: [RFC 3/5] rust: sync: Arc: Introduces Arc::get_inner() helper

[Vincenzo Palazzo]

On Thu Feb 2, 2023 at 12:22 AM CET, Boqun Feng wrote:
> Getting a `&ArcInner<T>` should always be safe from a `Arc<T>`,
> therefore add this helper function to avoid duplicate unsafe
> `ptr.as_ref()`.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---

Reviwed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

>  rust/kernel/sync/arc.rs | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index fbfceaa3096e..4d8de20c996f 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -201,6 +201,13 @@ impl<T: ?Sized> Arc<T> {
>          // reference can be created.
>          unsafe { ArcBorrow::new(self.ptr) }
>      }
> +
> +    /// Returns a reference to the internal [`ArcInner`].
> +    fn get_inner(&self) -> &ArcInner<T> {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
>  }
>  
>  impl<T: 'static> ForeignOwnable for Arc<T> {
> @@ -233,9 +240,7 @@ impl<T: ?Sized> Deref for Arc<T> {
>      type Target = T;
>  
>      fn deref(&self) -> &Self::Target {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> -        // safe to dereference it.
> -        unsafe { &self.ptr.as_ref().data }
> +        &self.get_inner().data
>      }
>  }
>  
> @@ -244,7 +249,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>          // INVARIANT: C `refcount_inc` 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 { bindings::refcount_inc(self.get_inner().refcount.get()) };
>  
>          // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>          unsafe { Self::from_inner(self.ptr) }
> @@ -253,11 +258,7 @@ impl<T: ?Sized> Clone for Arc<T> {
>  
>  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();
> +        let refcount = self.get_inner().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.
> -- 
> 2.39.1

Re: [RFC 4/5] rust: sync: impl Debug for {Unique,}Arc

[Vincenzo Palazzo]

On Thu Feb 2, 2023 at 12:22 AM CET, Boqun Feng wrote:
> This allows printing macros to print the data and the refcount nubmer
> of these struct for debugging purposes.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
Reviwed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

>  rust/kernel/sync/arc.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 4d8de20c996f..f143d8305c36 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -474,6 +474,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
>  ///
>  /// # test().unwrap();
>  /// ```
> +#[derive(Debug)]
>  pub struct UniqueArc<T: ?Sized> {
>      inner: Arc<T>,
>  }
> @@ -545,3 +546,15 @@ impl<T: fmt::Display + ?Sized> fmt::Display for Arc<T> {
>          fmt::Display::fmt(self.deref(), f)
>      }
>  }
> +
> +impl<T: fmt::Debug + ?Sized> fmt::Debug for Arc<T> {
> +    /// Formats debug output for [`Arc`].
> +    ///
> +    /// Refcount is also printed for debugging purpose.
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        f.debug_struct("Arc")
> +            .field("refcount", &self.get_inner().count())
> +            .field("data", &self.deref())
> +            .finish()
> +    }
> +}
> -- 
> 2.39.1

Re: [RFC 5/5] sample: rust: print: Add sampe code for Arc printing

[Vincenzo Palazzo]

On Thu Feb 2, 2023 at 12:22 AM CET, Boqun Feng wrote:
> This both demonstrates the usage of different print format in Rust and
> serves as a selftest for the `Display` and `Debug` implementation of
> `Arc` and its friends.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
Reviwed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

>  samples/rust/rust_print.rs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
> index 8b39d9cef6d1..165a8d7b1c07 100644
> --- a/samples/rust/rust_print.rs
> +++ b/samples/rust/rust_print.rs
> @@ -15,6 +15,30 @@ module! {
>  
>  struct RustPrint;
>  
> +fn arc_print() -> Result {
> +    use kernel::sync::*;
> +
> +    let a = Arc::try_new(1)?;
> +    let b = UniqueArc::try_new("hello, world")?;
> +
> +    // Prints the value of data in `a`.
> +    pr_info!("{}", a);
> +
> +    // Uses ":?" to print debug fmt of `b`.
> +    pr_info!("{:?}", b);
> +
> +    let a: Arc<&str> = b.into();
> +    let c = a.clone();
> +
> +    // Uses `dbg` to print, will move `c`.
> +    dbg!(c);
> +
> +    // Prints debug fmt with pretty-print "#" and number-in-hex "x".
> +    pr_info!("{:#x?}", a);
> +
> +    Ok(())
> +}
> +
>  impl kernel::Module for RustPrint {
>      fn init(_module: &'static ThisModule) -> Result<Self> {
>          pr_info!("Rust printing macros sample (init)\n");
> @@ -43,6 +67,8 @@ impl kernel::Module for RustPrint {
>          pr_cont!(" is {}", "continued");
>          pr_cont!(" with {}\n", "args");
>  
> +        arc_print()?;
> +
>          Ok(RustPrint)
>      }
>  }
> -- 
> 2.39.1