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

* [Qemu-devel] QEMU question: is eventfd not thread safe?
@ 2012-07-01 11:06 Alexey Kardashevskiy
  2012-07-01 12:43 ` Michael S. Tsirkin
  2012-07-01 13:32 ` Paolo Bonzini
  0 siblings, 2 replies; 28+ messages in thread
From: Alexey Kardashevskiy @ 2012-07-01 11:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alex Williamson, David Gibson, mst

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread