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

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 gi

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
Davide Libenzi wrote: > 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...)

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. Ne

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

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

2009-06-24 Thread Davide Libenzi
On Tue, 23 Jun 2009, Davide Libenzi wrote: > On Tue, 23 Jun 2009, Rusty Russell wrote: > > > The first 'struct eventfd_ctx;' line is not required. > > Will repost dropping that. Almost forgot. While fixing lg.h to drop the fwd declaration, I noticed there's another one ;) - Davide --- a/dr

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

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

2009-06-23 Thread Rusty Russell
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 sensible is that userspace can use these fds; an 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

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

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

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 http://vger.ker

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 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-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-22 Thread Gregory Haskins
Davide Libenzi wrote: > 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) a

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

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, >> respecti

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 inter

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. > > > >

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 ev

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

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

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.

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? >>>

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 i

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

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

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: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 userspace is the producer and/or consumer (migration

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

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(

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

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 act

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

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 sp

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? > >

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* :)

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

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(i

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 r

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 dele

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 (irqf

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 patc

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/correc

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 p

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

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

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 >>

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 i

[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 notifica