From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53087) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoWvH-0003rJ-I7 for qemu-devel@nongnu.org; Tue, 20 Oct 2015 09:25:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoWvE-0008I1-Ks for qemu-devel@nongnu.org; Tue, 20 Oct 2015 09:25:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54605) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoWvE-0008Ht-DZ for qemu-devel@nongnu.org; Tue, 20 Oct 2015 09:25:48 -0400 Date: Tue, 20 Oct 2015 16:25:44 +0300 From: "Michael S. Tsirkin" Message-ID: <20151020162402-mutt-send-email-mst@redhat.com> References: <1445259567-28516-1-git-send-email-thibaut.collet@6wind.com> <1445259567-28516-2-git-send-email-thibaut.collet@6wind.com> <20151019183948-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/1] vhost: set the correct queue index in case of migration with multiqueue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thibaut Collet Cc: Paolo Bonzini , Jason Wang , =?iso-8859-1?Q?Marc-Andr=E9?= Lureau , qemu-devel , Linhaifeng 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 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 > > > > 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