linux-kernel - Re: [RFC][PATCH 7/7] kref: Implement using refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Nov 2016 16:38:22 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Will Deacon <will.deacon@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Reshetova, Elena" <elena.reshetova@...el.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "hpa@...or.com" <hpa@...or.com>,
        "dave@...gbits.org" <dave@...gbits.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

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

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)