Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1347819

Re: [PATCH] futex: replace bare barrier() with a READ_ONCE()

Show all headers | View raw

On Wed, Mar 2, 2016 at 2:39 PM, Darren Hart <dvhart@infradead.org> wrote:
> 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.
>
>

Yep,  beside the informative point,  the more specifics of READ_ONCE
over barrier
is what I meant  "lightweight", I missed emphasizing this point.
Thanks for pointing it.

I will respin this patch to reflect this.

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

Yep. And after re-thinking, I am now less convinced in this second
argument, since
it involves a comparison of q->lock_ptr in the loop body,  so this may
defeat any attempts
that compilers try to optimize the load out of the loop,  even without
a READ_ONCE().

But I will also incorporate this in the second submission for review .



Regards,
Jianyu Zhan

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH] futex:  replace bare barrier() with a READ_ONCE() Jianyu Zhan <nasa4836@gmail.com> - 2016-02-29 12:50 +0100
  Re: [PATCH] futex:  replace bare barrier() with a READ_ONCE() Darren Hart <dvhart@infradead.org> - 2016-02-29 23:30 +0100
    Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() Jianyu Zhan <nasa4836@gmail.com> - 2016-03-01 01:50 +0100
      Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() Darren Hart <dvhart@infradead.org> - 2016-03-02 02:40 +0100
        Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() Jianyu Zhan <nasa4836@gmail.com> - 2016-03-02 03:40 +0100
          Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() Darren Hart <dvhart@infradead.org> - 2016-03-02 07:40 +0100
            Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() Jianyu Zhan <nasa4836@gmail.com> - 2016-03-02 08:20 +0100

csiph-web