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

[Boqun Feng]

On Mon, Nov 21, 2016 at 08:48:26AM +0100, Ingo Molnar wrote:
> 
> * Boqun Feng <boqun.feng@xxxxxxxxx> 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?
> 

Sure, this looks pretty great! Much more informative and readable than
my version ;-) Thank you.

Regards,
Boqun

> Thanks,
> 
> 	Ingo

Attachment: signature.asc
Description: PGP signature