Re: [PATCH 0/2] eventfd: new EFD_STATE flag

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Davide Libenzi <davidel@xmailserver.org>
Cc: Avi Kivity <avi@redhat.com>, kvm@vger.kernel.org
Subject: Re: [PATCH 0/2] eventfd: new EFD_STATE flag
Date: Thu, 7 Jan 2010 08:45:03 +0200	[thread overview]
Message-ID: <20100107064503.GA10379@redhat.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1001061555390.17695@makko.or.mcafeemobile.com>

On Wed, Jan 06, 2010 at 03:59:11PM -0800, Davide Libenzi wrote:
> On Thu, 7 Jan 2010, Michael S. Tsirkin wrote:
> 
> > On Wed, Jan 06, 2010 at 02:46:06PM -0800, Davide Libenzi wrote:
> > > On Thu, 7 Jan 2010, Michael S. Tsirkin wrote:
> > > 
> > > > > On top of creating an interface which requires a lock, which noone can get 
> > > > > from the interface itself, since it's not exposed.
> > > > 
> > > > I think here's how KVM gets it: the way it does is by calling poll with
> > > > our own poll table, then in poll_queue_proc we get wait queue pointer,
> > > > and we use the wait queue. Lock is in there :)
> > > 
> > > Yes, I know you are called locked, but it does not lead to a clean 
> > > interface.
> > 
> > True.
> > 
> > > > > I could split the two and have a locked one, and an unlocked one, but that 
> > > > > looks shitty too (for the above reason).
> > > > 
> > > > Yes, this will work. Thanks!
> > > 
> > > This is a lot more complex than I thought. The wakeup code is already 
> > > enumerating the list, and doing a wakeup might trigger a secondary 
> > > enumeration/recursion.
> > 
> > For KVM what you describe is I think is not a problem: we check wake type
> > and ignore POLLOUT events.
> 
> You seem to think in one dimension only ;)
> The interface needs to be stable for everyone.
> 
> 
> 
> > Maybe yes. I'll think it over and get back to you. Thanks!
> 
> Let me know.
> 
> 
> 
> - Davide

OK. What I think we need is a way to remove ourselves from the eventfd
wait queue and clear the counter atomically.

We currently do
        remove_wait_queue(irqfd->wqh, &irqfd->wait);
where wqh saves the eventfd wait queue head.

If we do this before proposed eventfd_read_ctx, we can lose events.
If we do this after, we can get spurious events.

An unlocked read is one way to fix this.

-- 
MST