lkml.org 
[lkml]   [2016]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] futex: replace bare barrier() with a READ_ONCE()
On Tue, Mar 01, 2016 at 08:44:48AM +0800, Jianyu Zhan wrote:
> On Tue, Mar 1, 2016 at 6:22 AM, Darren Hart <dvhart@infradead.org> wrote:
> > You've mention it causes problems a few times, but you do not specify what
> > problem it causes or how it manifests.
> >
> > Is this a theoretical bug, or have you experienced a failure case. If so, how
> > did this manifest? Do you have a reproducer we could add to the futex testsuite
> > in the kernel selftests?
>
>
> 1. For the first problem I memtioned, it is a bug that described in
> commit e91467ecd1ef.
>
> The scenario is like:
>
> lock_ptr = q->lock_ptr
> if (lock_ptr != 0) spin_lock(lock_ptr)
>
> and the compiler generates code that is equivalent to :
>
> if (q->lock_ptr != 0) spin_lock(q->lock_ptr)
>
> The problem is q->lock_ptr can change between the test of nullness of
> q->lock_ptr and the lock on q->lock_ptr
> So a barrier() is inserted into the load of q->lock_ptr and the test
> of nullness, to avoid the pointer aliasing.
>
> Apparently, a READ_ONCE() fits the goal here.


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.

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

Thanks,

--
Darren Hart
Intel Open Source Technology Center

\
 
 \ /
  Last update: 2016-03-02 03:21    [W:0.431 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site