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

[Michael S. Tsirkin]

On Sun, Jul 01, 2012 at 09:06:20PM +1000, Alexey Kardashevskiy wrote:
> QEMU from qemu.org/master (not qemu-kvm), linux host 3.4, ppc64.
> 
> Shortly the problem is in the host kernel: closing a file in one thread does 
> not interrupt select() waiting on the same file description in another thread.
> 
> Longer story is:
> I'll use VFIO as an example as I hit this when I was debugging it but VFIO 
> itself has nothing to do with the issue.
> 
> The bug looks like: I start the guest with MSI-capable PCI card passed via 
> VFIO. The guest does dhclient from .bashrc and this dhclient does not receive 
> anything till I press any key.
> I did not see it for a while as I always start the guest and then typed 
> "dhclient" manually in the guest console.
> 
> What happens:
> VFIO initializes INTx eventfd via event_notifier_init() (returns fd=XX) and 
> qemu_set_fd_handler() before the guest starts. Then QEMU starts the guest and 
> enters a loop is os_host_main_loop_wait() which stays in select() until 
> something happens.
> 
> >From the host kernel prospective, the XX fd was allocated, struct file* (P1) 
> >with eventfd specific private_data allocated and initialized. select() added 
> >a file refcounter (called get_file() in __pollwait()) and started waiting on 
> >file* P1 but not on fd's number XX (what is mmm weird but ok).
> 
> The guest starts and tries to initialize MSI for the PCI card passed through. 
> The guest does PCI config write and this write creates a second thread in 
> QEMU.

Why does this create a thread btw?

> In this thread QEMU-VFIO closes fd XX which makes the host kernel release 
> fd=XX, release the corresponding file* P1 (fput() in filp_close()) but this 
> does not free this P1 as there is select() waiting on it.

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

> 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