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
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 <[email protected]>
---
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
This allows reading the current count of a refcount in an `ArcInner`.
Signed-off-by: Boqun Feng <[email protected]>
---
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
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 <[email protected]>
---
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
This allows printing macros to print the data and the refcount nubmer
of these struct for debugging purposes.
Signed-off-by: Boqun Feng <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
> > ---
> > 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
On Wed, 1 Feb 2023 15:22:40 -0800
Boqun Feng <[email protected]> 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 <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
> ---
> 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)
> + }
> +}
On Thu, 2 Feb 2023 10:14:06 +0100
Peter Zijlstra <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Wed, 1 Feb 2023 15:22:41 -0800
Boqun Feng <[email protected]> wrote:
> This allows reading the current count of a refcount in an `ArcInner`.
>
> Signed-off-by: Boqun Feng <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
> ---
> 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> {}
>
On Wed, 1 Feb 2023 15:22:42 -0800
Boqun Feng <[email protected]> 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 <[email protected]>
Reviewed-by: Gary Guo <[email protected]>
> ---
> 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.
On Wed, 1 Feb 2023 15:22:43 -0800
Boqun Feng <[email protected]> wrote:
> This allows printing macros to print the data and the refcount nubmer
> of these struct for debugging purposes.
>
> Signed-off-by: Boqun Feng <[email protected]>
> ---
> 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
On Thu, Feb 02, 2023 at 02:21:53PM +0000, Gary Guo wrote:
> On Thu, 2 Feb 2023 10:14:06 +0100
> Peter Zijlstra <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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
On Thursday, February 2nd, 2023 at 00:22, Boqun Feng <[email protected]> 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 <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
>
> ---
> 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
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
On Thursday, February 2nd, 2023 at 00:22, Boqun Feng <[email protected]> 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 <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
> ---
> 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
On Thursday, February 2nd, 2023 at 00:22, Boqun Feng <[email protected]> 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 <[email protected]>
Reviewed-by: Björn Roy Baron <[email protected]>
> ---
> 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
On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <[email protected]> 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
On Thu, Feb 02, 2023 at 10:47:12PM +0100, Miguel Ojeda wrote:
> On Thu, Feb 2, 2023 at 5:52 PM Boqun Feng <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
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
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.
On Fri, 3 Feb 2023 at 16:17, Josh Stone <[email protected]> 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.
On Thu, Feb 02, 2023 at 02:28:04PM +0000, Gary Guo wrote:
> On Wed, 1 Feb 2023 15:22:43 -0800
> Boqun Feng <[email protected]> wrote:
>
> > This allows printing macros to print the data and the refcount nubmer
> > of these struct for debugging purposes.
> >
> > Signed-off-by: Boqun Feng <[email protected]>
> > ---
> > 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
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 <[email protected]>
Reviewed-by: Finn Behrens <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Finn Behrens <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
Reviwed-by: Vincenzo Palazzo <[email protected]>
> 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
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 <[email protected]>
> ---
Reviwed-by: Vincenzo Palazzo <[email protected]>
> 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
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 <[email protected]>
> ---
Reviwed-by: Vincenzo Palazzo <[email protected]>
> 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
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 <[email protected]>
> ---
Reviwed-by: Vincenzo Palazzo <[email protected]>
> 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
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 <[email protected]>
> ---
Reviwed-by: Vincenzo Palazzo <[email protected]>
> 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