From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id A4F0E370 for ; Mon, 23 Nov 2015 04:20:51 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP; 22 Nov 2015 19:20:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,335,1444719600"; d="scan'208";a="605484689" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by FMSMGA003.fm.intel.com with ESMTP; 22 Nov 2015 19:20:49 -0800 Date: Mon, 23 Nov 2015 11:22:37 +0800 From: Yuanhan Liu To: Victor Kaplansky Message-ID: <20151123032237.GC2325@yliu-dev.sh.intel.com> References: <1448026465-27460-1-git-send-email-victork@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1448026465-27460-1-git-send-email-victork@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: dev@dpdk.org, Marcel Apfelbaum , Marc-Andre Lureau , "Michael S. Tsirkin" Subject: Re: [dpdk-dev] [PATCH] vhost-user: fix enabling of queue pair X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Nov 2015 03:20:52 -0000 On Fri, Nov 20, 2015 at 03:45:00PM +0200, Victor Kaplansky wrote: > The VHOST_USER_SET_VRING_ENABLE request is sent for each queue in > queue-pair separately. So, a queue-pair should be considered > enabled only when both RX and TX queues are enabled. Hi Victor, It was sent per queue-pair, and I found that Michael changed it to per vring. The reason I made it per queue-pair is that the backend can handle both queues in one queue-pair. However, I agree that making it per queue (vring) is better here, to keep the consistency of sending all requests (say vring_call) per vring. > > The old code caused segfault when last TX queue was enabled. > > Signed-off-by: Victor Kaplansky > --- > lib/librte_vhost/vhost_user/virtio-net-user.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte_vhost/vhost_user/virtio-net-user.c > index d07452a..c06e53e 100644 > --- a/lib/librte_vhost/vhost_user/virtio-net-user.c > +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c > @@ -317,21 +317,25 @@ user_set_vring_enable(struct vhost_device_ctx ctx, > struct vhost_vring_state *state) > { > struct virtio_net *dev = get_device(ctx); > - uint16_t base_idx = state->index; > + uint16_t idx = state->index; > + uint16_t base_idx = (idx / VIRTIO_QNUM) * VIRTIO_QNUM; > int enable = (int)state->num; > + int qpair_enabled; > > RTE_LOG(INFO, VHOST_CONFIG, > "set queue enable: %d to qp idx: %d\n", > enable, state->index); > > + dev->virtqueue[idx]->enabled = enable; > + qpair_enabled = > + dev->virtqueue[base_idx + VIRTIO_RXQ]->enabled && > + dev->virtqueue[base_idx + VIRTIO_TXQ]->enabled; > + > if (notify_ops->vring_state_changed) { > notify_ops->vring_state_changed(dev, base_idx / VIRTIO_QNUM, > - enable); > + qpair_enabled); Here we could make the vring_state_changed callback be per vring then, instead of per queue-pair. That make the code simpler, as well as more consistent. Thanks. --yliu > } > > - dev->virtqueue[base_idx + VIRTIO_RXQ]->enabled = enable; > - dev->virtqueue[base_idx + VIRTIO_TXQ]->enabled = enable; > - > return 0; > } > > -- > --Victor