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

[Ingo Molnar <mingo@kernel.org>]

* 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