Messages in this thread | | | Date | Thu, 2 Feb 2023 17:17:55 +0100 | From | Greg KH <> | Subject | Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count() |
| |
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
| |