From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 0A3DC5684 for ; Thu, 5 May 2016 05:03:18 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 04 May 2016 20:03:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,580,1455004800"; d="scan'208";a="959209861" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga001.fm.intel.com with ESMTP; 04 May 2016 20:03:17 -0700 Date: Wed, 4 May 2016 20:07:04 -0700 From: Yuanhan Liu To: "Xie, Huawei" Cc: "dev@dpdk.org" Message-ID: <20160505030704.GU5641@yliu-dev.sh.intel.com> References: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> <20160505000327.GT5641@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue 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: Thu, 05 May 2016 03:03:19 -0000 On Thu, May 05, 2016 at 01:54:25AM +0000, Xie, Huawei wrote: > On 5/5/2016 7:59 AM, Yuanhan Liu wrote: > > On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote: > >> -int virtio_dev_queue_setup(struct rte_eth_dev *dev, > >> - int queue_type, > >> - uint16_t queue_idx, > >> +static int > >> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev, > > While it's good to split Rx/Tx specific stuff, but why are you trying to > > remove a common queue_setup function that does common setups, such as vring > > memory allocation. > > > > This results to much duplicated code: following diff summary also shows > > it clearly: > > The motivation to do this is we need separate RX/TX queue setup. We actually have done that. If you look at current rx/tx/ctrl_queue_setup() code, we invoked the common function; we also did some queue specific settings. It has not been done in a very clean way though: there are quite many "if .. else .." as you stated. And that's what you are going to resolve, but IMO, you went far: you made __same__ code 3 copies, one for rx, tx and ctrl queue, respectively. > The switch/case in the common queue setup looks bad. Assuming you are talking about the "if .. else .." ... While I agree with you on that, introducing so many duplicated code is worse. > I am aware of the common operations, and i had planned to extract them, > maybe i could do this in this patchset. If you meant to do in another patch on top of this patch, then it looks like the wrong way to go: breaking something first and then fixing it later does not sound a good practice to me. > > > > 7 files changed, 655 insertions(+), 422 deletions(-) > > > > which makes it harder for maintaining. > > > >> -} > >> + rxvq = (struct virtnet_rx *)RTE_PTR_ADD(vq, > >> + sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra)); > >> + rxvq->vq = vq; > >> + vq->sw_ring = sw_ring; > > sw_ring is needed for rx queue only, why not moving it to rx queue struct? > > Actually this is not about sw_ring. > I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra. > Two issues > 1. RX uses both sw_ring and vq_desc_extra > 2. ndescs in vq_desc_extra isn't really needed, we could simply > calculate this when we walk through the desc chain, and in most cases, > it is 1 or 2. > > As it is not related to this rework, will do this in a separate patch. Yes, it's not related to this patch, and this patch does rx/tx split only. So, thinking that sw_ring is for rx only, you should move there. It will not against with your plan; you can make corresponding change there. But for this patch, let's do the split only. BTW, I still would suggest you to build the patch on top of the cleanup and memory leak fix patches from Jianfeng. Your patch won't apply on top of current dpdk-next-virtio, and one way or another, you need do a rebase. Last, if I were you, I would split this patch in two: one to move the queue specific settings to it's queue setup function, another to split rx/tx fields. That would make it easier for review. --yliu