eventfd: allow atomic read and waitqueue remove
PATCH v2 1/3 eventfd: allow atomic read and waitqueue remove
This is a backport of commit: 03db343a6320f780937078433fa7d8da955e6fce modified in a way that introduces some code duplication on the one hand, but reduces the risk of regressing existing eventfd users on the other hand.
KVM needs a wait to atomically remove themselves from the eventfd ->poll() wait queue head, in order to handle correctly their IRQfd deassign operation.
This patch introduces such API, plus a way to read an eventfd from its context.
---
Avi, Davidel, how about only including the following part for -stable then? Reason is, I still would like to be able to use irqfd there, and getting spurious interrupts 100% of times unmask is done isn't a very good idea IMO ...
fs/eventfd.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/eventfd.h | 9 +++++++++ 2 files changed, 44 insertions(+), 0 deletions(-)
fs/eventfd.c +35 -0
@@ -135,6 +135,41 @@ static unsigned int eventfd_poll(struct file *file, poll_table *wait)return events; } +static void eventfd_ctx_do_read (struct eventfd_ctx *ctx,__u64 *cnt) +{ + *cnt = (ctx->flags &EFD_SEMAPHORE ) ?1 : ctx->count; + ctx->count -= *cnt; +} + +/** +* eventfd_ctx_remove_wait_queue - Read the current counter and removes wait queue. +* @ctx: [in] Pointer to eventfd context. +* @wait: [in] Wait queue to be removed. +* @cnt: [out] Pointer to the 64bit conter value. +* +* Returns zero if successful, or the following error codes: +* +* -EAGAIN : The operation would have blocked. +* +* This is used to atomically remove a wait queue entry from the eventfd wait +* queue head, and read/reset the counter value. +*/ +int eventfd_ctx_remove_wait_queue (struct eventfd_ctx *ctx,wait_queue_t *wait, +__u64 *cnt) +{ +unsigned long flags; + +spin_lock_irqsave (&ctx->wqh.lock, flags); +eventfd_ctx_do_read (ctx, cnt); +__remove_wait_queue (&ctx->wqh, wait); +if (*cnt !=0 &&waitqueue_active (&ctx->wqh)) +wake_up_locked_poll (&ctx->wqh,POLLOUT ); +spin_unlock_irqrestore (&ctx->wqh.lock, flags); + +return *cnt !=0 ?0 : -EAGAIN ; +} +EXPORT_SYMBOL_GPL (eventfd_ctx_remove_wait_queue); +static ssize_t eventfd_read (struct file *file,char __user *buf,size_t count,loff_t *ppos) {
include/linux/eventfd.h +9 -0
@@ -10,6 +10,7 @@#include <linux/fcntl.h> #include <linux/file.h> +#include <linux/wait.h> /* * CAREFUL: Check include/asm-generic/fcntl.h when defining
@@ -34,6 +35,8 @@ struct file *eventfd_fget(int fd);struct eventfd_ctx *eventfd_ctx_fdget (int fd);struct eventfd_ctx *eventfd_ctx_fileget (struct file *file);int eventfd_signal (struct eventfd_ctx *ctx,int n); +int eventfd_ctx_remove_wait_queue (struct eventfd_ctx *ctx,wait_queue_t *wait, +__u64 *cnt);#else /* CONFIG_EVENTFD */
@@ -61,6 +64,12 @@ static inline void eventfd_ctx_put(struct eventfd_ctx *ctx) } +static inline int eventfd_ctx_remove_wait_queue (struct eventfd_ctx *ctx, +wait_queue_t *wait,__u64 *cnt) +{ +return -ENOSYS ; +} +#endif #endif /* _LINUX_EVENTFD_H */
Signature · 1 line
1.6.6.144.g5c3af
Quoted 12 lines · Michael S. Tsirkin
This is a backport of commit:
03db343a6320f780937078433fa7d8da955e6fcemodified in a way that introduces some code duplication on the one hand, but reduces the risk of regressing existing eventfd users on the other hand.KVM needs a wait to atomically remove themselves from the eventfd ->poll() wait queue head, in order to handle correctly their IRQfd deassign operation.
This patch introduces such API, plus a way to read an eventfd from its context.
Quoted 7 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Avi, Davidel, how about only including the following part for -stable then? Reason is, I still would like to be able to use irqfd there, and getting spurious interrupts 100% of times unmask is done isn't a very good idea IMO ...
It's the same thing. Unless there are *real* problems in KVM due to the spurious ints, I still think this is .33 material.
- Davide
Quoted 18 lines · Davide Libenzi
Quoted 15 lines · Michael S. Tsirkin
This is a backport of commit:
03db343a6320f780937078433fa7d8da955e6fcemodified in a way that introduces some code duplication on the one hand, but reduces the risk of regressing existing eventfd users on the other hand.KVM needs a wait to atomically remove themselves from the eventfd ->poll() wait queue head, in order to handle correctly their IRQfd deassign operation.
This patch introduces such API, plus a way to read an eventfd from its context.
Signed-off-by Michael S. Tsirkin<[email protected]>---
Quoted 8 lines
Quoted 5 lines
Avi, Davidel, how about only including the following part for -stable then? Reason is, I still would like to be able to use irqfd there, and getting spurious interrupts 100% of times unmask is done isn't a very good idea IMO ...
It's the same thing. Unless there are *real* problems in KVM due to the spurious ints, I still think this is .33 material.
I agree.
But I think we can solve this in another way in .32: we can clear the eventfd from irqfd->inject work, which is in process context. The new stuff is only needed for lockless clearing, no?
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 3 lines · Avi Kivity
But I think we can solve this in another way in .32: we can clear the eventfd from
irqfd->injectwork, which is in process context. The new stuff is only needed for lockless clearing, no?
I meant atomic clearing, when we inject interrupts from the irqfd atomic context.
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 28 lines · Avi Kivity
Quoted 18 lines · Davide Libenzi
Quoted 15 lines · Michael S. Tsirkin
This is a backport of commit:
03db343a6320f780937078433fa7d8da955e6fcemodified in a way that introduces some code duplication on the one hand, but reduces the risk of regressing existing eventfd users on the other hand.KVM needs a wait to atomically remove themselves from the eventfd ->poll() wait queue head, in order to handle correctly their IRQfd deassign operation.
This patch introduces such API, plus a way to read an eventfd from its context.
Signed-off-by Michael S. Tsirkin<[email protected]>---
Quoted 8 lines
Quoted 5 lines
Avi, Davidel, how about only including the following part for -stable then? Reason is, I still would like to be able to use irqfd there, and getting spurious interrupts 100% of times unmask is done isn't a very good idea IMO ...
It's the same thing. Unless there are *real* problems in KVM due to the spurious ints, I still think this is .33 material.
Quoted 5 lines
I agree.
But I think we can solve this in another way in .32: we can clear the eventfd from
irqfd->injectwork, which is in process context. The new stuff is only needed for lockless clearing, no?
No, AFAIK there's no way to clear the counter from kernel without this patch.
Quoted 2 lines
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 10 lines · Michael S. Tsirkin
Quoted 7 lines
I agree.
But I think we can solve this in another way in .32: we can clear the eventfd from
irqfd->injectwork, which is in process context. The new stuff is only needed for lockless clearing, no?No, AFAIK there's no way to clear the counter from kernel without this patch.
Can't you read from the file?
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Michael S. Tsirkin
Quoted 7 lines
I agree.
But I think we can solve this in another way in .32: we can clear the eventfd from
irqfd->injectwork, which is in process context. The new stuff is only needed for lockless clearing, no?
Quoted 5 lines
Quoted 3 lines
No, AFAIK there's no way to clear the counter from kernel without this patch.
Can't you read from the file?
IMO no, the read could block.
Quoted 2 lines
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 5 lines · Michael S. Tsirkin
Quoted 3 lines
Can't you read from the file?
IMO no, the read could block.
But you're in process context. An eventfd never blocks.
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 8 lines · Avi Kivity
Quoted 5 lines · Michael S. Tsirkin
Quoted 3 lines
Can't you read from the file?
IMO no, the read could block.
But you're in process context. An eventfd never blocks.
Yes it blocks if counter is 0. And we don't know
it's not 0 unless we read :) catch-22.
Quoted 2 lines
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines
But you're in process context. An eventfd never blocks.
Yes it blocks if counter is 0. And we don't know
it's not 0 unless we read :) catch-22.
Ah yes, I forgot.
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 8 lines · Avi Kivity
Quoted 4 lines · Michael S. Tsirkin
Quoted 2 lines
But you're in process context. An eventfd never blocks.
Yes it blocks if counter is 0. And we don't know
it's not 0 unless we read :) catch-22.Ah yes, I forgot.
Well, you can poll it and then read it... this introduces a new race (if userspace does a read in parallel) but it's limited to kvm and buggy userspace.
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 7 lines · Avi Kivity
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Michael S. Tsirkin
Quoted 2 lines
But you're in process context. An eventfd never blocks.
Yes it blocks if counter is 0. And we don't know
it's not 0 unless we read :) catch-22.
Quoted 6 lines
Quoted 2 lines
Ah yes, I forgot.
Well, you can poll it and then read it... this introduces a new race (if userspace does a read in parallel) but it's limited to kvm and buggy userspace.
I would rather not require that userspace never reads this fd. You are right that it does not now, but adding this as requirement looks like exporting an implementation bug to userspace.
Signature · 1 line
MST
Quoted 15 lines · Michael S. Tsirkin
Quoted 13 lines · Avi Kivity
Quoted 11 lines · Avi Kivity
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines
But you're in process context. An eventfd never blocks.
Yes it blocks if counter is 0. And we don't know
it's not 0 unless we read :) catch-22.Ah yes, I forgot.
Quoted 8 lines
Quoted 4 lines
Well, you can poll it and then read it... this introduces a new race (if userspace does a read in parallel) but it's limited to kvm and buggy userspace.
I would rather not require that userspace never reads this fd. You are right that it does not now, but adding this as requirement looks like exporting an implementation bug to userspace.
Well, I don't like risking 2.6.32 non-kvm users for a bug that doesn't happen in practice now.
After it gets some time in 2.6.33, we can backport it to 2.6.32 (since that will be maintained long term).
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 8 lines · Avi Kivity
Quoted 5 lines · Michael S. Tsirkin
Quoted 3 lines
Can't you read from the file?
IMO no, the read could block.
But you're in process context. An eventfd never blocks.
Can you control the eventfd flags? Because if yes, O_NONBLOCK will never block.
- Davide
Quoted 6 lines · Davide Libenzi
Quoted 4 lines · Avi Kivity
Quoted 3 lines · Michael S. Tsirkin
Quoted 3 lines
Can't you read from the file?
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
IMO no, the read could block.
But you're in process context. An eventfd never blocks.
Can you control the eventfd flags? Because if yes,
O_NONBLOCKwill never block.
Userspace can but kvm can't.
Quoted 3 lines
- Davide