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

From: Ingo Molnar <mingo@kernel.org>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Reshetova, Elena" <elena.reshetova@intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"dave@progbits.org" <dave@progbits.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t
Date: Mon, 21 Nov 2016 08:48:26 +0100	[thread overview]
Message-ID: <20161121074826.GA29412@gmail.com> (raw)
In-Reply-To: <20161121040644.GE5227@tardis.cn.ibm.com>

* Boqun Feng <boqun.feng@gmail.com> wrote:

> > It also fails to decrement in the underflow case (which is fine, but not
> > obvious from the comment). Same thing below.
> > 
> 
> Maybe a table in the comment like the following helps?
> 
> /*
>  * T: return true, F: return fasle
>  * W: trigger WARNING
>  * N: no effect
>  *
>  *                      |       value before ops                  |
>  *                      |   0   |   1   | UINT_MAX - 1 | UINT_MAX |
>  * ---------------------+-------+-------+--------------+----------+
>  * inc()                |  W    |       |      W       |      N   |
>  * inc_not_zero()       |   FN  |   T   |      WT      |    WTN   |
>  * dec_and_test()       |  WFN  |   T   |       F      |     FN   |
>  * dec_and_mutex_lock() |  WFN  |   T   |       F      |     FN   |
>  * dec_and_spin_lock()  |  WFN  |   T   |       F      |     FN   |
>  */

Yes!

nit: s/fasle/false

Also, I think we want to do a couple of other changes as well to make it more 
readable, extend the columns with 'normal' values (2 and UINT_MAX-2) and order the 
colums properly. I.e. something like:

/*
 * The before/after outcome of various atomic ops:
 *
 *   T: returns true
 *   F: returns false
 *   ----------------------------------
 *   W: op triggers kernel WARNING
 *   ----------------------------------
 *   0: no change to atomic var value
 *   +: atomic var value increases by 1
 *   -: atomic var value decreases by 1
 *   ----------------------------------
 *  -1: UINT_MAX
 *  -2: UINT_MAX-1
 *  -3: UINT_MAX-2
 *
 * ---------------------+-----+-----+-----+-----+-----+-----+
 * value before:        |  -3 |  -2 |  -1 |   0 |   1 |   2 |
 * ---------------------+-----+-----+-----+-----+-----+-----+
 * value+effect after:                                      |
 * ---------------------+     |     |     |     |     |     |
 * inc()                | ..+ | W.+ | ..0 | W.+ | ..+ | ..+ |
 * inc_not_zero()       | .T+ | WT+ | WT0 | .F0 | .T+ | .T+ |
 * dec_and_test()       | .F- | .F- | .F0 | WF0 | .T- | .F- |
 * dec_and_mutex_lock() | .F- | .F- | .F0 | WF0 | .T- | .F- |
 * dec_and_spin_lock()  | .F- | .F- | .F0 | WF0 | .T- | .F- |
 * ---------------------+-----+-----+-----+-----+-----+-----+
 *
 * So for example: 'WT+' in the inc_not_zero() row and '-2' column
 * means that when the atomic_inc_not_zero() function is called
 * with an atomic var that has a value of UINT_MAX-1, then the
 * atomic var's value will increase to the maximum overflow value
 * of UINT_MAX and will produce a warning. The function returns
 * 'true'.
 */

I think this table makes the overflow/underflow semantics pretty clear and also 
documents the regular behavior of these atomic ops pretty intuitively.

Agreed?

Thanks,

	Ingo