From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42909) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZtkP-0006Y2-1q for qemu-devel@nongnu.org; Thu, 10 Sep 2015 00:46:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZZtkL-0007tE-Nu for qemu-devel@nongnu.org; Thu, 10 Sep 2015 00:46:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60000) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZZtkL-0007t6-Fm for qemu-devel@nongnu.org; Thu, 10 Sep 2015 00:46:05 -0400 References: <1441697927-16456-1-git-send-email-yuanhan.liu@linux.intel.com> <1441697927-16456-6-git-send-email-yuanhan.liu@linux.intel.com> <55F0F593.9030408@redhat.com> <20150910035702.GP2925@yliu-dev.sh.intel.com> From: Jason Wang Message-ID: <55F10B08.1060707@redhat.com> Date: Thu, 10 Sep 2015 12:46:00 +0800 MIME-Version: 1.0 In-Reply-To: <20150910035702.GP2925@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/7] vhost_net: move vhost_net_set_vq_index ahead at vhost_net_init List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yuanhan Liu Cc: mst@redhat.com, qemu-devel@nongnu.org, changchun.ouyang@intel.com On 09/10/2015 11:57 AM, Yuanhan Liu wrote: > On Thu, Sep 10, 2015 at 11:14:27AM +0800, Jason Wang wrote: >> >> On 09/08/2015 03:38 PM, Yuanhan Liu wrote: >>> So that we could use the `vq_index' as well in the vhost_net_init >>> stage, which is required when adding vhost-user multiple-queue support, >>> where we need the vq_index to indicate which queue pair we are gonna >>> initiate. >>> >>> vhost-user has no multiple queue support yet, hence no queue_index set >>> before. Here is a quick set to 0 at net_vhost_user_init() stage, and it >>> will be set properly soon in the next patch. >>> >>> Signed-off-by: Yuanhan Liu >>> --- >>> hw/net/vhost_net.c | 16 +++++++--------- >>> net/vhost-user.c | 1 + >>> 2 files changed, 8 insertions(+), 9 deletions(-) >>> >>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >>> index f9441e9..141b557 100644 >>> --- a/hw/net/vhost_net.c >>> +++ b/hw/net/vhost_net.c >>> @@ -138,6 +138,11 @@ static int vhost_net_get_fd(NetClientState *backend) >>> } >>> } >>> >>> +static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) >>> +{ >>> + net->dev.vq_index = vq_index; >>> +} >>> + >>> struct vhost_net *vhost_net_init(VhostNetOptions *options) >>> { >>> int r; >>> @@ -167,6 +172,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) >>> } >>> net->nc = options->net_backend; >>> >>> + vhost_net_set_vq_index(net, net->nc->queue_index * 2); >>> + >> This breaks vhost kernel multiqueue since queue_index was not >> initialized at this time. > Right, thanks for pointing it out. > >> We do this in set_netdev() instead of setting >> it in each kind of netdev. > Can we move it to net_init_tap() for setting the right queue_index > for each nc? > > Or, can we call vhost_net_set_vq_index twice, one at vhost_net_init(for > vhost-user mq support), another one at vhost_net_start(for vhost kernel > mq support)? > > Or, do you have better ideas? I think setting queue_index in net_init_tap() looks ok. But a question is that why need we do this at so early stage? ( Even before its peers is connected.) > >>> net->dev.nvqs = 2; >>> net->dev.vqs = net->vqs; >>> >>> @@ -196,11 +203,6 @@ fail: >>> return NULL; >>> } >>> >>> -static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index) >>> -{ >>> - net->dev.vq_index = vq_index; >>> -} >>> - >>> static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer, >>> bool set) >>> { >>> @@ -325,10 +327,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, >>> goto err; >>> } >>> >>> - for (i = 0; i < total_queues; i++) { >>> - vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2); >>> - } >>> - >>> r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true); >>> if (r < 0) { >>> error_report("Error binding guest notifier: %d", -r); >>> diff --git a/net/vhost-user.c b/net/vhost-user.c >>> index 93dcecd..2d6bbe5 100644 >>> --- a/net/vhost-user.c >>> +++ b/net/vhost-user.c >>> @@ -146,6 +146,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, >>> /* We don't provide a receive callback */ >>> s->nc.receive_disabled = 1; >>> s->chr = chr; >>> + nc->queue_index = 0; >>> >> Fail to understand why this is needed. It will be set to correct value >> in set_netdev(). > Right, it's unnecessary. > > BTW, May I ask you a quick question: why set_netdev is invoked after > tap init, while it is invoked before vhost-user init? > > --yliu I think the reason is that you must initialize all netdevs before a device tries to connect them. >>> qemu_chr_add_handlers(s->chr, NULL, NULL, net_vhost_user_event, s); >>>