Messages in this thread Patch in this message | | | From | Jianyu Zhan <> | Subject | [PATCH v3] futex: replace bare barrier() with more lightweight READ_ONCE() | Date | Mon, 7 Mar 2016 09:32:24 +0800 |
| |
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> --- 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
| |