* [Qemu-devel] [PATCH for-2.5 0/1] vhost-user: live migration with multiqueue fails @ 2015-11-24 16:10 Thibaut Collet 2015-11-24 16:10 ` [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start Thibaut Collet 0 siblings, 1 reply; 9+ messages in thread From: Thibaut Collet @ 2015-11-24 16:10 UTC (permalink / raw) To: thibaut.collet, qemu-devel, mst, jasowang, marcandre.lureau, yuanhan.liu 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-24 16:10 [Qemu-devel] [PATCH for-2.5 0/1] vhost-user: live migration with multiqueue fails Thibaut Collet @ 2015-11-24 16:10 ` Thibaut Collet 2015-11-24 20:52 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Thibaut Collet @ 2015-11-24 16:10 UTC (permalink / raw) To: thibaut.collet, qemu-devel, mst, jasowang, marcandre.lureau, yuanhan.liu 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-24 16:10 ` [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start Thibaut Collet @ 2015-11-24 20:52 ` Michael S. Tsirkin 2015-11-24 21:05 ` Thibaut Collet 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2015-11-24 20:52 UTC (permalink / raw) To: Thibaut Collet Cc: yuanhan.liu, jasowang, Victor Kaplansky, marcandre.lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-24 20:52 ` Michael S. Tsirkin @ 2015-11-24 21:05 ` Thibaut Collet 2015-11-24 21:23 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Thibaut Collet @ 2015-11-24 21:05 UTC (permalink / raw) To: Michael S. Tsirkin Cc: yuanhan.liu, Jason Wang, Victor Kaplansky, Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-24 21:05 ` Thibaut Collet @ 2015-11-24 21:23 ` Michael S. Tsirkin 2015-11-25 1:32 ` Yuanhan Liu 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2015-11-24 21:23 UTC (permalink / raw) To: Thibaut Collet Cc: yuanhan.liu, Jason Wang, Victor Kaplansky, Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-24 21:23 ` Michael S. Tsirkin @ 2015-11-25 1:32 ` Yuanhan Liu 2015-11-25 11:04 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Yuanhan Liu @ 2015-11-25 1:32 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Thibaut Collet, Jason Wang, Victor Kaplansky, Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-25 1:32 ` Yuanhan Liu @ 2015-11-25 11:04 ` Michael S. Tsirkin 2015-11-25 12:17 ` Thibaut Collet 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2015-11-25 11:04 UTC (permalink / raw) To: Yuanhan Liu Cc: Thibaut Collet, Jason Wang, Victor Kaplansky, Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-25 11:04 ` Michael S. Tsirkin @ 2015-11-25 12:17 ` Thibaut Collet 2015-11-25 12:23 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Thibaut Collet @ 2015-11-25 12:17 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Yuanhan Liu, Victor Kaplansky, Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start 2015-11-25 12:17 ` Thibaut Collet @ 2015-11-25 12:23 ` Michael S. Tsirkin 0 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2015-11-25 12:23 UTC (permalink / raw) To: Thibaut Collet Cc: Jason Wang, Yuanhan Liu, Victor Kaplansky, Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-11-25 12:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-24 16:10 [Qemu-devel] [PATCH for-2.5 0/1] vhost-user: live migration with multiqueue fails Thibaut Collet 2015-11-24 16:10 ` [Qemu-devel] [PATCH for-2.5 1/1] vhost-user: do not send SET_VRING_ENABLE at start Thibaut Collet 2015-11-24 20:52 ` Michael S. Tsirkin 2015-11-24 21:05 ` Thibaut Collet 2015-11-24 21:23 ` Michael S. Tsirkin 2015-11-25 1:32 ` Yuanhan Liu 2015-11-25 11:04 ` Michael S. Tsirkin 2015-11-25 12:17 ` Thibaut Collet 2015-11-25 12:23 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.