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

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Gleb Natapov <gleb@redhat.com>
Cc: Davide Libenzi <davidel@xmailserver.org>,
	Avi Kivity <avi@redhat.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH 0/2] eventfd: new EFD_STATE flag
Date: Mon, 11 Jan 2010 11:41:25 +0200	[thread overview]
Message-ID: <20100111094125.GF1642@redhat.com> (raw)
In-Reply-To: <20100111093616.GE7549@redhat.com>

On Mon, Jan 11, 2010 at 11:36:16AM +0200, Gleb Natapov wrote:
> On Mon, Jan 11, 2010 at 11:19:19AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 11, 2010 at 11:08:38AM +0200, Gleb Natapov wrote:
> > > On Mon, Jan 11, 2010 at 11:02:27AM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Jan 11, 2010 at 11:01:27AM +0200, Gleb Natapov wrote:
> > > > > On Thu, Jan 07, 2010 at 12:36:01PM +0200, Michael S. Tsirkin wrote:
> > > > > > Or if I do it the other way:
> > > > > > 		remove_wait_queue(irqfd->wqh, &irqfd->wait);
> > > > > > 	->
> > > > > > 		eventfd_read_ctx(irqfd->eventfd, &ucnt);
> > > > > > 
> > > > > > now, if device signals eventfd at point marked by ->,
> > > > > > it will not be sent but counter will be cleared,
> > > > > > so we will loose a message.
> > > > > > 
> > > > > May be I am missing something here, but why doing it like that is a
> > > > > problem? Event delivery races with interrupt masking so making masking
> > > > > succeed before event delivery is OK. Event generation is asynchronous
> > > > > anyway and could have happened jiffy latter anyway.
> > > > > 
> > > > > --
> > > > > 			Gleb.
> > > > 
> > > > No, event generation would only trigger a single interrupt.  This race
> > > > generates two interrupts for a single event.  This can never happen with
> > > > real hardware.  eventfd_ctx_remove_wait_queue would solve this problem.
> > > > 
> > > In quoted test above you are saying that "we will loose a message". So
> > > how does it generates two interrupts when we loose a message?
> > >   
> > > --
> > > 			Gleb.
> > 
> > 
> > Right, sorry. I think what you miss is the fact that this is done during
> > interrupt vector masking/unmasking, so events signalled while eventfd is not
> > assigned to interrupt must not be lost, they should be pending and
> > delivered later when interrupt vector is unmasked, that is when
> > eventfd is reassigned to an interrupt.
> > 
> Is this how MSI works? If interrupt is triggered while it is masked it
> is reasserted after unmasking? This is certainly no true for interrupt
> masking on irq chip level.

Yes.

> > So this implementation really loses an interrupt:
> > 
> > 	remove_wait_queue(irqfd->wqh, &irqfd->wait);
> > 		-> at this point vector is masked: we are not polling
> > 		   eventfd anymore, event triggered at this point should cause interrupt
> > 		   after vector is unmasked, but the only thing is causes
> > 		   is counter increment in eventfd.
> > 
> > 	eventfd_read_ctx(irqfd->eventfd, &ucnt);
> > 		-> the above call would clear the counter, so
> > 		   we won't get an interrupt when vector is later
> > 		   unmasked.
> > 
> Don't you going to use ucnt to set interrupt status bit? Can't you
> re-trigger the interrupt after unmasking if status bit is set?

Yes, this is what Davidel suggested originally.  There's nowhere to save
the ucnt though because event is being deasserted.

> --
> 			Gleb.