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 00:29:20 +0200	[thread overview]
Message-ID: <20100106222920.GB29200@redhat.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1001061310050.17695@makko.or.mcafeemobile.com>

On Wed, Jan 06, 2010 at 01:17:02PM -0800, Davide Libenzi wrote:
> On Wed, 6 Jan 2010, Michael S. Tsirkin wrote:
> 
> > I tried to explain, no?  That patch was taking wait queue spinlock and
> > was assuming that eventfd_read_ctx is called from a task that can block.
> > KVM attaches its own poller so this is not a good fit for us.  Hope this
> > clarifies.
> > 
> > > and yet again you managed to add an underscore at the beginning of the
> > > API name, in an API set where there are no leading underscores.  Even
> > > if KVM does that for its own reasons, you should be able to fit your
> > > naming "style" to the interface you're adding your droplets into.  I
> > > will post an eventfd_read_ctx() to Andrew ASAP.
> > > 
> > > 
> > > 
> > > - Davide
> > 
> > Sorry about the underscore - it's easy to fix but I won't bore you with
> > more patch revisions before I understand what makes you unhappy.
> > 
> > Before KVM starts creating workqueues and doing schedule_work calls just
> > to work around the API, can we discuss this please? I am not hung on my
> > patch, but could we have it work so that we can call eventfd_read_ctx
> > from wait queue callback directly?
> 
> The "read" needs to wake possible OUT waiters, *cannot* be done your way.

Right. Now I see. Thanks!

> 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 :)

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

> I thought you *already* do your own stuff from a work queue, why would you 
> need to read from IRQ context?
> 
> 
> 
> - Davide

This was just work around while interrupt delivery was using mutex
locks, so we had a workqueue to work around that limitation.  Now that
it has been all switched to RCU, we'll be getting getting rid of this.

-- 
MST