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

[Davidlohr Bueso]

On Fri, 05 Feb 2016, Ingo Molnar wrote:

> 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.

Sure, fair enough.

> 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.

Right, but we do in fact use smp barriers in this cases in the real code, that mb() is
just in the comments, I guess it would be desirable to change it to smp_mb nonetheless.

However, could these changes be in a followup? Mainly because the barrier B references
will be updated across all futex.c... unless there are still concerns about this particular
patch, of course.

Thanks,
Davidlohr