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

From: Greg KH <gregkh@linuxfoundation.org>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Peter Zijlstra" <peterz@infradead.org>,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Will Deacon" <will@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Vincenzo Palazzo" <vincenzopalazzodev@gmail.com>
Subject: Re: [RFC 2/5] rust: sync: Arc: Introduces ArcInner::count()
Date: Fri, 3 Feb 2023 08:38:25 +0100	[thread overview]
Message-ID: <Y9y58cy1GKZLdyjb@kroah.com> (raw)
In-Reply-To: <Y9y21JyuPF1nCCzr@Boquns-Mac-mini.local>

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