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

[Gregory Haskins]

Michael S. Tsirkin wrote:
> 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.
>   

Which?  Leaving the code as is, or a hybrid?
> 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?
>   
-ENOPARSE

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

I agree, and that is why I am advocating this POLLHUP solution.  It was
only this other way to begin with because the technology didn't exist
until Davide showed me the light.

Problem with your request is that I already looked into what is
essentially a bi-directional reference problem (for a different reason)
when I started the POLLHUP series.  Its messy to do this in a way that
doesn't negatively impact the fast path (introducing locking, etc) or
make my head explode making sure it doesn't race.  Afaict, we would need
to solve this problem to do what you are proposing (patches welcome).

If this hybrid decoupled-deassign + unified-close is indeed an important
feature set, I suggest that we still consider this POLLHUP series for
inclusion, and then someone can re-introduce DEASSIGN support in the
future as a CAP bit extension.  That way we at least get the desirable
close() properties that we both seem in favor of, and get this advanced
use case when we need it (and can figure out the locking design).


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

Well, thats just it.  Today, you *do* create the eventfd to bundle with
the gsi (take a look at my userspace patches..I posted some new ones
today).   The eventfd is returned after you specify the GSI via
kvm_irqfd().  Thats why I am arguing that it is natural for close() to
terminate the assignment.  To me, this is consistent with other
interfaces that return an fd (socket(), open(), etc).

That said, if we are going to support your proposal going forward, we
should probably change libkvm::kvm_irqfd() to take the fd as a
parameter, instead of returning it.


>  Who knows what else this eventfd descriptor
> can be used for?
>   

Perhaps, but you are exceeding the original design specifications of
irqfd as it is, so we can't really predict what limitations it will have
for other esoteric uses either.   While I think the masking use case is
bogus, I guess I don't specifically object to what you are proposing
w.r.t. gsi remapping.  Fwiw, this use-case you are presenting would have
been more useful if proposed during the 10+ revisions of design review
on irqfd instead of now, but se la vie.

-Greg

Attachment: signature.asc
Description: OpenPGP digital signature