Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

[Peter Zijlstra]

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.