Re: [KVM PATCH v10] kvm: add support for irqfd

From: Gregory Haskins <ghaskins@novell.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	avi@redhat.com, davidel@xmailserver.org, mtosatti@redhat.com
Subject: Re: [KVM PATCH v10] kvm: add support for irqfd
Date: Wed, 27 May 2009 16:07:23 -0400	[thread overview]
Message-ID: <4A1D9D7B.9080507@novell.com> (raw)
In-Reply-To: <20090527184106.GA18463@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3868 bytes --]

Michael S. Tsirkin wrote:
> On Wed, May 27, 2009 at 10:06:50AM -0400, Gregory Haskins wrote:
>   
>> Michael S. Tsirkin wrote:
>>     
>>> On Wed, May 20, 2009 at 10:30:49AM -0400, Gregory Haskins wrote:
>>>   
>>>       
>>>> +static int
>>>> +kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
>>>> +{
>>>> +	struct _irqfd *irqfd;
>>>> +	struct file *file = NULL;
>>>> +	int ret;
>>>> +
>>>> +	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
>>>> +	if (!irqfd)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	irqfd->kvm = kvm;
>>>> +	irqfd->gsi = gsi;
>>>> +	INIT_LIST_HEAD(&irqfd->list);
>>>> +	INIT_WORK(&irqfd->work, irqfd_inject);
>>>> +
>>>> +	/*
>>>> +	 * Embed the file* lifetime in the irqfd.
>>>> +	 */
>>>> +	file = fget(fd);
>>>> +	if (IS_ERR(file)) {
>>>> +		ret = PTR_ERR(file);
>>>> +		goto fail;
>>>> +	}
>>>>     
>>>>         
>>> So we get a reference to a file, and unless the user is nice to us, it
>>> will only be dropped when kvm char device file is closed?
>>> I think this will deadlock if the fd in question is the open kvm char device.
>>>
>>>
>>>   
>>>       
>> Hmm...I hadn't considered this possibility, though I am not sure if it
>> would cause a deadlock in the pattern you suggest.  It seems more like
>> it would result in, at worst, an extra reference to itself (and thus a
>> leak) rather than a deadlock...
>>
>> I digress.  In either case, perhaps I should s/fget/eventfd_fget to at
>> least limit the type of fd to eventfd.  I was trying to be "slick" by
>> not needing the eventfd_fget() exported, but I am going to need to
>> export it later anyway for iosignalfd, so its probably a moot point.
>>
>> Thanks Michael,
>> -Greg
>>
>>     
>
> This only works as long as eventfd does not do fget on some fd as well.
> Which it does not do now, and may never do - but we create a fragile
> system this way.
>
> I think it's really wrong, fundamentally, to keep a reference to a
> file until another file is closed, unless you are code under fs/.
> We will get nasty circular references sooner or later.
>   

Hmm.. I understand your concern, but I respectfully disagree.

One object referencing another is a natural expression, regardless of
what type they may be.  The fact is that introducing the concept of
irqfd creates a relationship between an eventfd instance and a kvm
instance whether we like it or not, and this relationship needs to be
managed.  It is therefore IMO perfectly natural to express that
relationship with a reference count, and I do not currently see anything
wrong or even particularly fragile about how I've currently done this. 
I'm sure there are other ways, however.  Do you have a particular
suggestion in mind?

> Isn't the real reason we use fd to be able to support the same interface
> on top of both kvm and lguest?
>   

Actually, the reason why we use an fd is to decouple the
interrupt-producing end-point from the KVM core.  Ignoring eventfd in
specific for a moment, one convenient way to do that is with an fd
because it provides a nice, already written/tested handle-to-pointer
translation, and a polymorphic interface (e.g. f_ops).  Choosing to use
eventfd flavored fd's buys us additional advantages in terms of
leveraging already tested f_ops code, and compatibility with an
interface that is designed-for/used-by other established subsystems for
signaling.
> And if so, wouldn't some kind of bus be a better solution?
>   

Ultimately I aim to implement a bus (vbus, specifically) in terms of
irqfd (and iosignalfd, for that matter).  However, the eventfd
interfaces are general purpose and can be used in other areas as well
(for instance, virtio-pci, or the shared-mem driver recently
discussed).  I realize this is probably not the point you were making
here, but fyi.

Regards,
-Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]