Message ID | 1343793936-28000-1-git-send-email-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
On 08/01/2012 07:05 AM, David Gibson wrote: > From: Alexey Kardashevskiy <aik@ozlabs.ru> > > QEMU uses IO handlers to run select() in the main loop. > The handlers list is managed by qemu_set_fd_handler() helper > which works fine when called from the main thread as it is > called when select() is not waiting. > > However IO handlers list can be changed in the thread other than > the main one doing os_host_main_loop_wait(), for example, as a result > of a hypercall which changes PCI config space (VFIO on POWER is the case) > and enables/disabled MSI/MSIX which creates eventfd handles. > As the main loop should be waiting on the newly created eventfds, > it has to be restarted. > > The patch adds the qemu_notify_event() call to interrupt select() > to make main_loop() restart select() with the updated IO handlers > list. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > iohandler.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/iohandler.c b/iohandler.c > index 3c74de6..dea4355 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; > + qemu_notify_event(); > } > return 0; > } Perhaps it's better to do this unconditionally (on the delete path too) so that removals are processed without delay and we don't have closed fds hanging around in select().
On Mon, Aug 06, 2012 at 05:05:57PM +0300, Avi Kivity wrote: > On 08/01/2012 07:05 AM, David Gibson wrote: > > From: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > QEMU uses IO handlers to run select() in the main loop. > > The handlers list is managed by qemu_set_fd_handler() helper > > which works fine when called from the main thread as it is > > called when select() is not waiting. > > > > However IO handlers list can be changed in the thread other than > > the main one doing os_host_main_loop_wait(), for example, as a result > > of a hypercall which changes PCI config space (VFIO on POWER is the case) > > and enables/disabled MSI/MSIX which creates eventfd handles. > > As the main loop should be waiting on the newly created eventfds, > > it has to be restarted. > > > > The patch adds the qemu_notify_event() call to interrupt select() > > to make main_loop() restart select() with the updated IO handlers > > list. > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > iohandler.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/iohandler.c b/iohandler.c > > index 3c74de6..dea4355 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; > > + qemu_notify_event(); > > } > > return 0; > > } > > Perhaps it's better to do this unconditionally (on the delete path too) > so that removals are processed without delay and we don't have closed > fds hanging around in select(). Well, I understand that Alexey discussed the patch with Paolo and Michael Tsirkin, and this was the preferred approach for now. Since obviously no events will happen on deleted fds, removing them from the select() is not really urgent. This is a very straightforward fix for a real problem, can we please just merge the damn thing.
Il 07/08/2012 06:02, David Gibson ha scritto: >> Perhaps it's better to do this unconditionally (on the delete path too) >> so that removals are processed without delay and we don't have closed >> fds hanging around in select(). > > Well, I understand that Alexey discussed the patch with Paolo and > Michael Tsirkin, and this was the preferred approach for now. Since > obviously no events will happen on deleted fds, removing them from the > select() is not really urgent. Avi is not speaking about deleted fds, but about existing fds whose handlers are temporarily removed. I don't see it as a blocker for merging the patch because we've never observed it (and it's unlikely, because temporary removal of handlers typically occurs from within a handler, not from another thread). Paolo
diff --git a/iohandler.c b/iohandler.c index 3c74de6..dea4355 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; + qemu_notify_event(); } return 0; }