On Mon, Nov 21, 2016 at 08:48:26AM +0100, Ingo Molnar wrote: > > * Boqun Feng <boqun.f...@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? >
Sure, this looks pretty great! Much more informative and readable than my version ;-) Thank you. Regards, Boqun > Thanks, > > Ingo
signature.asc
Description: PGP signature
