Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()

[Michael S. Tsirkin]

On Tue, Jun 02, 2009 at 12:34:12PM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Jun 02, 2009 at 12:14:15PM -0400, Gregory Haskins wrote:
> >   
> >> Michael S. Tsirkin wrote:
> >>     
> >>> On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote:
> >>>   
> >>>       
> >>>> (Applies to kvm.git/master:25deed73)
> >>>>
> >>>> Please see the header for 2/2 for a description.  This patch series has
> >>>> been fully tested and appears to be working correctly.  I have it as an RFC
> >>>> for now because it needs Davide's official submission/SOB for patch 1/2, and
> >>>> it should get some eyeballs/acks on my SRCU usage before going in.
> >>>>
> >>>> I will submit the updated irqfd userspace which eschews the deassign() verb
> >>>> since we can now just use the close(fd) method alone.  I will also address
> >>>> the userspace review comments from Avi.
> >>>>     
> >>>>         
> >>> We are not killing the deassign though, do we?
> >>>   
> >>>       
> >> Yes, it is not needed any more now that we have proper
> >> release-notification from eventfd.
> >>
> >>     
> >>> It's good to have that option e.g. for when we pass
> >>> the fd to another process.
> >>>   
> >>>       
> >> Passing the fd to another app should up the underlying file reference
> >> count.  If the producer app wants to "deassign" it simply calls
> >> close(fd) (as opposed to today where it calls DEASSIGN+close), but the
> >> reference count will allow the consuming app to leave the eventfd's file
> >> open.  Or am I misunderstanding you?
> >>
> >> -Greg
> >>
> >>
> >>     
> >
> > I think we want to keep supporting the deassign ioctl. This, even though
> > close overlaps with it functionally somewhat.
> >
> > This allows qemu to pass eventfd to another process/device, and then
> > block/unblock interrupts as seen by that process by
> > assigning/deassigning irq to it. This is much easier and lightweight
> > than asking another process to close the fd and passing another fd
> > later.
> >
> >   
> Perhaps, but if that is the case we should just ignore this series and
> continue with the DEASSIGN+close methodology since it already provides
> that separation. Trying to do a hybrid is just messy.

As I see it, it's the least evil.

One-way ioctl operations on file descriptors are messier still. What's
another example of an ioctl that can't be undone without closing the fd?
And having close not clean up the state unless you do an ioctl first is
very messy IMO - I don't think you'll find any such examples in kernel.

> But in any case, I think that approach is flawed.  DEASSIGN shouldn't be
> used as a mask in my opinion, and we shouldn't be reassigning a
> channel's meaning under the covers like that.  If this is in fact a
> valid use case, we should have a separate "GSI_MASK" type operation that
> is independent of irqfd.
>  Likewise, we really should pass a new fd if
> the gsi-routing is changing.  Today there is a tight coupling of
> fd-to-gsi, and I think that makes sense to continue this association.
> 
> -Greg
> 

I'm not arguing that this use-case is not theoretical. Just that if you
don't create the fd to connect to GSI, you shouln't ask the user to
destroy it to disconnect. Who knows what else this eventfd descriptor
can be used for?

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/