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 4AEF47E93 for ; Wed, 15 Oct 2014 11:40:13 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 15 Oct 2014 02:41:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,723,1406617200"; d="scan'208";a="589296607" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga001.jf.intel.com with ESMTP; 15 Oct 2014 02:47:27 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.195.1; Wed, 15 Oct 2014 02:47:27 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.195.1; Wed, 15 Oct 2014 02:47:27 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.192]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.204]) with mapi id 14.03.0195.001; Wed, 15 Oct 2014 17:47:26 +0800 From: "Chen, Jing D" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support Thread-Index: AQHP1zBT6w/kKa8rS0Kz9pa3obOE+pwvPHsAgAGakvD//5OPAIAAmRyw Date: Wed, 15 Oct 2014 09:47:25 +0000 Message-ID: <4341B239C0EFF9468EE453F9E9F4604D015F36E1@shsmsx102.ccr.corp.intel.com> References: <1411478047-1251-1-git-send-email-jing.d.chen@intel.com> <20784375.UWMeVtoV91@xps13> <4341B239C0EFF9468EE453F9E9F4604D015F35FE@shsmsx102.ccr.corp.intel.com> <10317086.OmtuOpvoDk@xps13> In-Reply-To: <10317086.OmtuOpvoDk@xps13> 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 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support 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: Wed, 15 Oct 2014 09:40:13 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Wednesday, October 15, 2014 4:11 PM > To: Chen, Jing D > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support >=20 > 2014-10-15 06:59, Chen, Jing D: > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > > > enum rte_eth_rx_mq_mode { > > > > - ETH_MQ_RX_NONE =3D 0, /**< None of DCB,RSS or VMDQ mode */ > > > > - > > > > - ETH_MQ_RX_RSS, /**< For RX side, only RSS is on */ > > > > - ETH_MQ_RX_DCB, /**< For RX side,only DCB is on. */ > > > > - ETH_MQ_RX_DCB_RSS, /**< Both DCB and RSS enable */ > > > > - > > > > - ETH_MQ_RX_VMDQ_ONLY, /**< Only VMDQ, no RSS nor DCB */ > > > > - ETH_MQ_RX_VMDQ_RSS, /**< RSS mode with VMDQ */ > > > > - ETH_MQ_RX_VMDQ_DCB, /**< Use VMDQ+DCB to route traffic to > > > queues */ > > > > - ETH_MQ_RX_VMDQ_DCB_RSS, /**< Enable both VMDQ and DCB in > > > VMDq */ > > > > + /**< None of DCB,RSS or VMDQ mode */ > > > > + ETH_MQ_RX_NONE =3D 0, > > > > + > > > > + /**< For RX side, only RSS is on */ > > > > + ETH_MQ_RX_RSS =3D ETH_MQ_RX_RSS_FLAG, > > > > + /**< For RX side,only DCB is on. */ > > > > + ETH_MQ_RX_DCB =3D ETH_MQ_RX_DCB_FLAG, > > > > + /**< Both DCB and RSS enable */ > > > > + ETH_MQ_RX_DCB_RSS =3D ETH_MQ_RX_RSS_FLAG | > > > ETH_MQ_RX_DCB_FLAG, > > > > + > > > > + /**< Only VMDQ, no RSS nor DCB */ > > > > + ETH_MQ_RX_VMDQ_ONLY =3D ETH_MQ_RX_VMDQ_FLAG, > > > > + /**< RSS mode with VMDQ */ > > > > + ETH_MQ_RX_VMDQ_RSS =3D ETH_MQ_RX_RSS_FLAG | > > > ETH_MQ_RX_VMDQ_FLAG, > > > > + /**< Use VMDQ+DCB to route traffic to queues */ > > > > + ETH_MQ_RX_VMDQ_DCB =3D ETH_MQ_RX_VMDQ_FLAG | > > > ETH_MQ_RX_DCB_FLAG, > > > > + /**< Enable both VMDQ and DCB in VMDq */ > > > > + ETH_MQ_RX_VMDQ_DCB_RSS =3D ETH_MQ_RX_RSS_FLAG | > > > ETH_MQ_RX_DCB_FLAG | > > > > + ETH_MQ_RX_VMDQ_FLAG, > > > > }; > > > > > > Why not simply remove all these combinations and keep only flags? > > > Please keep it simple. > > > > One reason is back-compatibility. >=20 > I understand but I think we should prefer cleanup. > As there is no way to advertise deprecation of flags, it should be > simply removed. >=20 > > Another reason is not all NIC driver support all the combined modes, on= ly > limited sets > > driver supported. Under this condition, it's better to use the combinat= ion > definition > > (VMDQ_DCB, DCB_RSS, etc) to let driver check whether it supports. >=20 > Driver can do the same checks with simple flags and it's probably simpler > (e.g. a driver which doesn't support VMDQ had no need to check all VMDQ > combinations). Below is an example with the change in ixgbe_dcb_hw_configure(). DCB only=20 can be enabled in case DCB or VMDQ_DCB is selected. Before the change: switch(dev->data->dev_conf.rxmode.mq_mode){ case ETH_MQ_RX_VMDQ_DCB: ..... case ETH_MQ_RX_DCB: ..... default: FAILED. } With the change, it will be: switch(dev->data->dev_conf.rxmode.mq_mode){ case ETH_MQ_RX_VMDQ_FLAG | ETH_MQ_RX_DCB_FLAG: ..... case ETH_MQ_RX_DCB_FLAG: ..... Default: FAILED } Won't it look weird for reading? In fact, it's more complex in rte_eth_dev_= check_mq_mode(), With the change, the code will look weird. In fact, I don't see benefit with the change to old code. New PMD driver ca= n use simple flag while old driver (IXGBE/IGB) can use original definition. >=20 > > > There are only few typos and minor things but it would help to have m= ore > > > careful reviews. Having a list of people at the beginning of the patc= h > > > didn't help in this case. > > > > I listed all the code reviewers out to reduce their workload to reply t= he > email, > > not mean to make it easier to be applied. >=20 > I have no problem with listing of reviewers when submitting patches. > To say more, I prefer you list them by yourself and you add new reviewers > when sending new versions of the patchset. > But I would like reviewers to be more careful. They are especially useful= to > discuss design choices and check typos. > Having reviewer give credits to the patch only if we are confident that t= he > review task is generally seriously achieved. >=20 > -- > Thomas