summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaolo Bonzini <pbonzini@redhat.com>2016-03-22 14:27:14 -0700
committerLinus Torvalds <torvalds@linux-foundation.org>2016-03-22 15:36:02 -0700
commita484c3dd9426e1f450c7ed8ad5a2002ea1b309ea (patch)
treeeb9b33089aa7b7cd4b33c52c1422caba3b6be32e
parent0335695dfa4df01edff5bb102b9a82a0668ee51e (diff)
eventfd: document lockless access in eventfd_poll
Since commit e22553e2a25e ("eventfd: don't take the spinlock in eventfd_poll", 2015-02-17), eventfd is reading ctx->count outside ctx->wqh.lock. However, things aren't as simple as the read barrier in eventfd_poll would suggest. In fact, the read barrier, besides lacking a comment, is not paired in any obvious manner with another read barrier, and it is pointless because it is sitting between a write (deep in poll_wait) and the read of ctx->count. The read barrier is acting just as a compiler barrier, for which we can use READ_ONCE instead. This is what the code change in this patch does. The documentation change is just as important, however. The question, posed by Andrea Arcangeli, is then why the thing is safe on architectures where spin_unlock does not imply a store-load memory barrier. The answer is that it's safe because writes of ctx->count use the same lock as poll_wait, and hence an acquire barrier implicit in poll_wait provides the necessary synchronization between eventfd_poll and callers of wake_up_locked_poll. This is sort of mentioned in the commit message with respect to eventfd_ctx_read ("eventfd_read is similar, it will do a single decrement with the lock held") but it applies to all other callers too. It's tricky enough that it should be documented in the code. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Andrea Arcangeli <aarcange@redhat.com> Cc: Chris Mason <clm@fb.com> Cc: Davide Libenzi <davidel@xmailserver.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
-rw-r--r--fs/eventfd.c42
1 files changed, 40 insertions, 2 deletions
diff --git a/fs/eventfd.c b/fs/eventfd.c
index ed70cf9fdc7b..1231cd1999d8 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -121,8 +121,46 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait)
u64 count;
poll_wait(file, &ctx->wqh, wait);
- smp_rmb();
- count = ctx->count;
+
+ /*
+ * All writes to ctx->count occur within ctx->wqh.lock. This read
+ * can be done outside ctx->wqh.lock because we know that poll_wait
+ * takes that lock (through add_wait_queue) if our caller will sleep.
+ *
+ * The read _can_ therefore seep into add_wait_queue's critical
+ * section, but cannot move above it! add_wait_queue's spin_lock acts
+ * as an acquire barrier and ensures that the read be ordered properly
+ * against the writes. The following CAN happen and is safe:
+ *
+ * poll write
+ * ----------------- ------------
+ * lock ctx->wqh.lock (in poll_wait)
+ * count = ctx->count
+ * __add_wait_queue
+ * unlock ctx->wqh.lock
+ * lock ctx->qwh.lock
+ * ctx->count += n
+ * if (waitqueue_active)
+ * wake_up_locked_poll
+ * unlock ctx->qwh.lock
+ * eventfd_poll returns 0
+ *
+ * but the following, which would miss a wakeup, cannot happen:
+ *
+ * poll write
+ * ----------------- ------------
+ * count = ctx->count (INVALID!)
+ * lock ctx->qwh.lock
+ * ctx->count += n
+ * **waitqueue_active is false**
+ * **no wake_up_locked_poll!**
+ * unlock ctx->qwh.lock
+ * lock ctx->wqh.lock (in poll_wait)
+ * __add_wait_queue
+ * unlock ctx->wqh.lock
+ * eventfd_poll returns 0
+ */
+ count = READ_ONCE(ctx->count);
if (count > 0)
events |= POLLIN;