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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Gregory Haskins <ghaskins@novell.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: Sun, 14 Jun 2009 16:20:40 +0300	[thread overview]
Message-ID: <20090614132040.GB10646@redhat.com> (raw)
In-Reply-To: <4A34EC47.5090103@novell.com>

On Sun, Jun 14, 2009 at 08:25:43AM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Thu, Jun 11, 2009 at 04:16:47PM +0300, Michael S. Tsirkin wrote:
> >   
> >>> +
> >>> +	ret = file->f_op->poll(file, &irqfd->pt);
> >>> +	if (ret < 0)
> >>> +		goto fail;
> >>>       
> >
> > Looking at it some more, we have:
> > struct file_operations {
> > ....
> > 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
> >
> > So the comparison above does not seem to make sense:
> > it seems that the return value from poll can not be negative.
> >   
> 
> Indeed.  Will fix.
> > Will the callback be executed if someone did a write to eventfd
> > before we attached it? If no, maybe we should call it here
> > if ret != 0.
> >   
> 
> I do the cleanup in case the callback has been called, but poll() fails
> somewhere internally afterwards.  Perhaps this is not a realistic
> scenario, but it was my motivation for adding the wqh cleanup.

Could it be that poll returns the event mask and you mistake it for
error?

> >   
> >>> +
> >>> +	irqfd->file = file;
> >>> +
> >>> +	mutex_lock(&kvm->lock);
> >>> +	list_add_tail(&irqfd->list, &kvm->irqfds);
> >>> +	mutex_unlock(&kvm->lock);
> >>> +
> >>> +	return 0;
> >>> +
> >>> +fail:
> >>> +	if (irqfd->wqh)
> >>> +		remove_wait_queue(irqfd->wqh, &irqfd->wait);
> >>>       
> >> Why are these 2 lines here? Either we might get a callback even though
> >> poll failed - and then this test without lock is probably racy -
> >> or we can't, and then we can replace the above with BUG_ON(irqfd->wqh).
> >>
> >> Which is it? I think the later ...
> >>
> >>
> >>     
> >>> +
> >>> +	if (file && !IS_ERR(file))
> >>> +		fput(file);
> >>> +
> >>> +	kfree(irqfd);
> >>> +	return ret;
> >>> +}
> >>> +
> >>>       
> 
> 