Messages in this thread | | | Date | Tue, 1 Mar 2016 22:39:14 -0800 | From | Darren Hart <> | Subject | Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() |
| |
On Wed, Mar 02, 2016 at 10:31:18AM +0800, Jianyu Zhan wrote: > On Wed, Mar 2, 2016 at 9:37 AM, Darren Hart <dvhart@infradead.org> wrote: > > read_once will use a *volatile assignment instead of calling barrier() > > directly for a word size argument. > > > > With weak statements like "apparently" (above) and "could be" (from the original > > post: This patch replaces this bare barrier() with a READ_ONCE(), a weaker form > > of barrier(), which could be more informative.)" I do not see a compelling > > reason to change what is notoriously fragile code with respect to barriers. > > > > > Fair enough. The "apparently" wording is lame without assembly code evidence. > I do not have such a s390 machine, so I have cc''ed the original > author incorporating this barrier(), > hopefully he could help test this. > > By "could be more informative" I mean a READ_ONCE has explanatory > effect by its name, instead of > a bare barrier() without more inline comment for why. > > As I said the retry code logic is effectively the same as > > while (lock_ptr = READ_ONCE(q->lock_ptr)) { > spin_lock(lock_ptr) > if (unlikely(lock_ptr != q->lock_ptr)) { > spin_unlock(lock_ptr); > } > > ... > } > > in which case READ_ONCE could perfectly fit, in that it will make > compiler only read it once in every testing condition, > which will eliminate the original problem that commit e91467ecd1ef > addressed, though assembly code proof is needed. > > Actually, the quotation I argued in previous mail could be used again > here, from memory-barriers.txt: > > (*) The compiler is within its rights to merge successive loads from > the same variable. Such merging can cause the compiler to "optimize" > the following code: > > while (tmp = a) > do_something_with(tmp); > > into the following code, which, although in some sense legitimate > for single-threaded code, is almost certainly not what the developer > intended: > > if (tmp = a) > for (;;) > do_something_with(tmp); > > Use READ_ONCE() to prevent the compiler from doing this to you: > > while (tmp = READ_ONCE(a)) > do_something_with(tmp); > > > I might be wrong, so I have cc'ed Paul, and Peter, I wish they give comment :-)
I believe you are correct with respect to the retry and while condition being an appropriate place for the application of READ_ONCE. The question is why is this preferred to the existing barrier()? I suggest:
While barrier() is a fairly brute force general application of a compiler barrier, READ_ONCE() is very specific, it targets only operations dealing with the specified variable. As such, its application both clearly identifies the volatile variable and frees the compiler to make optimizations a more general barrier would forbid.
> > > > As for #2... > > > >> 2. For the second problem I memtioned, yes, it is theoretical, and > >> it is also due to q->lock_ptr can change between > >> the test of nullness of q->lock_ptr and the lock on q->lock_ptr. > >> > >> the code is > >> > >> retry: > >> lock_ptr = q->lock_ptr; > >> if (lock_ptr != 0) { > >> spin_lock(lock_ptr) > >> if (unlikely(lock_ptr != q->lock_ptr)) { > >> spin_unlock(lock_ptr); > >> goto retry; > >> } > >> ... > >> } > >> > >> which is effectively the same as : > >> > >> while (lock_ptr = q->lock_ptr) { > >> spin_lock(lock_ptr) > >> if (unlikely(lock_ptr != q->lock_ptr)) { > >> spin_unlock(lock_ptr); > >> goto retry; > >> } > >> ... > >> } > >> > >> This might cause the compiler load the q->lock_ptr once and use it > >> many times, quoted from
Which is already covered by the barrier() in place today as a more general compiler barrier.
Your argument is then simply that READ_ONCE is a more specific solution to the problem.
> >> memory-barriers.txt: > >> > >> > >> (*) The compiler is within its rights to reload a variable, for example, > >> in cases where high register pressure prevents the compiler from > >> keeping all data of interest in registers. The compiler might > >> therefore optimize the variable 'tmp' out of our previous example: > >> > >> while (tmp = a) > >> do_something_with(tmp); > >> > >> This could result in the following code, which is perfectly safe in > >> single-threaded code, but can be fatal in concurrent code: > >> > >> while (a) > >> do_something_with(a); > >> > >> For example, the optimized version of this code could result in > >> passing a zero to do_something_with() in the case where the variable > >> a was modified by some other CPU between the "while" statement and > >> the call to do_something_with(). > >> > >> Again, use READ_ONCE() to prevent the compiler from doing this: > >> > >> while (tmp = READ_ONCE(a)) > >> do_something_with(tmp); > >> > > > > OK, so this is really the meat of the argument for the patch. You are looking to > > add a compiler barrier instead of a CPU memory barrier. This should be what your
I was incorrect in my statement here. barrier() is a general compiler barrier and READ_ONCE is a targeted compiler barrier (impacting only the operations dealing with the specified variable). This helps the argument considerably.
> > commit message is focused on and it should provide compelling evidence to > > justify risking the change. > > > > Compelling evidence for a compiler barrier would be a disassembly of the code > > block before and after, demonstrating the compiler generating broken code and the > > compiler generating correct code. > > > > In addition to this, however, I would want to see a strong convincing argument > > that the READ_ONCE volatile cast is sufficient to cover the original case which > > motivated the addition of the barrier() (not "apparently" and "could be"). > > As for #2, this is pure theoretical induction from this quotation, I > do have no convincing argument > and again I would like Paul to help clarify this. > > > Thanks very much! > > > Regards, > Jianyu Zhan >
-- Darren Hart Intel Open Source Technology Center
| |