Messages in this thread | | | Date | Tue, 8 Mar 2016 03:26:31 -0800 | From | Darren Hart <> | Subject | Re: [PATCH v3] futex: replace bare barrier() with more lightweight READ_ONCE() |
| |
On Mon, Mar 07, 2016 at 09:32:24AM +0800, Jianyu Zhan wrote: > Commit e91467ecd1ef ("bug in futex unqueue_me") introduces a barrier() > in unqueue_me(), to address below problem. > > The scenario is like this: > > ==================== > original code: > > 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; > } > ... > } > > ==================== > It was observed that compiler generates code that is equivalent to: > > retry: > if (q->lock_ptr != 0) { > spin_lock(q->lock_ptr) > if (unlikely(lock_ptr != q->lock_ptr)) { > spin_unlock(lock_ptr); > goto retry; > } > ... > } > > since q->lock_ptr might change between the test of non-nullness and spin_lock(), > the double load will cause trouble. So that commit uses a barrier() to prevent this. > > This patch replaces this bare barrier() with a READ_ONCE(). > > The reasons are: > > 1) READ_ONCE() is a more weak form of barrier() that affect only the specific > accesses, while barrier() is a more general compiler level memroy barrier. > READ_ONCE() was not available at that time when that patch was written. > > 2) READ_ONCE() which could be more informative by its name, while a bare barrier() > without comment leads to quite a bit of perplexity. > > Assembly code before(barrier version) and after this patch(READ_ONCE version) are the same: > > ==================== > Before(barrier version): > > unqueue_me(): > linux/kernel/futex.c:1930 > 1df6: 4c 8b bd 28 ff ff ff mov -0xd8(%rbp),%r15 > linux/kernel/futex.c:1932 > 1dfd: 4d 85 ff test %r15,%r15 > 1e00: 0f 84 5c 01 00 00 je 1f62 <futex_wait+0x292> > spin_lock(): > linux/include/linux/spinlock.h:302 > 1e06: 4c 89 ff mov %r15,%rdi > 1e09: e8 00 00 00 00 callq 1e0e <futex_wait+0x13e> > > ==================== > After(READ_ONCE version): > > __read_once_size(): > linux/include/linux/compiler.h:218 > 1df6: 4c 8b bd 28 ff ff ff mov -0xd8(%rbp),%r15 > unqueue_me(): > linux/kernel/futex.c:1935 > 1dfd: 4d 85 ff test %r15,%r15 > 1e00: 0f 84 5c 01 00 00 je 1f62 <futex_wait+0x292> > spin_lock(): > linux/include/linux/spinlock.h:302 > 1e06: 4c 89 ff mov %r15,%rdi > 1e09: e8 00 00 00 00 callq 1e0e <futex_wait+0x13e> > > Code size is also the same. > > Many thanks to Darren Hart <dvhart@infradead.org> for reviewing and > suggestion. > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Jianyu Zhan <nasa4836@gmail.com>
Thanks for sticking with this Jianyu.
Reviewed-by: Darren Hart <dvhart@linux.intel.com>
> --- > kernel/futex.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index 5d6ce64..25dbfed 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -1927,8 +1927,12 @@ static int unqueue_me(struct futex_q *q) > > /* In the common case we don't take the spinlock, which is nice. */ > retry: > - lock_ptr = q->lock_ptr; > - barrier(); > + /* > + * q->lock_ptr can change between this read and the following spin_lock. > + * Use READ_ONCE to forbid the compiler from reloading q->lock_ptr and > + * optimizing lock_ptr out of the logic below. > + */ > + lock_ptr = READ_ONCE(q->lock_ptr); > if (lock_ptr != NULL) { > spin_lock(lock_ptr); > /* > -- > 2.4.3 > >
-- Darren Hart Intel Open Source Technology Center
| |