| Message ID | 1477850917-1214-13-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/30/2016 10:23 PM, Michael S. Tsirkin wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > Now that there is not anymore a switch from the generic ioeventfd handler > to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is > always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd > does nothing in this case. Move the invocation to vhost.c, which is the > only place that needs it. > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/virtio/virtio-bus.h | 6 ------ > hw/virtio/vhost.c | 3 +++ > hw/virtio/virtio-bus.c | 23 ++++++++--------------- > 3 files changed, 11 insertions(+), 21 deletions(-) This breaks vhost-net for s390/kvm after rebooting the guest. (ping fails and ifconfig shows no packets is TXed) Any idea? > > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index af6b5c4..cbdf745 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -94,12 +94,6 @@ struct VirtioBusState { > BusState parent_obj; > > /* > - * Set if the default ioeventfd handlers are disabled by vhost > - * or dataplane. > - */ > - bool ioeventfd_disabled; > - > - /* > * Set if ioeventfd has been started. > */ > bool ioeventfd_started; > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 501a5f4..131f164 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1196,6 +1196,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > goto fail; > } > > + virtio_device_stop_ioeventfd(vdev); > for (i = 0; i < hdev->nvqs; ++i) { > r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, > true); > @@ -1215,6 +1216,7 @@ fail_vq: > } > assert (e >= 0); > } > + virtio_device_start_ioeventfd(vdev); > fail: > return r; > } > @@ -1237,6 +1239,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > } > assert (r >= 0); > } > + virtio_device_start_ioeventfd(vdev); > } > > /* Test and clear event pending status. > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index a619445..b0e4544 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -190,7 +190,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus) > if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) { > return -ENOSYS; > } > - if (bus->ioeventfd_started || bus->ioeventfd_disabled) { > + if (bus->ioeventfd_started) { > return 0; > } > r = vdc->start_ioeventfd(vdev); > @@ -226,8 +226,8 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus) > } > > /* > - * This function switches from/to the generic ioeventfd handler. > - * assign==false means 'use generic ioeventfd handler'. > + * This function switches ioeventfd on/off in the device. > + * The caller must set or clear the handlers for the EventNotifier. > */ > int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > { > @@ -237,19 +237,12 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > if (!k->ioeventfd_assign) { > return -ENOSYS; > } > - bus->ioeventfd_disabled = assign; > if (assign) { > - /* > - * Stop using the generic ioeventfd, we are doing eventfd handling > - * ourselves below > - * > - * FIXME: We should just switch the handler and not deassign the > - * ioeventfd. > - * Otherwise, there's a window where we don't have an > - * ioeventfd and we may end up with a notification where > - * we don't expect one. > - */ > - virtio_bus_stop_ioeventfd(bus); > + assert(!bus->ioeventfd_started); > + } else { > + if (!bus->ioeventfd_started) { > + return 0; > + } > } > return set_host_notifier_internal(proxy, bus, n, assign); > } >
On 10/11/2016 15:35, Christian Borntraeger wrote: > On 10/30/2016 10:23 PM, Michael S. Tsirkin wrote: >> From: Paolo Bonzini <pbonzini@redhat.com> >> >> Now that there is not anymore a switch from the generic ioeventfd handler >> to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is >> always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd >> does nothing in this case. Move the invocation to vhost.c, which is the >> only place that needs it. >> >> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> include/hw/virtio/virtio-bus.h | 6 ------ >> hw/virtio/vhost.c | 3 +++ >> hw/virtio/virtio-bus.c | 23 ++++++++--------------- >> 3 files changed, 11 insertions(+), 21 deletions(-) > > This breaks vhost-net for s390/kvm after rebooting the guest. (ping fails and > ifconfig shows no packets is TXed) > > Any idea? Patch from Felipe: [PATCH v2] vhost: Update 'ioeventfd_started' with host notifiers Paolo >> >> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h >> index af6b5c4..cbdf745 100644 >> --- a/include/hw/virtio/virtio-bus.h >> +++ b/include/hw/virtio/virtio-bus.h >> @@ -94,12 +94,6 @@ struct VirtioBusState { >> BusState parent_obj; >> >> /* >> - * Set if the default ioeventfd handlers are disabled by vhost >> - * or dataplane. >> - */ >> - bool ioeventfd_disabled; >> - >> - /* >> * Set if ioeventfd has been started. >> */ >> bool ioeventfd_started; >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 501a5f4..131f164 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1196,6 +1196,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >> goto fail; >> } >> >> + virtio_device_stop_ioeventfd(vdev); >> for (i = 0; i < hdev->nvqs; ++i) { >> r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, >> true); >> @@ -1215,6 +1216,7 @@ fail_vq: >> } >> assert (e >= 0); >> } >> + virtio_device_start_ioeventfd(vdev); >> fail: >> return r; >> } >> @@ -1237,6 +1239,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >> } >> assert (r >= 0); >> } >> + virtio_device_start_ioeventfd(vdev); >> } >> >> /* Test and clear event pending status. >> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >> index a619445..b0e4544 100644 >> --- a/hw/virtio/virtio-bus.c >> +++ b/hw/virtio/virtio-bus.c >> @@ -190,7 +190,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus) >> if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) { >> return -ENOSYS; >> } >> - if (bus->ioeventfd_started || bus->ioeventfd_disabled) { >> + if (bus->ioeventfd_started) { >> return 0; >> } >> r = vdc->start_ioeventfd(vdev); >> @@ -226,8 +226,8 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus) >> } >> >> /* >> - * This function switches from/to the generic ioeventfd handler. >> - * assign==false means 'use generic ioeventfd handler'. >> + * This function switches ioeventfd on/off in the device. >> + * The caller must set or clear the handlers for the EventNotifier. >> */ >> int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) >> { >> @@ -237,19 +237,12 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) >> if (!k->ioeventfd_assign) { >> return -ENOSYS; >> } >> - bus->ioeventfd_disabled = assign; >> if (assign) { >> - /* >> - * Stop using the generic ioeventfd, we are doing eventfd handling >> - * ourselves below >> - * >> - * FIXME: We should just switch the handler and not deassign the >> - * ioeventfd. >> - * Otherwise, there's a window where we don't have an >> - * ioeventfd and we may end up with a notification where >> - * we don't expect one. >> - */ >> - virtio_bus_stop_ioeventfd(bus); >> + assert(!bus->ioeventfd_started); >> + } else { >> + if (!bus->ioeventfd_started) { >> + return 0; >> + } >> } >> return set_host_notifier_internal(proxy, bus, n, assign); >> } >> >
On 11/10/2016 03:38 PM, Paolo Bonzini wrote: > > > On 10/11/2016 15:35, Christian Borntraeger wrote: >> On 10/30/2016 10:23 PM, Michael S. Tsirkin wrote: >>> From: Paolo Bonzini <pbonzini@redhat.com> >>> >>> Now that there is not anymore a switch from the generic ioeventfd handler >>> to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is >>> always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd >>> does nothing in this case. Move the invocation to vhost.c, which is the >>> only place that needs it. >>> >>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> include/hw/virtio/virtio-bus.h | 6 ------ >>> hw/virtio/vhost.c | 3 +++ >>> hw/virtio/virtio-bus.c | 23 ++++++++--------------- >>> 3 files changed, 11 insertions(+), 21 deletions(-) >> >> This breaks vhost-net for s390/kvm after rebooting the guest. (ping fails and >> ifconfig shows no packets is TXed) >> >> Any idea? > > Patch from Felipe: > [PATCH v2] vhost: Update 'ioeventfd_started' with host notifiers > > Paolo Yes, that fixes the issue
On 11/10/2016 03:48 PM, Christian Borntraeger wrote: > On 11/10/2016 03:38 PM, Paolo Bonzini wrote: >> >> >> On 10/11/2016 15:35, Christian Borntraeger wrote: >>> On 10/30/2016 10:23 PM, Michael S. Tsirkin wrote: >>>> From: Paolo Bonzini <pbonzini@redhat.com> >>>> >>>> Now that there is not anymore a switch from the generic ioeventfd handler >>>> to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is >>>> always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd >>>> does nothing in this case. Move the invocation to vhost.c, which is the >>>> only place that needs it. >>>> >>>> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>> --- >>>> include/hw/virtio/virtio-bus.h | 6 ------ >>>> hw/virtio/vhost.c | 3 +++ >>>> hw/virtio/virtio-bus.c | 23 ++++++++--------------- >>>> 3 files changed, 11 insertions(+), 21 deletions(-) >>> >>> This breaks vhost-net for s390/kvm after rebooting the guest. (ping fails and >>> ifconfig shows no packets is TXed) >>> >>> Any idea? >> >> Patch from Felipe: >> [PATCH v2] vhost: Update 'ioeventfd_started' with host notifiers >> >> Paolo > > Yes, that fixes the issue > hmm, not quite. This patch on top of commit 6bbcb76301a72dc80c8d29af13d40bb9a759c9c6 MAINTAINERS: Remove obsolete stable branches still triggers qemu-system-s390x: hw/s390x/virtio-ccw.c:1076: virtio_ccw_remove_irqfd: Assertion `ret == 0' failed. for some cases according to Farhan. There are too many patches floating around, what is the latest assumed to be final patch set? We can then test that. Thanks Christian
On 15/11/2016 09:27, Christian Borntraeger wrote: > hmm, not quite. > This patch on top of > commit 6bbcb76301a72dc80c8d29af13d40bb9a759c9c6 MAINTAINERS: Remove obsolete stable branches > still triggers > > qemu-system-s390x: hw/s390x/virtio-ccw.c:1076: virtio_ccw_remove_irqfd: Assertion `ret == 0' failed. > for some cases according to Farhan. > > There are too many patches floating around, what is the latest assumed to be > final patch set? We can then test that. I'm sending it as soon as I finish testing. But for s390 it should be just one patch, the rest only affects PCI and MMIO (and it had been broken forever for dataplane, so the refactoring did its job by exposing everyone to dataplane bugs, and even before the release :)). Paolo
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h index af6b5c4..cbdf745 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -94,12 +94,6 @@ struct VirtioBusState { BusState parent_obj; /* - * Set if the default ioeventfd handlers are disabled by vhost - * or dataplane. - */ - bool ioeventfd_disabled; - - /* * Set if ioeventfd has been started. */ bool ioeventfd_started; diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 501a5f4..131f164 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1196,6 +1196,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) goto fail; } + virtio_device_stop_ioeventfd(vdev); for (i = 0; i < hdev->nvqs; ++i) { r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, true); @@ -1215,6 +1216,7 @@ fail_vq: } assert (e >= 0); } + virtio_device_start_ioeventfd(vdev); fail: return r; } @@ -1237,6 +1239,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) } assert (r >= 0); } + virtio_device_start_ioeventfd(vdev); } /* Test and clear event pending status. diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index a619445..b0e4544 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -190,7 +190,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus) if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) { return -ENOSYS; } - if (bus->ioeventfd_started || bus->ioeventfd_disabled) { + if (bus->ioeventfd_started) { return 0; } r = vdc->start_ioeventfd(vdev); @@ -226,8 +226,8 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus) } /* - * This function switches from/to the generic ioeventfd handler. - * assign==false means 'use generic ioeventfd handler'. + * This function switches ioeventfd on/off in the device. + * The caller must set or clear the handlers for the EventNotifier. */ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) { @@ -237,19 +237,12 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) if (!k->ioeventfd_assign) { return -ENOSYS; } - bus->ioeventfd_disabled = assign; if (assign) { - /* - * Stop using the generic ioeventfd, we are doing eventfd handling - * ourselves below - * - * FIXME: We should just switch the handler and not deassign the - * ioeventfd. - * Otherwise, there's a window where we don't have an - * ioeventfd and we may end up with a notification where - * we don't expect one. - */ - virtio_bus_stop_ioeventfd(bus); + assert(!bus->ioeventfd_started); + } else { + if (!bus->ioeventfd_started) { + return 0; + } } return set_host_notifier_internal(proxy, bus, n, assign); }