From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753383AbcKRS50 (ORCPT ); Fri, 18 Nov 2016 13:57:26 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:59436 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753093AbcKRS5W (ORCPT ); Fri, 18 Nov 2016 13:57:22 -0500 Date: Fri, 18 Nov 2016 19:57:19 +0100 From: Peter Zijlstra To: Will Deacon Cc: "Reshetova, Elena" , "gregkh@linuxfoundation.org" , "keescook@chromium.org" , "arnd@arndb.de" , "tglx@linutronix.de" , "mingo@kernel.org" , "hpa@zytor.com" , "dave@progbits.org" , "linux-kernel@vger.kernel.org" , boqun.feng@gmail.com Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Message-ID: <20161118185719.GY3117@twins.programming.kicks-ass.net> References: <20161114173946.501528675@infradead.org> <20161114174446.832175072@infradead.org> <2236FBA76BA1254E88B949DDB74E612B41C148C4@IRSMSX102.ger.corp.intel.com> <20161118113718.GP3117@twins.programming.kicks-ass.net> <20161118170655.GZ13470@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161118170655.GZ13470@arm.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Nov 18, 2016 at 05:06:55PM +0000, Will Deacon wrote: > On Fri, Nov 18, 2016 at 12:37:18PM +0100, Peter Zijlstra wrote: > > +static inline void refcount_set(refcount_t *r, int n) > > +{ > > + atomic_set(&r->refs, n); > > +} > > + > > +static inline unsigned int refcount_read(const refcount_t *r) > > +{ > > + return atomic_read(&r->refs); > > +} > > Minor nit, but it might be worth being consistent in our usage of int > (parameter to refcount_set) and unsigned int (return value of > refcount_read). Duh, I actually spotted that once and still didn't fix that :/ > > +static inline __must_check > > +bool refcount_inc_not_zero(refcount_t *r) > > +{ > > + unsigned int old, new, val = atomic_read(&r->refs); > > + > > + for (;;) { > > + if (!val) > > + return false; > > + > > + if (unlikely(val == UINT_MAX)) > > + return true; > > + > > + new = val + 1; > > + old = atomic_cmpxchg_relaxed(&r->refs, val, new); > > + if (old == val) > > + break; > > + > > + val = old; > > Hmm, it's a shame this code is duplicated from refcount_inc, but I suppose > you can actually be racing against the counter going to zero here and really > need to check it each time round the loop. Humph. That said, given that > refcount_inc WARNs if the thing is zero, maybe that could just call > refcount_inc_not_zero and warn if it returns false? Does it matter that > we don't actually do the increment? Dunno, it _should_ not, but then again, who knows. I can certainly write it as WARN_ON(!refcount_inc_not_zero()); > > +static inline __must_check > > +bool refcount_dec_and_test(refcount_t *r) > > +{ > > + unsigned int old, new, val = atomic_read(&r->refs); > > + > > + for (;;) { > > + if (val == UINT_MAX) > > + return false; > > + > > + new = val - 1; > > + if (WARN(new > val, "refcount_t: underflow; use-after-free.\n")) > > + return false; > > Wouldn't it be clearer to compare val with 0 before doing the decrement? Maybe, this way you can change the 1 and it'll keep working. Then again, you can't do that with the inc side, so who cares.