diff mbox

[v9,10/20] Gracefully handle ioctl failure in vhost_virtqueue_stop

Message ID 1393957383-16685-11-git-send-email-a.motakis@virtualopensystems.com
State New
Headers show

Commit Message

Antonios Motakis March 4, 2014, 6:22 p.m. UTC 
On stopping the vhost, a call to VHOST_GET_VRING_BASE is issued. The
received value is stored as last_avail_idx, so the virtqueue can continue
operating if the connection is resumed. Handle the failure of this call
and use the current avail_idx. Some packets from the avail ring may be
omitted but still we keep a sane value and can continue on reconnect.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 hw/virtio/vhost.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Michael S. Tsirkin March 4, 2014, 6:45 p.m. UTC | #1 
On Tue, Mar 04, 2014 at 07:22:53PM +0100, Antonios Motakis wrote:
> On stopping the vhost, a call to VHOST_GET_VRING_BASE is issued. The
> received value is stored as last_avail_idx, so the virtqueue can continue
> operating if the connection is resumed. Handle the failure of this call
> and use the current avail_idx. Some packets from the avail ring may be
> omitted but still we keep a sane value and can continue on reconnect.

omitted how?
some guests crash if we never complete handling buffers,
or networking breaks, etc ...

This would be a big problem for reconnect, some robust way to
communicate avail ring state would need to be found.
Is reconnect really a mandatory feature for you?
I'd suggest you drop it from v1, focus on basic functionality.

> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>

Problem is, a bunch of stuff breaks if vhost keeps
going when we ask it to stop.
In particular it will keep looking at the ring
state when guest asked it to stop doing this,
this will corrupt guest memory.


> ---
>  hw/virtio/vhost.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9e336ad..322e2c0 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -758,12 +758,13 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
>      assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
>      r = ioctl(dev->control, VHOST_GET_VRING_BASE, &state);
>      if (r < 0) {
> +        state.num = virtio_queue_get_avail_idx(vdev, idx);
>          fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx, r);
>          fflush(stderr);
>      }
>      virtio_queue_set_last_avail_idx(vdev, idx, state.num);
>      virtio_queue_invalidate_signalled_used(vdev, idx);
> -    assert (r >= 0);
> +
>      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
>                                0, virtio_queue_get_ring_size(vdev, idx));
>      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),
> -- 
> 1.8.3.2
Antonios Motakis March 5, 2014, 1:38 p.m. UTC | #2 
Hello,


On Tue, Mar 4, 2014 at 7:45 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Mar 04, 2014 at 07:22:53PM +0100, Antonios Motakis wrote:
> > On stopping the vhost, a call to VHOST_GET_VRING_BASE is issued. The
> > received value is stored as last_avail_idx, so the virtqueue can continue
> > operating if the connection is resumed. Handle the failure of this call
> > and use the current avail_idx. Some packets from the avail ring may be
> > omitted but still we keep a sane value and can continue on reconnect.
>
> omitted how?
> some guests crash if we never complete handling buffers,
> or networking breaks, etc ...
>
> This would be a big problem for reconnect, some robust way to
> communicate avail ring state would need to be found.
> Is reconnect really a mandatory feature for you?
> I'd suggest you drop it from v1, focus on basic functionality.
>
>
Reconnect would be a really useful feature for us, so we tried to keep it
in a reasonable way.

However we didn't take into account that some guests might crash under
those assumptions. Looks like we have no option but to remove reconnect
altogether for now; maybe a future extension to the virtio-net spec will
allow us to do it cleanly, but I don't see an obvious workaround to keep
this in now.

Thanks for pointing this out.

Btw, since it looks like we are closing a final version of the patches,
what kind of timeframe should we aim for inclusion? Should we already
rebase on top of Paolo's NUMA patch series?

>
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>
> Problem is, a bunch of stuff breaks if vhost keeps
> going when we ask it to stop.
> In particular it will keep looking at the ring
> state when guest asked it to stop doing this,
> this will corrupt guest memory.
>
>
> > ---
> >  hw/virtio/vhost.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 9e336ad..322e2c0 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -758,12 +758,13 @@ static void vhost_virtqueue_stop(struct vhost_dev
> *dev,
> >      assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >      r = ioctl(dev->control, VHOST_GET_VRING_BASE, &state);
> >      if (r < 0) {
> > +        state.num = virtio_queue_get_avail_idx(vdev, idx);
> >          fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx,
> r);
> >          fflush(stderr);
> >      }
> >      virtio_queue_set_last_avail_idx(vdev, idx, state.num);
> >      virtio_queue_invalidate_signalled_used(vdev, idx);
> > -    assert (r >= 0);
> > +
> >      cpu_physical_memory_unmap(vq->ring,
> virtio_queue_get_ring_size(vdev, idx),
> >                                0, virtio_queue_get_ring_size(vdev, idx));
> >      cpu_physical_memory_unmap(vq->used,
> virtio_queue_get_used_size(vdev, idx),
> > --
> > 1.8.3.2
>
Michael S. Tsirkin March 5, 2014, 1:47 p.m. UTC | #3 
On Wed, Mar 05, 2014 at 02:38:34PM +0100, Antonios Motakis wrote:
> Hello,
> 
> 
> On Tue, Mar 4, 2014 at 7:45 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Mar 04, 2014 at 07:22:53PM +0100, Antonios Motakis wrote:
>     > On stopping the vhost, a call to VHOST_GET_VRING_BASE is issued. The
>     > received value is stored as last_avail_idx, so the virtqueue can continue
>     > operating if the connection is resumed. Handle the failure of this call
>     > and use the current avail_idx. Some packets from the avail ring may be
>     > omitted but still we keep a sane value and can continue on reconnect.
> 
>     omitted how?
>     some guests crash if we never complete handling buffers,
>     or networking breaks, etc ...
> 
>     This would be a big problem for reconnect, some robust way to
>     communicate avail ring state would need to be found.
>     Is reconnect really a mandatory feature for you?
>     I'd suggest you drop it from v1, focus on basic functionality.
> 
> 
> 
> Reconnect would be a really useful feature for us, so we tried to keep it in a
> reasonable way.
> 
> However we didn't take into account that some guests might crash under those
> assumptions. Looks like we have no option but to remove reconnect altogether
> for now; maybe a future extension to the virtio-net spec will allow us to do it
> cleanly, but I don't see an obvious workaround to keep this in now.
> 
> Thanks for pointing this out.
> 
> Btw, since it looks like we are closing a final version of the patches, what
> kind of timeframe should we aim for inclusion? Should we already rebase on top
> of Paolo's NUMA patch series?
> 

I think it should be possible to merge after Paolo's
patches go in, yes. I haven't followed them closely
so I don't know when will that be.

I wish someone else would ack the char dev changes - anyone?
But it's not a blocker requirement.


>     >
>     > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>     > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> 
>     Problem is, a bunch of stuff breaks if vhost keeps
>     going when we ask it to stop.
>     In particular it will keep looking at the ring
>     state when guest asked it to stop doing this,
>     this will corrupt guest memory.
> 
> 
>     > ---
>     >  hw/virtio/vhost.c | 3 ++-
>     >  1 file changed, 2 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>     > index 9e336ad..322e2c0 100644
>     > --- a/hw/virtio/vhost.c
>     > +++ b/hw/virtio/vhost.c
>     > @@ -758,12 +758,13 @@ static void vhost_virtqueue_stop(struct vhost_dev
>     *dev,
>     >      assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
>     >      r = ioctl(dev->control, VHOST_GET_VRING_BASE, &state);
>     >      if (r < 0) {
>     > +        state.num = virtio_queue_get_avail_idx(vdev, idx);
>     >          fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx,
>     r);
>     >          fflush(stderr);
>     >      }
>     >      virtio_queue_set_last_avail_idx(vdev, idx, state.num);
>     >      virtio_queue_invalidate_signalled_used(vdev, idx);
>     > -    assert (r >= 0);
>     > +
>     >      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev,
>     idx),
>     >                                0, virtio_queue_get_ring_size(vdev, idx));
>     >      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev,
>     idx),
>     > --
>     > 1.8.3.2
> 
> 
> 
> 
> --
> Antonios Motakis
> Virtual Open Systems
diff mbox

Patch

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9e336ad..322e2c0 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -758,12 +758,13 @@  static void vhost_virtqueue_stop(struct vhost_dev *dev,
     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
     r = ioctl(dev->control, VHOST_GET_VRING_BASE, &state);
     if (r < 0) {
+        state.num = virtio_queue_get_avail_idx(vdev, idx);
         fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx, r);
         fflush(stderr);
     }
     virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     virtio_queue_invalidate_signalled_used(vdev, idx);
-    assert (r >= 0);
+
     cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
                               0, virtio_queue_get_ring_size(vdev, idx));
     cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),