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 <dvh...@infradead.org> for reviewing and
suggestion.
Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
Signed-off-by: Jianyu Zhan <nasa4...@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
