Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-26 Thread Michael S. Tsirkin
On Thu, Jun 25, 2009 at 02:41:09PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Thu, Jun 25, 2009 at 01:32:22PM -0400, Gregory Haskins wrote: Please take a close look at it and consider for merging, if you would. With all the incremental patching, I kind of lost

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-25 Thread Davide Libenzi
On Thu, 25 Jun 2009, Rusty Russell wrote: On Thu, 25 Jun 2009 08:15:11 am Davide Libenzi wrote: Some components would like to know if userspace dropped the fd, and take proper action accordingly (release resources, drop module instances, etc...). Like to know? Possibly. Need to

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-25 Thread Michael S. Tsirkin
On Thu, Jun 25, 2009 at 01:32:22PM -0400, Gregory Haskins wrote: Please take a close look at it and consider for merging, if you would. With all the incremental patching, I kind of lost track of what the complete file would look like. Is there a git tree I could pull? -- MST -- To unsubscribe

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-25 Thread Gregory Haskins
Michael S. Tsirkin wrote: On Thu, Jun 25, 2009 at 01:32:22PM -0400, Gregory Haskins wrote: Please take a close look at it and consider for merging, if you would. With all the incremental patching, I kind of lost track of what the complete file would look like. Is there a git tree I

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-25 Thread Rusty Russell
On Thu, 25 Jun 2009 08:15:11 am Davide Libenzi wrote: On Wed, 24 Jun 2009, Rusty Russell wrote: On Tue, 23 Jun 2009 03:33:22 am Davide Libenzi wrote: What you're doing there, is setting up a kernel-to-kernel (since userspace only role is to create the eventfd) communication, using a

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-24 Thread Davide Libenzi
On Wed, 24 Jun 2009, Rusty Russell wrote: On Tue, 23 Jun 2009 03:33:22 am Davide Libenzi wrote: What you're doing there, is setting up a kernel-to-kernel (since userspace only role is to create the eventfd) communication, using a file* as accessory. That IMO is plain wrong. The most

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-23 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: To be honest, I am not sure. I would guess its not a *huge* deal, though it was obviously enough of a concern to at least discuss it. I can definitely say that I think the other issues which are being fixed are substantially more important. Ok

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-23 Thread Gregory Haskins
Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: To be honest, I am not sure. I would guess its not a *huge* deal, though it was obviously enough of a concern to at least discuss it. I can definitely say that I think the other issues which are being fixed are

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-23 Thread Davide Libenzi
On Tue, 23 Jun 2009, Rusty Russell wrote: The first 'struct eventfd_ctx;' line is not required. Will repost dropping that. - Davide -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-23 Thread Davide Libenzi
On Tue, 23 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: To be honest, I am not sure. I would guess its not a *huge* deal, though it was obviously enough of a concern to at least discuss it. I can definitely say that I think

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-23 Thread Gregory Haskins
Davide Libenzi wrote: On Tue, 23 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: To be honest, I am not sure. I would guess its not a *huge* deal, though it was obviously enough of a concern to at least discuss it. I

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-23 Thread Michael S. Tsirkin
On Tue, Jun 23, 2009 at 07:35:00AM -0700, Davide Libenzi wrote: On Tue, 23 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: To be honest, I am not sure. I would guess its not a *huge* deal, though it was obviously enough of

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Davide Libenzi wrote: On Sun, 21 Jun 2009, Gregory Haskins wrote: This looks great, Davide. I am fairly certain I can now solve the races and even implement Michael's DEASSIGN feature with this patch in place. I will actually fire it up tomorrow when I am back in the office and give it

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Sun, 21 Jun 2009, Gregory Haskins wrote: This looks great, Davide. I am fairly certain I can now solve the races and even implement Michael's DEASSIGN feature with this patch in place. I will actually fire it

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Sun, 21 Jun 2009, Gregory Haskins wrote: This looks great, Davide. I am fairly certain I can now solve the races and even implement Michael's DEASSIGN feature with this patch in

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: I am probably confused or perhaps have the wrong terminology, but isnt that ok. I am concerned about the consumer (the guy getting the POLLINs) to be able to detect POLLHUP when the last producer (f_ops-write() from userspace, eventfd_signal() from

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: I am probably confused or perhaps have the wrong terminology, but isnt that ok. I am concerned about the consumer (the guy getting the POLLINs) to be able to detect POLLHUP when the last producer (f_ops-write() from

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: The general thesis is for decoupling of the two subsystems. In order to do this, you need some form of polymorphism and an intermediate handle mechanism which is userspace friendly. File-descriptors already fit this role neatly, with the int fd

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:03:22AM -0700, Davide Libenzi wrote: In your case of kernel-to-kernel scenario, why would you need eventfd at all, if userspace role in that model is simply to create it? That's not 100% true. We have a mode where

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Michael S. Tsirkin
On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote: A file* based kernel-to-kernel interface is rather wrong IMO. But eventfd_ctx should work fine. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Davide Libenzi wrote: On Mon, 22 Jun 2009, Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:03:22AM -0700, Davide Libenzi wrote: In your case of kernel-to-kernel scenario, why would you need eventfd at all, if userspace role in that model is simply to create it? That's

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote: A file* based kernel-to-kernel interface is rather wrong IMO. But eventfd_ctx should work fine. Yeah, and I guess we can always just say that qemu can't close the fd or something. Seems

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote: A file* based kernel-to-kernel interface is rather wrong IMO. But eventfd_ctx should work fine. Yeah, and I guess we can always just

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Mon, 22 Jun 2009, Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:03:22AM -0700, Davide Libenzi wrote: In your case of kernel-to-kernel scenario, why would you need eventfd at all, if userspace role

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote: A file* based kernel-to-kernel interface is rather wrong IMO. But eventfd_ctx should work fine.

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Michael S. Tsirkin
On Mon, Jun 22, 2009 at 03:26:41PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Mon, Jun 22, 2009 at 11:51:42AM -0700, Davide Libenzi wrote: A file* based kernel-to-kernel interface is rather wrong IMO. But eventfd_ctx should work fine. Yeah, and I guess

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: So up in userspace, the vbus pci-device would have an open reference to the kvm guest (derived from /dev/kvm) and an open reference to a vbus (derived from /dev/vbus). Lets call these kvmfd, and vbusfd, respectively. For something like an

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Gregory Haskins
Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: So up in userspace, the vbus pci-device would have an open reference to the kvm guest (derived from /dev/kvm) and an open reference to a vbus (derived from /dev/vbus). Lets call these kvmfd, and vbusfd, respectively. For

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Davide Libenzi
On Mon, 22 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Mon, 22 Jun 2009, Gregory Haskins wrote: So up in userspace, the vbus pci-device would have an open reference to the kvm guest (derived from /dev/kvm) and an open reference to a vbus (derived from /dev/vbus).

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-22 Thread Rusty Russell
On Mon, 22 Jun 2009 09:24:20 am Davide Libenzi wrote: I actually ended up exposing the eventfd context anyway, since IMO is a better option instead of holding references to the eventfd file (that makes VFS people uneasy). lguest is a special case since we don't let them close the fds, except

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-21 Thread Davide Libenzi
On Sat, 20 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: How about the one below? Maybe with an interface that can be undone w/out a file* :) This is another

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-21 Thread Gregory Haskins
Davide Libenzi wrote: On Sat, 20 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: How about the one below? Maybe with an interface that can

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-21 Thread Davide Libenzi
On Sun, 21 Jun 2009, Gregory Haskins wrote: This looks great, Davide. I am fairly certain I can now solve the races and even implement Michael's DEASSIGN feature with this patch in place. I will actually fire it up tomorrow when I am back in the office and give it a spin, but I do not spy

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-20 Thread Davide Libenzi
On Fri, 19 Jun 2009, Gregory Haskins wrote: In the POLLIN event, you schedule_work(irqfd-inject) and there are no races there AFAICS (you basically do not care of anything eventfd memory related at all). For POLLHUP, you do: spin_lock(irqfd-slock); if (irqfd-wqh)

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-20 Thread Davide Libenzi
On Sat, 20 Jun 2009, Davide Libenzi wrote: How about the one below? Maybe with an interface that can be undone w/out a file* :) - Davide --- fs/eventfd.c| 34 +- include/linux/eventfd.h |8 2 files changed, 41 insertions(+), 1

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-20 Thread Davide Libenzi
On Sat, 20 Jun 2009, Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: How about the one below? Maybe with an interface that can be undone w/out a file* :) This is another alternative, based on a low-carb diet of your notifier patch. Same concept of de-coupling VFS

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-20 Thread Gregory Haskins
Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: In the POLLIN event, you schedule_work(irqfd-inject) and there are no races there AFAICS (you basically do not care of anything eventfd memory related at all). For POLLHUP, you do: spin_lock(irqfd-slock); if

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-20 Thread Gregory Haskins
Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: On Sat, 20 Jun 2009, Davide Libenzi wrote: How about the one below? Maybe with an interface that can be undone w/out a file* :) This is another alternative, based on a low-carb diet of your notifier

[PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Gregory Haskins
eventfd currently emits a POLLHUP wakeup on f_ops-release() to generate a notifier-release() callback. This lets notification clients know if the eventfd is about to go away and is very useful particularly for in-kernel clients. However, as it stands today it is not possible to use the

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Davide Libenzi
On Fri, 19 Jun 2009, Gregory Haskins wrote: eventfd currently emits a POLLHUP wakeup on f_ops-release() to generate a notifier-release() callback. This lets notification clients know if the eventfd is about to go away and is very useful particularly for in-kernel clients. However, as it

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Gregory Haskins
Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: eventfd currently emits a POLLHUP wakeup on f_ops-release() to generate a notifier-release() callback. This lets notification clients know if the eventfd is about to go away and is very useful particularly for in-kernel

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Davide Libenzi
On Fri, 19 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: eventfd currently emits a POLLHUP wakeup on f_ops-release() to generate a notifier-release() callback. This lets notification clients know if the eventfd is about to go

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Gregory Haskins
Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: eventfd currently emits a POLLHUP wakeup on f_ops-release() to generate a notifier-release() callback. This lets notification clients

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Davide Libenzi
On Fri, 19 Jun 2009, Gregory Haskins wrote: I am fairly confident it is not that simple after having thought about this issue over the last few days. But I've been wrong in the past. Propose a patch and I will review it for races/correctness, if you like. Perhaps a combination of that plus

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Davide Libenzi
On Fri, 19 Jun 2009, Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: I am fairly confident it is not that simple after having thought about this issue over the last few days. But I've been wrong in the past. Propose a patch and I will review it for races/correctness,

Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

2009-06-19 Thread Gregory Haskins
Davide Libenzi wrote: On Fri, 19 Jun 2009, Davide Libenzi wrote: On Fri, 19 Jun 2009, Gregory Haskins wrote: I am fairly confident it is not that simple after having thought about this issue over the last few days. But I've been wrong in the past. Propose a patch and I will