| Message ID | 4FF02F2C.30006@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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
On 01/07/12 22:43, Michael S. Tsirkin wrote: > 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? It is the thread where the guest is running I guess (?). This is the backtrace of this second thread: (gdb) bt #0 vfio_enable_msi (vdev=0x110200f0) at /home/aik/qemu-impreza/hw/ppc/../vfio_pci.c:699 #1 0x000000001036c948 in vfio_pci_write_config (pdev=0x110200f0, addr=0xd2, val=0x81, len=0x2) at /home/aik/qemu-impreza/hw/ppc/../vfio_pci.c:979 #2 0x00000000101b1388 in pci_host_config_write_common (pci_dev=0x110200f0, addr=0xd2, limit=0x100, val=0x81, len=0x2) at /home/aik/qemu-impreza/hw/pci_host.c:54 #3 0x000000001035d70c in finish_write_pci_config (spapr=0x10efa860, buid=0x2, addr=0xd2, size=0x2, val=0x81, rets=0xd64758) at /home/aik/qemu-impreza/hw/ppc/../spapr_pci.c:170 #4 0x000000001035d874 in rtas_ibm_write_pci_config (spapr=0x10efa860, token=0x2010, nargs=0x5, args=0xd64744, nret=0x1, rets=0xd64758) at /home/aik/qemu-impreza/hw/ppc/../spapr_pci.c:194 #5 0x0000000010360bcc in spapr_rtas_call (spapr=0x10efa860, token=0x2010, nargs=0x5, args=0xd64744, nret=0x1, rets=0xd64758) at /home/aik/qemu-impreza/hw/ppc/../spapr_rtas.c:218 #6 0x0000000010358150 in h_rtas (env=0xfffb733f040, spapr=0x10efa860, opcode=0xf000, args=0xfffb7fea030) at /home/aik/qemu-impreza/hw/ppc/../spapr_hcall.c:560 #7 0x0000000010358c4c in spapr_hypercall (env=0xfffb733f040, opcode=0xf000, args=0xfffb7fea030) at /home/aik/qemu-impreza/hw/ppc/../spapr_hcall.c:734 #8 0x00000000103dab2c in kvm_arch_handle_exit (env=0xfffb733f040, run=0xfffb7fea000) at /home/aik/qemu-impreza/target-ppc/kvm.c:769 #9 0x00000000103a8a94 in kvm_cpu_exec (env=0xfffb733f040) at /home/aik/qemu-impreza/kvm-all.c:1536 #10 0x00000000102ede24 in qemu_kvm_cpu_thread_fn (arg=0xfffb733f040) at /home/aik/qemu-impreza/cpus.c:752 #11 0x00000fffb7ab19f0 in .start_thread () from /lib64/libpthread.so.0 #12 0x00000fffb7706774 in .__clone () from /lib64/libc.so.6 > >> 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. QEMU does not literally remove it but it marks the event as deleted and actually removes it much later from qemu_iohandler_poll() which is called after os_host_main_loop_wait(). Removal is done by calling qemu_set_fd_handler(fd, NULL, NULL, vdev); But even if it did remove it, what would it change? > >> 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
Il 01/07/2012 13:06, Alexey Kardashevskiy ha scritto: > 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. This sounds like a missing qemu_notify_event(). There was a recent thread on a similar problem with block/iscsi.c. Paolo
On 01/07/12 23:32, Paolo Bonzini wrote: > Il 01/07/2012 13:06, Alexey Kardashevskiy ha scritto: >> 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. > > This sounds like a missing qemu_notify_event(). There was a recent > thread on a similar problem with block/iscsi.c. Oh, right, that helps too when place in qemu_set_fd_handler2().
> 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
> >> 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; > >> } That probably wants to be a pthread_kill targetted at the main loop. > >> +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); > >> +} > >> + Won't that conflict with the business in coroutine-sigaltstack.c ? Hrm... looking at it, it looks like it will save/restore the handler, so that should be good. Still, one might want to wrap that into something, like qemu_wake_main_loop(); Cheers, Ben. > >> int main_loop_init(void) > >> { > >> int ret; > >> > >> + sigusr2_init(); > >> + > >> qemu_mutex_lock_iothread(); > >> ret = qemu_signal_init(); > >> if (ret) { > >> -- > >> 1.7.10 > >
On 02/07/12 09:07, Benjamin Herrenschmidt wrote: >>>> 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; >>>> } > > That probably wants to be a pthread_kill targetted at the main loop. > >>>> +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); >>>> +} >>>> + > > Won't that conflict with the business in coroutine-sigaltstack.c ? The code which touches SIGUSR2 does not compile on power. > Hrm... looking at it, it looks like it will save/restore the handler, > so that should be good. > > Still, one might want to wrap that into something, like > qemu_wake_main_loop(); I already posted another patch with qemu_notify_event() in this mail thread later :) > > Cheers, > Ben. > >>>> int main_loop_init(void) >>>> { >>>> int ret; >>>> >>>> + sigusr2_init(); >>>> + >>>> qemu_mutex_lock_iothread(); >>>> ret = qemu_signal_init(); >>>> if (ret) { >>>> -- >>>> 1.7.10 >> >> > >
Hi, As Paolo said, I hit this with block/iscsi.c a few months back. Then about a month back I recall something that looks alkmost identical to this hit the IDE driver on the KVM list. ( At least the symptoms looked just like my symptoms, and the KVM guys managed to bisect it down to the exact same patch that I did that would expose the bug in block.iscsi.c, namely the lack of calling qemu_notify_event() ) If we have hit a problem 3 times recently due to developers not realizing the importance of calling qemu_notify_event(), maybe the API is suboptimal. Would it be possible to change the set-event-handlers functions to automatically call qemu_notify_event() when the descriptos change? To eliminate the need to call this function at all ? regards ronnie sahlberg On Mon, Jul 2, 2012 at 10:06 AM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 02/07/12 09:07, Benjamin Herrenschmidt wrote: >>>>> 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; >>>>> } >> >> That probably wants to be a pthread_kill targetted at the main loop. >> >>>>> +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); >>>>> +} >>>>> + >> >> Won't that conflict with the business in coroutine-sigaltstack.c ? > > The code which touches SIGUSR2 does not compile on power. > >> Hrm... looking at it, it looks like it will save/restore the handler, >> so that should be good. >> >> Still, one might want to wrap that into something, like >> qemu_wake_main_loop(); > > > I already posted another patch with qemu_notify_event() in this mail thread later :) > > >> >> Cheers, >> Ben. >> >>>>> int main_loop_init(void) >>>>> { >>>>> int ret; >>>>> >>>>> + sigusr2_init(); >>>>> + >>>>> qemu_mutex_lock_iothread(); >>>>> ret = qemu_signal_init(); >>>>> if (ret) { >>>>> -- >>>>> 1.7.10 >>> >>> >> >> > > > -- > Alexey >
On Mon, 2012-07-02 at 10:06 +1000, Alexey Kardashevskiy wrote: > > Won't that conflict with the business in coroutine-sigaltstack.c ? > > The code which touches SIGUSR2 does not compile on power. Oh, we don't get the altstack coroutine stuff ? interesting... > > Hrm... looking at it, it looks like it will save/restore the handler, > > so that should be good. > > > > Still, one might want to wrap that into something, like > > qemu_wake_main_loop(); > > > I already posted another patch with qemu_notify_event() in this mail thread later :) Ok. Thanks. Cheers, Ben. > > > > > Cheers, > > Ben. > > > >>>> int main_loop_init(void) > >>>> { > >>>> int ret; > >>>> > >>>> + sigusr2_init(); > >>>> + > >>>> qemu_mutex_lock_iothread(); > >>>> ret = qemu_signal_init(); > >>>> if (ret) { > >>>> -- > >>>> 1.7.10 > >> > >> > > > > > >
On Mon, 2012-07-02 at 10:42 +1000, ronnie sahlberg wrote: > > Would it be possible to change the set-event-handlers functions to > automatically call qemu_notify_event() when the descriptos change? > To eliminate the need to call this function at all ? That definitely sounds like the right thing to do. Cheers, Ben.
On Mon, 2012-07-02 at 10:06 +1000, Alexey Kardashevskiy wrote: > I already posted another patch with qemu_notify_event() in this mail > thread later :) Yup, just saw that, for some reason I got dropped from the CC list. BTW. I told you there must be an existing mechanism for that :-) Cheers, Ben.
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) {