| Message ID | 1445259567-28516-2-git-send-email-thibaut.collet@6wind.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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); }
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(-)