| Message ID | 1471544874-26996-3-git-send-email-rkagan@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote: > The protocol for virtio-balloon stats virtqueue doesn't allow more than > one element in the virtqueue. > > So, instead of trying to compensate for guest misbehavior if it sends > new data before the slot has been released by the host, just define the > stats virtqueue length to 1 initially and rely on the generic virtio > code to handle overflows. Looks good to me, one nit inline. > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Ladi Prosek <lprosek@redhat.com> > --- > hw/virtio/virtio-balloon.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 5af429a..0baf4b3 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -262,13 +262,6 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > goto out; > } > > - if (s->stats_vq_elem != NULL) { > - /* This should never happen if the driver follows the spec. */ > - virtqueue_push(vq, s->stats_vq_elem, 0); > - virtio_notify(vdev, vq); > - g_free(s->stats_vq_elem); > - } > - Worth adding an assert? assert(s->stats_vq_elem == NULL); // enforced by one-element stats virtqueue > s->stats_vq_elem = elem; > > /* Initialize the stats to get rid of any stale values. This is only > @@ -443,7 +436,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) > > s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); > s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); > - s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); > + s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats); > > reset_stats(s); > } > -- > 2.7.4 >
On Fri, Aug 19, 2016 at 12:05:29PM +0200, Ladi Prosek wrote: > On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote: > > The protocol for virtio-balloon stats virtqueue doesn't allow more than > > one element in the virtqueue. > > > > So, instead of trying to compensate for guest misbehavior if it sends > > new data before the slot has been released by the host, just define the > > stats virtqueue length to 1 initially and rely on the generic virtio > > code to handle overflows. > > Looks good to me, one nit inline. > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Ladi Prosek <lprosek@redhat.com> > > --- > > hw/virtio/virtio-balloon.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index 5af429a..0baf4b3 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -262,13 +262,6 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > > goto out; > > } > > > > - if (s->stats_vq_elem != NULL) { > > - /* This should never happen if the driver follows the spec. */ > > - virtqueue_push(vq, s->stats_vq_elem, 0); > > - virtio_notify(vdev, vq); > > - g_free(s->stats_vq_elem); > > - } > > - > > Worth adding an assert? > > assert(s->stats_vq_elem == NULL); // enforced by one-element stats virtqueue Yep, makes perfect sense. Thanks, Roman.
On Thu, Aug 18, 2016 at 09:27:52PM +0300, Roman Kagan wrote: > The protocol for virtio-balloon stats virtqueue doesn't allow more than > one element in the virtqueue. > > So, instead of trying to compensate for guest misbehavior if it sends > new data before the slot has been released by the host, just define the > stats virtqueue length to 1 initially and rely on the generic virtio > code to handle overflows. > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Ladi Prosek <lprosek@redhat.com> > --- > hw/virtio/virtio-balloon.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 5af429a..0baf4b3 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -262,13 +262,6 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > goto out; > } > > - if (s->stats_vq_elem != NULL) { > - /* This should never happen if the driver follows the spec. */ > - virtqueue_push(vq, s->stats_vq_elem, 0); > - virtio_notify(vdev, vq); > - g_free(s->stats_vq_elem); > - } > - > s->stats_vq_elem = elem; > > /* Initialize the stats to get rid of any stale values. This is only > @@ -443,7 +436,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) > > s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); > s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); > - s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); > + s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats); This change breaks live migration compatibility. The device is not allowed to change in existing machine types. I think Ladi's fix for this bug is the way to go. It preserves migration compatibility.
On Fri, Aug 19, 2016 at 02:44:10PM +0100, Stefan Hajnoczi wrote: > On Thu, Aug 18, 2016 at 09:27:52PM +0300, Roman Kagan wrote: > > The protocol for virtio-balloon stats virtqueue doesn't allow more than > > one element in the virtqueue. > > > > So, instead of trying to compensate for guest misbehavior if it sends > > new data before the slot has been released by the host, just define the > > stats virtqueue length to 1 initially and rely on the generic virtio > > code to handle overflows. > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Ladi Prosek <lprosek@redhat.com> > > --- > > hw/virtio/virtio-balloon.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index 5af429a..0baf4b3 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -262,13 +262,6 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > > goto out; > > } > > > > - if (s->stats_vq_elem != NULL) { > > - /* This should never happen if the driver follows the spec. */ > > - virtqueue_push(vq, s->stats_vq_elem, 0); > > - virtio_notify(vdev, vq); > > - g_free(s->stats_vq_elem); > > - } > > - > > s->stats_vq_elem = elem; > > > > /* Initialize the stats to get rid of any stale values. This is only > > @@ -443,7 +436,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) > > > > s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); > > s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); > > - s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); > > + s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats); > > This change breaks live migration compatibility. The device is not > allowed to change in existing machine types. Ideed. > I think Ladi's fix for this bug is the way to go. It preserves > migration compatibility. Agreed. Thanks, Roman.
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index 5af429a..0baf4b3 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -262,13 +262,6 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) goto out; } - if (s->stats_vq_elem != NULL) { - /* This should never happen if the driver follows the spec. */ - virtqueue_push(vq, s->stats_vq_elem, 0); - virtio_notify(vdev, vq); - g_free(s->stats_vq_elem); - } - s->stats_vq_elem = elem; /* Initialize the stats to get rid of any stale values. This is only @@ -443,7 +436,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); - s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats); + s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats); reset_stats(s); }
The protocol for virtio-balloon stats virtqueue doesn't allow more than one element in the virtqueue. So, instead of trying to compensate for guest misbehavior if it sends new data before the slot has been released by the host, just define the stats virtqueue length to 1 initially and rely on the generic virtio code to handle overflows. Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Ladi Prosek <lprosek@redhat.com> --- hw/virtio/virtio-balloon.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)