[Qemu-devel] [PATCH 0/1] vhost-user: support of live migration with multiqueue

[Qemu-devel] [PATCH 0/1] vhost-user: support of live migration with multiqueue

[Thibaut Collet]

Patches serie for vhost-user live migration from Marc-Andre Lureau
[PATCH v8 00/27] vhost-user: add migration support
(http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg02452.html) does not
work if multiqueue is set.
This patch correct an issue of queue index when a migration is started with
multiqueue

Thibaut Collet (1):
  vhost: set the correct queue index in case of migration with
    multiqueue

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

-- 
2.1.4

[Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue

[Thibaut Collet]

When a live migration is started the log address to mark dirty pages is provided
to the vhost backend through the vhost_dev_set_log function.
This function is called for each queue pairs but the queue index is wrongly set:
always set to the first queue pair. Then vhost backend lost descriptor addresses
of the queue pairs greater than 1 and behaviour of the vhost backend is
unpredictable.

The queue index is computed by taking account of the vq_index (to retrieve the
queue pair index) and calling the vhost_get_vq_index method of the backend.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index feeaaa4..de29968 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
 
 static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
 {
-    int r, t, i;
+    int r, t, i, idx;
     r = vhost_dev_set_features(dev, enable_log);
     if (r < 0) {
         goto err_features;
     }
     for (i = 0; i < dev->nvqs; ++i) {
-        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
+        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
+        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
                                      enable_log);
         if (r < 0) {
             goto err_vq;
@@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
     return 0;
 err_vq:
     for (; i >= 0; --i) {
-        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
+        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
+        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
                                      dev->log_enabled);
         assert(t >= 0);
     }
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue

[Michael S. Tsirkin]

On Mon, Oct 19, 2015 at 02:59:27PM +0200, Thibaut Collet wrote:
> When a live migration is started the log address to mark dirty pages is provided
> to the vhost backend through the vhost_dev_set_log function.
> This function is called for each queue pairs but the queue index is wrongly set:
> always set to the first queue pair. Then vhost backend lost descriptor addresses
> of the queue pairs greater than 1 and behaviour of the vhost backend is
> unpredictable.
> 
> The queue index is computed by taking account of the vq_index (to retrieve the
> queue pair index) and calling the vhost_get_vq_index method of the backend.
> 
> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>

This needs some thought to make sure we don't break the kernel vhost.

I queued this temporarily to enable your testing but I think it would be
preferable to make vhost_virtqueue_set_addr for vhost_user call
vhost_get_vq_index internally.



> ---
>  hw/virtio/vhost.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index feeaaa4..de29968 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
>  
>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>  {
> -    int r, t, i;
> +    int r, t, i, idx;
>      r = vhost_dev_set_features(dev, enable_log);
>      if (r < 0) {
>          goto err_features;
>      }
>      for (i = 0; i < dev->nvqs; ++i) {
> -        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> +        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>                                       enable_log);
>          if (r < 0) {
>              goto err_vq;
> @@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>      return 0;
>  err_vq:
>      for (; i >= 0; --i) {
> -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> +        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>                                       dev->log_enabled);
>          assert(t >= 0);
>      }
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue

[Thibaut Collet]

On Mon, Oct 19, 2015 at 5:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Oct 19, 2015 at 02:59:27PM +0200, Thibaut Collet wrote:
>> When a live migration is started the log address to mark dirty pages is provided
>> to the vhost backend through the vhost_dev_set_log function.
>> This function is called for each queue pairs but the queue index is wrongly set:
>> always set to the first queue pair. Then vhost backend lost descriptor addresses
>> of the queue pairs greater than 1 and behaviour of the vhost backend is
>> unpredictable.
>>
>> The queue index is computed by taking account of the vq_index (to retrieve the
>> queue pair index) and calling the vhost_get_vq_index method of the backend.
>>
>> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
>
> This needs some thought to make sure we don't break the kernel vhost.

For kernel vhost my patch does nothing has vhost_get_vq_index method
for vhost kernel subtract dev->vq_index (that was just added before)
and idx is still equal to i.

>
> I queued this temporarily to enable your testing but I think it would be
> preferable to make vhost_virtqueue_set_addr for vhost_user call
> vhost_get_vq_index internally.
>

If I call the vhost_get_vq_index internally by vhost_user when
vhost_virtqueue_set_addr is called I will break the
vhost_virtqueue_start: this function calls the vhost_get_vq_index
function for vhost user and vhost kernel to initializes the queue.

>
>
>> ---
>>  hw/virtio/vhost.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index feeaaa4..de29968 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
>>
>>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>>  {
>> -    int r, t, i;
>> +    int r, t, i, idx;
>>      r = vhost_dev_set_features(dev, enable_log);
>>      if (r < 0) {
>>          goto err_features;
>>      }
>>      for (i = 0; i < dev->nvqs; ++i) {
>> -        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
>> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
>> +        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>>                                       enable_log);
>>          if (r < 0) {
>>              goto err_vq;
>> @@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>>      return 0;
>>  err_vq:
>>      for (; i >= 0; --i) {
>> -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
>> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
>> +        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>>                                       dev->log_enabled);
>>          assert(t >= 0);
>>      }
>> --
>> 2.1.4

Re: [Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue

[Michael S. Tsirkin]

On Mon, Oct 19, 2015 at 06:41:38PM +0200, Thibaut Collet wrote:
> On Mon, Oct 19, 2015 at 5:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Oct 19, 2015 at 02:59:27PM +0200, Thibaut Collet wrote:
> >> When a live migration is started the log address to mark dirty pages is provided
> >> to the vhost backend through the vhost_dev_set_log function.
> >> This function is called for each queue pairs but the queue index is wrongly set:
> >> always set to the first queue pair. Then vhost backend lost descriptor addresses
> >> of the queue pairs greater than 1 and behaviour of the vhost backend is
> >> unpredictable.
> >>
> >> The queue index is computed by taking account of the vq_index (to retrieve the
> >> queue pair index) and calling the vhost_get_vq_index method of the backend.
> >>
> >> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> >
> > This needs some thought to make sure we don't break the kernel vhost.
> 
> For kernel vhost my patch does nothing has vhost_get_vq_index method
> for vhost kernel subtract dev->vq_index (that was just added before)
> and idx is still equal to i.
> 
> >
> > I queued this temporarily to enable your testing but I think it would be
> > preferable to make vhost_virtqueue_set_addr for vhost_user call
> > vhost_get_vq_index internally.
> >
> 
> If I call the vhost_get_vq_index internally by vhost_user when
> vhost_virtqueue_set_addr is called I will break the
> vhost_virtqueue_start: this function calls the vhost_get_vq_index
> function for vhost user and vhost kernel to initializes the queue.

So drop vhost_get_vq_index from there as well then?


> >
> >
> >> ---
> >>  hw/virtio/vhost.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index feeaaa4..de29968 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
> >>
> >>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >>  {
> >> -    int r, t, i;
> >> +    int r, t, i, idx;
> >>      r = vhost_dev_set_features(dev, enable_log);
> >>      if (r < 0) {
> >>          goto err_features;
> >>      }
> >>      for (i = 0; i < dev->nvqs; ++i) {
> >> -        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> >> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> >> +        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >>                                       enable_log);
> >>          if (r < 0) {
> >>              goto err_vq;
> >> @@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >>      return 0;
> >>  err_vq:
> >>      for (; i >= 0; --i) {
> >> -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> >> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> >> +        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >>                                       dev->log_enabled);
> >>          assert(t >= 0);
> >>      }
> >> --
> >> 2.1.4

Re: [Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue

[Michael S. Tsirkin]

On Mon, Oct 19, 2015 at 02:59:27PM +0200, Thibaut Collet wrote:
> When a live migration is started the log address to mark dirty pages is provided
> to the vhost backend through the vhost_dev_set_log function.
> This function is called for each queue pairs but the queue index is wrongly set:
> always set to the first queue pair. Then vhost backend lost descriptor addresses
> of the queue pairs greater than 1 and behaviour of the vhost backend is
> unpredictable.
> 
> The queue index is computed by taking account of the vq_index (to retrieve the
> queue pair index) and calling the vhost_get_vq_index method of the backend.
> 
> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>

Thanks!
The code in question was added by:
    commit a9f98bb5ebe6fb1869321dcc58e72041ae626ad8
    Author: Jason Wang <jasowang@redhat.com>
    Date:   Wed Jan 30 19:12:35 2013 +0800

        vhost: multiqueue support

Jason, could you comment on whether this makes sense please?
If yes - this is an old bug, and we need this on stable,
do we not?

Maybe we should refactor vhost_virtqueue_set_addr to
make it call vhost_get_vq_index internally automatically.
All callers do this anyway.
This can be a patch on top.

> ---
>  hw/virtio/vhost.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index feeaaa4..de29968 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
>  
>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>  {
> -    int r, t, i;
> +    int r, t, i, idx;
>      r = vhost_dev_set_features(dev, enable_log);
>      if (r < 0) {
>          goto err_features;
>      }
>      for (i = 0; i < dev->nvqs; ++i) {
> -        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> +        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>                                       enable_log);
>          if (r < 0) {
>              goto err_vq;
> @@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>      return 0;
>  err_vq:
>      for (; i >= 0; --i) {
> -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> +        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>                                       dev->log_enabled);
>          assert(t >= 0);
>      }
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue

[Jason Wang]

On 10/20/2015 09:33 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 19, 2015 at 02:59:27PM +0200, Thibaut Collet wrote:
>> When a live migration is started the log address to mark dirty pages is provided
>> to the vhost backend through the vhost_dev_set_log function.
>> This function is called for each queue pairs but the queue index is wrongly set:
>> always set to the first queue pair. Then vhost backend lost descriptor addresses
>> of the queue pairs greater than 1 and behaviour of the vhost backend is
>> unpredictable.
>>
>> The queue index is computed by taking account of the vq_index (to retrieve the
>> queue pair index) and calling the vhost_get_vq_index method of the backend.
>>
>> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> Thanks!
> The code in question was added by:
>     commit a9f98bb5ebe6fb1869321dcc58e72041ae626ad8
>     Author: Jason Wang <jasowang@redhat.com>
>     Date:   Wed Jan 30 19:12:35 2013 +0800
>
>         vhost: multiqueue support
>
> Jason, could you comment on whether this makes sense please?

It makes sense.

> If yes - this is an old bug, and we need this on stable,
> do we not?

But not an old bug, only vhost-user has this issue. So no need for stable.

>
> Maybe we should refactor vhost_virtqueue_set_addr to
> make it call vhost_get_vq_index internally automatically.
> All callers do this anyway.
> This can be a patch on top.

Yes, this looks cleaner.

>
>> ---
>>  hw/virtio/vhost.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index feeaaa4..de29968 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
>>  
>>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>>  {
>> -    int r, t, i;
>> +    int r, t, i, idx;
>>      r = vhost_dev_set_features(dev, enable_log);
>>      if (r < 0) {
>>          goto err_features;
>>      }
>>      for (i = 0; i < dev->nvqs; ++i) {
>> -        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
>> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
>> +        r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>>                                       enable_log);
>>          if (r < 0) {
>>              goto err_vq;
>> @@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>>      return 0;
>>  err_vq:
>>      for (; i >= 0; --i) {
>> -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
>> +        idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
>> +        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>>                                       dev->log_enabled);
>>          assert(t >= 0);
>>      }
>> -- 
>> 2.1.4