From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <huawei.xie@intel.com> Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 17B592906 for <dev@dpdk.org>; Thu, 5 May 2016 07:29:31 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 04 May 2016 22:29:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,580,1455004800"; d="scan'208";a="97553485" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 04 May 2016 22:29:30 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 4 May 2016 22:29:30 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 4 May 2016 22:29:30 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.148]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.45]) with mapi id 14.03.0248.002; Thu, 5 May 2016 13:29:28 +0800 From: "Xie, Huawei" <huawei.xie@intel.com> To: Yuanhan Liu <yuanhan.liu@linux.intel.com> CC: "dev@dpdk.org" <dev@dpdk.org> Thread-Topic: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue Thread-Index: AdGmcQ0ysEggt5/7Q9GTakcbXYug1Q== Date: Thu, 5 May 2016 05:29:27 +0000 Message-ID: <C37D651A908B024F974696C65296B57B4C74CB1F@SHSMSX101.ccr.corp.intel.com> References: <1462323027-91942-1-git-send-email-huawei.xie@intel.com> <20160505000327.GT5641@yliu-dev.sh.intel.com> <C37D651A908B024F974696C65296B57B4C74C637@SHSMSX101.ccr.corp.intel.com> <20160505030704.GU5641@yliu-dev.sh.intel.com> <C37D651A908B024F974696C65296B57B4C74C7E4@SHSMSX101.ccr.corp.intel.com> <20160505035000.GY5641@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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Thu, 05 May 2016 05:29:32 -0000 On 5/5/2016 11:46 AM, Yuanhan Liu wrote:=0A= > On Thu, May 05, 2016 at 03:29:44AM +0000, Xie, Huawei wrote:=0A= >> 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= to=0A= >>>>> remove a common queue_setup function that does common setups, such as= vring=0A= >>>>> memory allocation.=0A= >>>>>=0A= >>>>> This results to much duplicated code: following diff summary also sho= ws=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_set= up()=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 qu= ite=0A= >>> many "if .. else .." as you stated. And that's what you are going to re= solve,=0A= >>> but IMO, you went far: you made __same__ code 3 copies, one for rx, tx = and=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 = worse.=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= >> To your later comment, we could split first, then do the queue setup rew= ork.=0A= > Well, if you insist, I'm Okay. But please don't do it in the way this=0A= > patch does, that introduces quite many duplicated codes.=0A= =0A= Yuanhan, I have no insist.=0A= =0A= Our target is 1) remove the queue type if else checking in the=0A= virtio_dev_queue_setup 2) extract the common setup for vq and call them=0A= in specific RX/TX/CQ setup.=0A= For 2, which is really meaningful to me is the queue size retrieve,=0A= queue allocation=0A= =0A= What I mean is firstly we split the queue, without breaking the common=0A= setup; then introduce RX/TX specific setup calling extracted common=0A= setup, so we don't have a chance to introduce duplicated code.=0A= =0A= =0A= > --yliu=0A= >=0A= =0A=