Re: [PATCH v5] futex: Remove requirement for lock_page in get_futex_key

[Ingo Molnar]

* Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> On Thu, 4 Feb 2016, Davidlohr Bueso wrote:
> 
> > On Thu, 04 Feb 2016, Thomas Gleixner wrote:
> > 
> > > On Wed, 3 Feb 2016, Davidlohr Bueso wrote:
> > > > +		 * We are not calling into get_futex_key_refs() in file-backed
> > > > +		 * cases, therefore a successful atomic_inc return below will
> > > > +		 * guarantee that get_futex_key() will continue to imply MB
> > > > (B).
> > > 
> > > Can you please make that "MB (B)" part a bit more outstanding. I really had
> > > to
> > > search for it.
> > 
> > Hmm as you know this is mostly explained at the begining of the file, and we
> > sprinkle MB (B) around the code based on that description. So I'm a bit
> > confused
> > as to why you don't like like that comment.
> 
> The other "MB (B)" places are more outstanding. It did not spring in my eye 
> immideately. So it's a pure cosmetic issue.

So I too didn't understand that sentence at first, because the capitalization 
really throws off quick parsing of that comment, as 'MB' ususally denotes 
megabytes.

So please change it to "mb(); (A)" or so - and I think all of these comments 
should be changed to use a standard API name for the barrier they imply, as the 
head of futex.c does:

 *   waiters++; (a)
 *   mb(); (A) <-- paired with -.
 *                              |
 *   lock(hash_bucket(futex));  |
 *                              |
 *   uval = *futex;             |
 *                              |        *futex = newval;
 *                              |        sys_futex(WAKE, futex);
 *                              |          futex_wake(futex);
 *                              |
 *                              `------->  mb(); (B)

Btw., pedantic: shouldn't that be smp_mb()? Futexes don't operate on IO spaces, so 
on UP they only need compiler barriers.

Thanks,

	Ingo