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