On Wed, 16 Mar 2016 13:49:10 +0100 Paolo Bonzini <pbonz...@redhat.com> wrote:
> On 16/03/2016 13:42, Cornelia Huck wrote: > > On Wed, 16 Mar 2016 13:32:59 +0100 > > Paolo Bonzini <pbonz...@redhat.com> wrote: > > > >> On 16/03/2016 13:22, Cornelia Huck wrote: > >>>>> Yeah, it doesn't help that the functions are underdocumented (as in the > >>>>> "assign" parameter above). > >>> My understanding is: > >>> > >>> - 'assign': set up a new notifier (true) or disable it (false) > >>> - 'set_handler': use our handler (true) or have it handled elsewhere > >>> (false) > >> > >> Right. So if we're setting up a new notifier in > >> virtio_queue_aio_set_host_notifier_handler, virtio_pci_stop_ioeventfd > >> should > > ... not call virtio_queue_host_notifier_read. This needs to be cascaded into virtio_queue_set_host_notifier_fd_handler(), I guess. > > >>>>> I don't think the ->set_host_notifiers() api really allows for this. > >>>> > >>>> I think it does, assign is the last argument to k->set_host_notifier(). > >>> > >>> This depends on whether we want 'assign' to clean up any old notifiers > >>> before setting up new ones. I think we want different behaviour for > >>> dataplane and vhost. > >> > >> I think dataplane and vhost are the same. > >> > >> The question is whether ioeventfd=off,vhost=on or > >> ioeventfd=off,dataplane=on are valid combinations; I think they aren't. > > > > We should disallow that even temporary, then. > > > > (This would imply that we need to drop the _stop_ioeventfd() call in > > ->set_host_notifier(), correct?) > > No, it would not. ioeventfd=off,vhost=on would mean: "when vhost is > off, use vCPU thread notification". *confused* Is ioeventfd=off supposed to mean "don't talk to the kernel, do everything in qemu"? > > When turning on vhost you'd still stop ioeventfd (i.e. stop processing > the virtqueue in QEMU's main iothread), but you don't need to do > anything to the event notifier. vhost will pick it up and work on the > virtqueue if necessary. Likewise for dataplane. So "disassociate the handler and switch over to the new one"? > > >> If they aren't, it should be okay to remove the > >> virtio_queue_host_notifier_read call in > >> virtio_queue_set_host_notifier_fd_handler and > >> virtio_queue_aio_set_host_notifier_handler. That's because a handler > >> for the notifier will always be set _somewhere_. It could be the usual > >> ioeventfd handler, the vhost handler or the dataplane handler, but one > >> will be there. > > > > It should; but we probably need to do a final read when we stop the > > ioeventfd. > > I was thinking of handing the final read directly to the next guy who > polls the event notifier instead. So, when called from vhost or > dataplane, virtio_pci_stop_ioeventfd would use > assign=true/set_handler=false ("a new notifier is going to be set up by > the caller"). OK, then we'd need to pass a new parameter for this. > > The host notifier API unfortunately is full of indirections. I'm not > sure how many of them are actually necessary. Oh yes, it's very hard to follow, especially with not-very-well defined parameters.