From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 594E12B8B for ; Thu, 5 May 2016 05:29:47 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 04 May 2016 20:29:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,580,1455004800"; d="scan'208";a="697004779" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 04 May 2016 20:29:46 -0700 Received: from fmsmsx118.amr.corp.intel.com (10.18.116.18) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 4 May 2016 20:29:46 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx118.amr.corp.intel.com (10.18.116.18) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 4 May 2016 20:29:46 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.148]) by SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0248.002; Thu, 5 May 2016 11:29:45 +0800 From: "Xie, Huawei" To: Yuanhan Liu CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue Thread-Index: AdGmcQ0ysEggt5/7Q9GTakcbXYug1Q== Date: Thu, 5 May 2016 03:29:44 +0000 Message-ID: References: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> <20160505000327.GT5641@yliu-dev.sh.intel.com> <20160505030704.GU5641@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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:29:47 -0000 On 5/5/2016 11:03 AM, Yuanhan Liu wrote:=0A= > On Thu, May 05, 2016 at 01:54:25AM +0000, Xie, Huawei wrote:=0A= >> On 5/5/2016 7:59 AM, Yuanhan Liu wrote:=0A= >>> On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:=0A= >>>> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,=0A= >>>> - int queue_type,=0A= >>>> - uint16_t queue_idx,=0A= >>>> +static int=0A= >>>> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,=0A= >>> While it's good to split Rx/Tx specific stuff, but why are you trying t= o=0A= >>> remove a common queue_setup function that does common setups, such as v= ring=0A= >>> memory allocation.=0A= >>>=0A= >>> This results to much duplicated code: following diff summary also shows= =0A= >>> it clearly:=0A= >> The motivation to do this is we need separate RX/TX queue setup.=0A= > We actually have done that. If you look at current rx/tx/ctrl_queue_setup= ()=0A= > code, we invoked the common function; we also did some queue specific=0A= > settings. It has not been done in a very clean way though: there are quit= e=0A= > many "if .. else .." as you stated. And that's what you are going to reso= lve,=0A= > but IMO, you went far: you made __same__ code 3 copies, one for rx, tx an= d=0A= > ctrl queue, respectively.=0A= >=0A= >> The switch/case in the common queue setup looks bad.=0A= > Assuming you are talking about the "if .. else .." ...=0A= >=0A= > While I agree with you on that, introducing so many duplicated code is wo= rse.=0A= >=0A= >> I am aware of the common operations, and i had planned to extract them,= =0A= >> maybe i could do this in this patchset.=0A= > If you meant to do in another patch on top of this patch, then it looks= =0A= > like the wrong way to go: breaking something first and then fixing it=0A= > later does not sound a good practice to me.=0A= =0A= To your later comment, we could split first, then do the queue setup rework= .=0A= =0A= >=0A= >>> 7 files changed, 655 insertions(+), 422 deletions(-)=0A= >>>=0A= >>> which makes it harder for maintaining.=0A= >>>=0A= >>>> -}=0A= >>>> + rxvq =3D (struct virtnet_rx *)RTE_PTR_ADD(vq,=0A= >>>> + sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));=0A= >>>> + rxvq->vq =3D vq;=0A= >>>> + vq->sw_ring =3D sw_ring;=0A= >>> sw_ring is needed for rx queue only, why not moving it to rx queue stru= ct?=0A= >> Actually this is not about sw_ring.=0A= >> I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra= .=0A= >> Two issues=0A= >> 1. RX uses both sw_ring and vq_desc_extra=0A= >> 2. ndescs in vq_desc_extra isn't really needed, we could simply=0A= >> calculate this when we walk through the desc chain, and in most cases,= =0A= >> it is 1 or 2.=0A= >>=0A= >> As it is not related to this rework, will do this in a separate patch.= =0A= > Yes, it's not related to this patch, and this patch does rx/tx split=0A= > only. So, thinking that sw_ring is for rx only, you should move there.=0A= >=0A= > It will not against with your plan; you can make corresponding change=0A= > there. But for this patch, let's do the split only.=0A= >=0A= > BTW, I still would suggest you to build the patch on top of the cleanup= =0A= > and memory leak fix patches from Jianfeng. Your patch won't apply on=0A= > top of current dpdk-next-virtio, and one way or another, you need do=0A= > a rebase.=0A= >=0A= > Last, if I were you, I would split this patch in two: one to move=0A= > the queue specific settings to it's queue setup function, another=0A= > to split rx/tx fields. That would make it easier for review.=0A= >=0A= > --yliu=0A= >=0A= =0A=