Re: [Qemu-devel] QEMU question: is eventfd not thread safe?

[Benjamin Herrenschmidt]

> Doesn't qemu remove an fd handler before closing the fd?
> If not that's the bug right there.

No, it's just marked deleted, removal is deferred. But that doesn't
change the fact that you need to wake up select. Ie. What happens is:

 - eventfd gets you fd #x

 - thread 1 selects() on it which sleeps

 - thread 2 closes (x)

 - thread 2 does eventfd and gets fd #x (same number)

 - new eventfd gets signalled, but doesn't wake up select which
internally is still waiting on the old file descriptor.

The reason for that is that select() (and poll()) internally in
the kernel do a get_file() on the fd (as a result of eventfd->poll
calling poll_wait()) and keeps a reference to the struct file.

So the fd itself can be freed from the table by close, but the
struct file remains around (f_count is elevated), thus the close
does not calls eventfd->release (that only happens on the last
close, ie, after select times out or returns for another reason).

I think this was even overlooked in the design of eventfd itself,
ie, it tries to wakeup all waiters in its release() function but that
will not be called as long as either select or poll is waiting due
to the elevated refcount.

The right solution at this stage if for qemu to kick select() out of its
slumber when it closes the fd, a signal does the job.
 
Cheers,
Ben.

> > eventfd_release() could wake it up but it is called when its refcounter 
> > becomes 0 and this won't happen till select() interrupts.
> > 
> > Doing MSI init stuff, QEMU-VFIO calls the same event_notifier_init() 
> > (returns recycled fd=XX what is correct but confuses) and 
> > qemu_set_fd_handler() which adds a handler but select() does not pick it up.
> > The eventfd() (called by event_notifier_init()) allocates new struct file 
> > *P2 in the kernel, QEMU-VFIO passes this fd=XX to the kernel's VFIO. When 
> > MSI interrupt comes to the host kernel, the VFIO interrupt handler calls 
> > eventfd_signal() on the correct file* P2. However do_select() in the kernel 
> > does not interrupt to deliver eventfd event as it is still waiting on P1 - 
> > nobody interrupted select(). It can only interrupt on stdin events (like 
> > typing keys) and INTx interrupt (which does not happen as MSI is enabled).
> > 
> > So we need to sync eventfd()/close() calls in one thread with select() in 
> > another. Below is the patch which simply sends SIGUSR2 to the main thread 
> > (the sample patch is below). Another solution could be adding new IO 
> > handler to wake select() up. Or to do something with the kernel but I am 
> > not sure what - implementing file_operations::flush for eventfd to do 
> > wakeup did not help and I did not dig any further.
> > 
> > 
> > Does it make sense? What do I miss? What would be the right solution?
> > 
> > 
> > ---
> >  iohandler.c |    1 +
> >  main-loop.c |   17 +++++++++++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/iohandler.c b/iohandler.c
> > index 3c74de6..54f4c48 100644
> > --- a/iohandler.c
> > +++ b/iohandler.c
> > @@ -77,6 +77,7 @@ int qemu_set_fd_handler2(int fd,
> >          ioh->fd_write = fd_write;
> >          ioh->opaque = opaque;
> >          ioh->deleted = 0;
> > +        kill(getpid(), SIGUSR2);
> >      }
> >      return 0;
> >  }
> > diff --git a/main-loop.c b/main-loop.c
> > index eb3b6e6..f65e048 100644
> > --- a/main-loop.c
> > +++ b/main-loop.c
> > @@ -199,10 +199,27 @@ static int qemu_signal_init(void)
> >  }
> >  #endif
> >  
> > +static void sigusr2_print(int signal)
> > +{
> > +}
> > +
> > +static void sigusr2_init(void)
> > +{
> > +    struct sigaction action;
> > +
> > +    memset(&action, 0, sizeof(action));
> > +    sigfillset(&action.sa_mask);
> > +    action.sa_handler = sigusr2_print;
> > +    action.sa_flags = 0;
> > +    sigaction(SIGUSR2, &action, NULL);
> > +}
> > +
> >  int main_loop_init(void)
> >  {
> >      int ret;
> >  
> > +    sigusr2_init();
> > +
> >      qemu_mutex_lock_iothread();
> >      ret = qemu_signal_init();
> >      if (ret) {
> > -- 
> > 1.7.10