Re: [PATCH] kref: prefer atomic_inc_not_zero to atomic_add_unless

From: Daniel Vetter <daniel@ffwll.ch>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>,
	Christoph Hellwig <hch@lst.de>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH] kref: prefer atomic_inc_not_zero to atomic_add_unless
Date: Fri, 16 Dec 2016 08:36:32 +0100	[thread overview]
Message-ID: <20161216073632.ma3fqcjhgzfyebog@phenom.ffwll.local> (raw)
In-Reply-To: <20161215191049.GB19707@kroah.com>

On Thu, Dec 15, 2016 at 11:10:49AM -0800, Greg KH wrote:
> On Thu, Dec 15, 2016 at 07:55:54PM +0100, Jason A. Donenfeld wrote:
> > On most platforms, there exists this ifdef:
> > 
> >  #define atomic_inc_not_zero(v) atomic_add_unless((v), 1, 0)
> > 
> > This makes this patch functionally useless. However, on PPC, there is
> > actually an explicit definition of atomic_inc_not_zero with its own
> > assembly that is slightly more optimized than atomic_add_unless. So,
> > this patch changes kref to use atomic_inc_not_zero instead, for PPC and
> > any future platforms that might provide an explicit implementation.
> > 
> > This also puts this usage of kref more in line with a verbatim reading
> > of the examples in Paul McKenney's paper [1] in the section titled "2.4
> > Atomic Counting With Check and Release Memory Barrier", which uses
> > atomic_inc_not_zero.
> > 
> > [1] http://open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2167.pdf
> > 
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > Sorry to submit this again, but people keep reviewing it saying it's fine,
> > but then point to somebody else to actually merge this. At the end of the
> > chain of fingerpointing is usually Greg. "Just have Greg do it." At this
> > point I'm confused, but it's certainly been sufficiently reviewed and
> > accepted. So can one of you just respond saying "I'll take it!"
> 
> Well, the crazies over in drm land were the ones that merged this new
> api, so they should be the ones responsible for it.  But that was way
> back in 2012, odds are they don't remember it given the lunacy that is
> their subsystem...

We do, it's just that I couldn't find Jason's patch when Thomas reviewed
it and asked for a resend and it took Jason a while to do that ...

Maybe we even remember this api way too well, we're constantly adding new
users of it in drm ;-)

> I'll take it after 4.10-rc1 is out, thanks.

Oh, here's another resubmission of this patch. I've already applied this
to my 4.11 queue, will show up in linux-next as soon as -rc1 is out.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch