[Qemu-devel] [PATCH for-2.5 0/1] vhost-user: live migration with multiqueue fails

[Qemu-devel] [PATCH for-2.5 0/1] vhost-user: live migration with multiqueue fails

[Thibaut Collet]

Since commit 3a12f32229a live migration of vhost user, if multiqueue is set,
fails:
The state of the vring are lost, only the first queue pair is set to enable even
if there are several queue pairs enabled.

Thibaut Collet (1):
  vhost-user: do not send SET_VRING_ENABLE at start

 hw/virtio/vhost.c | 5 -----
 1 file changed, 5 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Thibaut Collet]

This patch reverts partially commit 3a12f32229a.

In case of live migration several queues can be enabled and not only the first
one. So inform backend that only the first queue is enabled is wrong.

Since commit 7263a0ad7899 backend is already notified of the state of the vring
through the vring attach operation. This function, called during the startup
sequence, provides the correct state of the vring, even in case of live
migration.

So nothing has to be added to give the vring state to the backend at the startup.

Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
---
 hw/virtio/vhost.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1794f0d..870cd12 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
     }
 
-    if (hdev->vhost_ops->vhost_set_vring_enable) {
-        /* only enable first vq pair by default */
-        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
-    }
-
     return 0;
 fail_log:
     vhost_log_put(hdev, false);
-- 
2.1.4

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Michael S. Tsirkin]

On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
> This patch reverts partially commit 3a12f32229a.
> 
> In case of live migration several queues can be enabled and not only the first
> one. So inform backend that only the first queue is enabled is wrong.
> 
> Since commit 7263a0ad7899 backend is already notified of the state of the vring
> through the vring attach operation. This function, called during the startup
> sequence, provides the correct state of the vring, even in case of live
> migration.
> 
> So nothing has to be added to give the vring state to the backend at the startup.
> 
> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> ---
>  hw/virtio/vhost.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1794f0d..870cd12 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>          }
>      }
>  
> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> -        /* only enable first vq pair by default */
> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> -    }
> -
>      return 0;
>  fail_log:
>      vhost_log_put(hdev, false);
> -- 
> 2.1.4

Yes - and I'm beginning to think that maybe we should revert
all of 3a12f32229a then, for symmetry.

Yunnan, Victor - what do you think?

-- 
MST

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Thibaut Collet]

On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
>> This patch reverts partially commit 3a12f32229a.
>>
>> In case of live migration several queues can be enabled and not only the first
>> one. So inform backend that only the first queue is enabled is wrong.
>>
>> Since commit 7263a0ad7899 backend is already notified of the state of the vring
>> through the vring attach operation. This function, called during the startup
>> sequence, provides the correct state of the vring, even in case of live
>> migration.
>>
>> So nothing has to be added to give the vring state to the backend at the startup.
>>
>> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
>> ---
>>  hw/virtio/vhost.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 1794f0d..870cd12 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>>          }
>>      }
>>
>> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
>> -        /* only enable first vq pair by default */
>> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
>> -    }
>> -
>>      return 0;
>>  fail_log:
>>      vhost_log_put(hdev, false);
>> --
>> 2.1.4
>
> Yes - and I'm beginning to think that maybe we should revert
> all of 3a12f32229a then, for symmetry.
>

Keep the disable vring on the stop can be useful. For example if the
VM is rebooted all the vring will be disabled and backend will avoid
to send packet to the VM in this case (I am not sure the virtio ring
address is always valid during a reboot and writingg data in this
memory can cause unexpected behaviour in this case).

> Yunnan, Victor - what do you think?
>
> --
> MST

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Michael S. Tsirkin]

On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote:
> On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
> >> This patch reverts partially commit 3a12f32229a.
> >>
> >> In case of live migration several queues can be enabled and not only the first
> >> one. So inform backend that only the first queue is enabled is wrong.
> >>
> >> Since commit 7263a0ad7899 backend is already notified of the state of the vring
> >> through the vring attach operation. This function, called during the startup
> >> sequence, provides the correct state of the vring, even in case of live
> >> migration.
> >>
> >> So nothing has to be added to give the vring state to the backend at the startup.
> >>
> >> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> >> ---
> >>  hw/virtio/vhost.c | 5 -----
> >>  1 file changed, 5 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 1794f0d..870cd12 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >>          }
> >>      }
> >>
> >> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> >> -        /* only enable first vq pair by default */
> >> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> >> -    }
> >> -
> >>      return 0;
> >>  fail_log:
> >>      vhost_log_put(hdev, false);
> >> --
> >> 2.1.4
> >
> > Yes - and I'm beginning to think that maybe we should revert
> > all of 3a12f32229a then, for symmetry.
> >
> 
> Keep the disable vring on the stop can be useful. For example if the
> VM is rebooted all the vring will be disabled and backend will avoid
> to send packet to the VM in this case (I am not sure the virtio ring
> address is always valid during a reboot and writingg data in this
> memory can cause unexpected behaviour in this case).

I think there's still some confusion:
writing memory can still happen even if you disable the ring
since the TX ring is still processed so we write into the used ring.

We call GET_VRING_BASE on stop and that ensures rings are
stopped.


> > Yunnan, Victor - what do you think?
> >
> > --
> > MST

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Yuanhan Liu]

On Tue, Nov 24, 2015 at 11:23:34PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote:
> > On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
> > >> This patch reverts partially commit 3a12f32229a.
> > >>
> > >> In case of live migration several queues can be enabled and not only the first
> > >> one. So inform backend that only the first queue is enabled is wrong.
> > >>
> > >> Since commit 7263a0ad7899 backend is already notified of the state of the vring
> > >> through the vring attach operation. This function, called during the startup
> > >> sequence, provides the correct state of the vring, even in case of live
> > >> migration.
> > >>
> > >> So nothing has to be added to give the vring state to the backend at the startup.
> > >>
> > >> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> > >> ---
> > >>  hw/virtio/vhost.c | 5 -----
> > >>  1 file changed, 5 deletions(-)
> > >>
> > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > >> index 1794f0d..870cd12 100644
> > >> --- a/hw/virtio/vhost.c
> > >> +++ b/hw/virtio/vhost.c
> > >> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > >>          }
> > >>      }
> > >>
> > >> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> > >> -        /* only enable first vq pair by default */
> > >> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> > >> -    }
> > >> -
> > >>      return 0;
> > >>  fail_log:
> > >>      vhost_log_put(hdev, false);
> > >> --
> > >> 2.1.4
> > >
> > > Yes - and I'm beginning to think that maybe we should revert
> > > all of 3a12f32229a then, for symmetry.
> > >
> > 
> > Keep the disable vring on the stop can be useful. For example if the
> > VM is rebooted all the vring will be disabled and backend will avoid
> > to send packet to the VM in this case (I am not sure the virtio ring
> > address is always valid during a reboot and writingg data in this
> > memory can cause unexpected behaviour in this case).
> 
> I think there's still some confusion:
> writing memory can still happen even if you disable the ring
> since the TX ring is still processed so we write into the used ring.
> 
> We call GET_VRING_BASE on stop and that ensures rings are
> stopped.

Yes, that's what I suggested first, which also makes the logic quite
simple: we use GET_VRING_BASE as the sign of vring stop. Intead of
GET_VRING_BASE when protocol not negotiated, and SET_VRING_ENABLE
when protocol is negotiated.

Michael, should I submit a revert patch, or you could do it directly?

	--yliu
> 
> 
> > > Yunnan, Victor - what do you think?
> > >
> > > --
> > > MST

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Michael S. Tsirkin]

On Wed, Nov 25, 2015 at 09:32:15AM +0800, Yuanhan Liu wrote:
> On Tue, Nov 24, 2015 at 11:23:34PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote:
> > > On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
> > > >> This patch reverts partially commit 3a12f32229a.
> > > >>
> > > >> In case of live migration several queues can be enabled and not only the first
> > > >> one. So inform backend that only the first queue is enabled is wrong.
> > > >>
> > > >> Since commit 7263a0ad7899 backend is already notified of the state of the vring
> > > >> through the vring attach operation. This function, called during the startup
> > > >> sequence, provides the correct state of the vring, even in case of live
> > > >> migration.
> > > >>
> > > >> So nothing has to be added to give the vring state to the backend at the startup.
> > > >>
> > > >> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> > > >> ---
> > > >>  hw/virtio/vhost.c | 5 -----
> > > >>  1 file changed, 5 deletions(-)
> > > >>
> > > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > >> index 1794f0d..870cd12 100644
> > > >> --- a/hw/virtio/vhost.c
> > > >> +++ b/hw/virtio/vhost.c
> > > >> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > >>          }
> > > >>      }
> > > >>
> > > >> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> > > >> -        /* only enable first vq pair by default */
> > > >> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> > > >> -    }
> > > >> -
> > > >>      return 0;
> > > >>  fail_log:
> > > >>      vhost_log_put(hdev, false);
> > > >> --
> > > >> 2.1.4
> > > >
> > > > Yes - and I'm beginning to think that maybe we should revert
> > > > all of 3a12f32229a then, for symmetry.
> > > >
> > > 
> > > Keep the disable vring on the stop can be useful. For example if the
> > > VM is rebooted all the vring will be disabled and backend will avoid
> > > to send packet to the VM in this case (I am not sure the virtio ring
> > > address is always valid during a reboot and writingg data in this
> > > memory can cause unexpected behaviour in this case).
> > 
> > I think there's still some confusion:
> > writing memory can still happen even if you disable the ring
> > since the TX ring is still processed so we write into the used ring.
> > 
> > We call GET_VRING_BASE on stop and that ensures rings are
> > stopped.
> 
> Yes, that's what I suggested first, which also makes the logic quite
> simple: we use GET_VRING_BASE as the sign of vring stop. Intead of
> GET_VRING_BASE when protocol not negotiated, and SET_VRING_ENABLE
> when protocol is negotiated.
> 
> Michael, should I submit a revert patch, or you could do it directly?
> 
> 	--yliu

I can handle it.

> > 
> > 
> > > > Yunnan, Victor - what do you think?
> > > >
> > > > --
> > > > MST

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Thibaut Collet]

On Wed, Nov 25, 2015 at 12:04 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Nov 25, 2015 at 09:32:15AM +0800, Yuanhan Liu wrote:
>> On Tue, Nov 24, 2015 at 11:23:34PM +0200, Michael S. Tsirkin wrote:
>> > On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote:
>> > > On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > > > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
>> > > >> This patch reverts partially commit 3a12f32229a.
>> > > >>
>> > > >> In case of live migration several queues can be enabled and not only the first
>> > > >> one. So inform backend that only the first queue is enabled is wrong.
>> > > >>
>> > > >> Since commit 7263a0ad7899 backend is already notified of the state of the vring
>> > > >> through the vring attach operation. This function, called during the startup
>> > > >> sequence, provides the correct state of the vring, even in case of live
>> > > >> migration.
>> > > >>
>> > > >> So nothing has to be added to give the vring state to the backend at the startup.
>> > > >>
>> > > >> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
>> > > >> ---
>> > > >>  hw/virtio/vhost.c | 5 -----
>> > > >>  1 file changed, 5 deletions(-)
>> > > >>
>> > > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> > > >> index 1794f0d..870cd12 100644
>> > > >> --- a/hw/virtio/vhost.c
>> > > >> +++ b/hw/virtio/vhost.c
>> > > >> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>> > > >>          }
>> > > >>      }
>> > > >>
>> > > >> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
>> > > >> -        /* only enable first vq pair by default */
>> > > >> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
>> > > >> -    }
>> > > >> -
>> > > >>      return 0;
>> > > >>  fail_log:
>> > > >>      vhost_log_put(hdev, false);
>> > > >> --
>> > > >> 2.1.4
>> > > >
>> > > > Yes - and I'm beginning to think that maybe we should revert
>> > > > all of 3a12f32229a then, for symmetry.
>> > > >
>> > >
>> > > Keep the disable vring on the stop can be useful. For example if the
>> > > VM is rebooted all the vring will be disabled and backend will avoid
>> > > to send packet to the VM in this case (I am not sure the virtio ring
>> > > address is always valid during a reboot and writingg data in this
>> > > memory can cause unexpected behaviour in this case).
>> >
>> > I think there's still some confusion:
>> > writing memory can still happen even if you disable the ring
>> > since the TX ring is still processed so we write into the used ring.
>> >
>> > We call GET_VRING_BASE on stop and that ensures rings are
>> > stopped.
>>
>> Yes, that's what I suggested first, which also makes the logic quite
>> simple: we use GET_VRING_BASE as the sign of vring stop. Intead of
>> GET_VRING_BASE when protocol not negotiated, and SET_VRING_ENABLE
>> when protocol is negotiated.
>>
>> Michael, should I submit a revert patch, or you could do it directly?
>>
>>       --yliu
>
> I can handle it.
>

The patch must be completly reverted. There are too an issue with
suspend/resume operations.
On the suspend the dev_stop is called that will disable all the vrings.
On the resume the dev_start is called but not the peer_attach (vring
is already attached) and state of the vring is not provided to the
backend.

>> >
>> >
>> > > > Yunnan, Victor - what do you think?
>> > > >
>> > > > --
>> > > > MST

Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start

[Michael S. Tsirkin]

On Wed, Nov 25, 2015 at 01:17:53PM +0100, Thibaut Collet wrote:
> On Wed, Nov 25, 2015 at 12:04 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Nov 25, 2015 at 09:32:15AM +0800, Yuanhan Liu wrote:
> >> On Tue, Nov 24, 2015 at 11:23:34PM +0200, Michael S. Tsirkin wrote:
> >> > On Tue, Nov 24, 2015 at 10:05:27PM +0100, Thibaut Collet wrote:
> >> > > On Tue, Nov 24, 2015 at 9:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > > > On Tue, Nov 24, 2015 at 05:10:36PM +0100, Thibaut Collet wrote:
> >> > > >> This patch reverts partially commit 3a12f32229a.
> >> > > >>
> >> > > >> In case of live migration several queues can be enabled and not only the first
> >> > > >> one. So inform backend that only the first queue is enabled is wrong.
> >> > > >>
> >> > > >> Since commit 7263a0ad7899 backend is already notified of the state of the vring
> >> > > >> through the vring attach operation. This function, called during the startup
> >> > > >> sequence, provides the correct state of the vring, even in case of live
> >> > > >> migration.
> >> > > >>
> >> > > >> So nothing has to be added to give the vring state to the backend at the startup.
> >> > > >>
> >> > > >> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> >> > > >> ---
> >> > > >>  hw/virtio/vhost.c | 5 -----
> >> > > >>  1 file changed, 5 deletions(-)
> >> > > >>
> >> > > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> > > >> index 1794f0d..870cd12 100644
> >> > > >> --- a/hw/virtio/vhost.c
> >> > > >> +++ b/hw/virtio/vhost.c
> >> > > >> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> >> > > >>          }
> >> > > >>      }
> >> > > >>
> >> > > >> -    if (hdev->vhost_ops->vhost_set_vring_enable) {
> >> > > >> -        /* only enable first vq pair by default */
> >> > > >> -        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> >> > > >> -    }
> >> > > >> -
> >> > > >>      return 0;
> >> > > >>  fail_log:
> >> > > >>      vhost_log_put(hdev, false);
> >> > > >> --
> >> > > >> 2.1.4
> >> > > >
> >> > > > Yes - and I'm beginning to think that maybe we should revert
> >> > > > all of 3a12f32229a then, for symmetry.
> >> > > >
> >> > >
> >> > > Keep the disable vring on the stop can be useful. For example if the
> >> > > VM is rebooted all the vring will be disabled and backend will avoid
> >> > > to send packet to the VM in this case (I am not sure the virtio ring
> >> > > address is always valid during a reboot and writingg data in this
> >> > > memory can cause unexpected behaviour in this case).
> >> >
> >> > I think there's still some confusion:
> >> > writing memory can still happen even if you disable the ring
> >> > since the TX ring is still processed so we write into the used ring.
> >> >
> >> > We call GET_VRING_BASE on stop and that ensures rings are
> >> > stopped.
> >>
> >> Yes, that's what I suggested first, which also makes the logic quite
> >> simple: we use GET_VRING_BASE as the sign of vring stop. Intead of
> >> GET_VRING_BASE when protocol not negotiated, and SET_VRING_ENABLE
> >> when protocol is negotiated.
> >>
> >> Michael, should I submit a revert patch, or you could do it directly?
> >>
> >>       --yliu
> >
> > I can handle it.
> >
> 
> The patch must be completly reverted. There are too an issue with
> suspend/resume operations.
> On the suspend the dev_stop is called that will disable all the vrings.
> On the resume the dev_start is called but not the peer_attach (vring
> is already attached) and state of the vring is not provided to the
> backend.

OK, I queued up a revert. Thanks!

> >> >
> >> >
> >> > > > Yunnan, Victor - what do you think?
> >> > > >
> >> > > > --
> >> > > > MST