Re: [RFC][PATCH 2/7] kref: Add kref_read()

From: Peter Zijlstra <peterz@infradead.org>
To: David Windsor <dwindsor@gmail.com>
Cc: "Reshetova, Elena" <elena.reshetova@intel.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Kees Cook <keescook@chromium.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Will Deacon <will.deacon@arm.com>, Arnd Bergmann <arnd@arndb.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()
Date: Tue, 22 Nov 2016 11:37:21 +0100	[thread overview]
Message-ID: <20161122103721.GO3092@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <CAEXv5_hXEOkwQw6zaood7f4Pugqn6ELsa4w89eD2Umv5m5Pyug@mail.gmail.com>

On Mon, Nov 21, 2016 at 03:12:33PM -0500, David Windsor wrote:
> On Mon, Nov 21, 2016 at 2:27 PM, Reshetova, Elena
> <elena.reshetova@intel.com> wrote:
> >> On Mon, Nov 21, 2016 at 04:49:15PM +0100, Peter Zijlstra wrote:
> >> > > Speaking of non-fitting patterns. This one is quite common in
> >> > > networking code for refcounters:
> >> > >
> >> > > if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {} This is from
> >> > > net/netfilter/nfnetlink_acct.c, but there are similar ones in other
> >> > > places.
> >> >
> >> > Cute, but weird it doesn't actually decrement if not 1.
> >>
> >> Hurgh.. creative refcounting that. The question is how much of that do
> >> we want to support? It really must not decrement there.

Now, arguably the 1->0 case is special, and we can provide limited
support for that, but I'd be hesitant to provide the full cmpxchg.

We could for instance provide: refcount_dec_if_one().

> > And one more creative usage:
> >
> > http://lxr.free-electrons.com/source/net/ipv4/udp.c#L1940
> >
> > if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
> >     return;
> >
> > I didn't even guess anyone is using atomic_inc_not_zero_hint...
> > But network code keeps surprising me today :)
> > So, yes, I guess the question is what to do with these cases really?
> 
> Many of the calls to non-supported functions can be decomposed into
> calls to supported functions. 

So it really depends on what the network guys are willing to put up
with, if their primary goal is to avoid the SHARED state, we could add a
load-exclusive. But I suspect they'd not be happy with that either...

> The ones that may prove interesting are
> ones like atomic_cmpxchg(), in which some sort of external locking is
> going to be required to achieve the same atomicity guarantees provided
> by cmpxchg, like so:
> 
> mutex_lock(lock);
> cnt = refcount_read(ref);
> if (cnt == val1) {
>     refcount_set(ref, val2);
> }
> mutex_unlock(lock);
> return cnt;

That cannot actually work in the presence of actual atomic instructions
not serialized by that lock.

Also, the network guys will absolutely kill you if you propose something
like that.