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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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