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

Davide Libenzi
2009-06-23T01:24:19Z

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). Lets call these kvmfd, and vbusfd,
> >> respectively. For something like an interrupt, we would hook the point
> >> where the PCI-MSI interrupt is assigned, and would do the following:
> >>
> >> gsi = kvm_irq_route_gsi();
> >> fd = eventfd(0, 0);
> >> ioctl(kvmfd, KVM_IRQFD_ASSIGN, {fd, gsi});
> >> ioctl(vbusfd, VBUS_SHMSIGNAL_ASSIGN, {sigid, fd});
> >>
> >> So userspace orchestrated the assignment of this one eventfd to a KVM
> >> consumer, and a VBUS producer. The two subsystems do not care about the
> >> details of the other side of the link, per se. VBUS just knows that it
> >> can eventfd_signal() its memory region to tell whomever is listening
> >> that it changed. Likewise, KVM just knows to inject "gsi" when it gets
> >> signalled. You could equally have given "fd" to a userspace thread for
> >> either producer or consumer roles, or any other combination.
> >>
> >> If we were doing PCI-passthough, substitute the last SHMSIGNAL_ASSIGN
> >> ioctl call with some PCI_PASSTHROUGH_ASSIGN verb and you get the idea.
> >>
> >> The important thing is that once this is established, userspace doesn't
> >> necessarily care about the fd anymore. So now the question is: do we
> >> keep it around for other things? Do we keep it around because we don't
> >> want KVM to see the POLLHUP, or do we address the "release" code so that
> >> it works even if userspace issued close(fd) at this point. I am not
> >> sure what the answer is, but this is the scenario we are concerned with
> >> in this thread. In the example above, vbus is free to produce events on
> >> its eventfd until it gets a SHMSIGNAL_DEASSIGN request.
> >>
> >
> > I see.
> > The thing remains, that in order to reliably handle generic
> > producer/consumer scenarios you'd need a reference counting similar to
> > pipes, where the notion of producer and consumer is very well defined.
> >
>
> I see your point.
>
> Well, I think the more important thing here is that we address the
> races, and add support for DEASSIGN. We can do both of those things
> with any of the patches that you and I have been kicking around. So
> what I propose is that we move forward with whatever patch you bless as
> proper for now. This producer-release issue is pretty minor in the
> grand scheme of things. We can always just have userspace hold the fd.

Is it a real problem? Can it be decently handled on the KVM side?
Reason I'm asking, is that I wouldn't want to change the interface, only
to find it unsuitable a few weeks later.

> I can either take in the last one you sent, or it sounds like you wanted
> to possibly do another round of cleanup? Whatever the case may be, let
> me know and we can coordinate with Andrew/Avi on what tree to pull it
> into. It sounds like riding in kvm.git is the perhaps the most logical.

Yes, I'd like to drop the pollcb bits (that you can implement KVM-side),
at least.
And yes, going kvm.git is probably the best path.

- Davide