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 30B2E7E70 for ; Wed, 15 Oct 2014 08:51:40 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 14 Oct 2014 23:49:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,722,1406617200"; d="scan'208";a="605584226" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga001.fm.intel.com with ESMTP; 14 Oct 2014 23:59:26 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 14 Oct 2014 23:59:26 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.192]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.190]) with mapi id 14.03.0195.001; Wed, 15 Oct 2014 14:59:24 +0800 From: "Chen, Jing D" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support Thread-Index: AQHP1zBT6w/kKa8rS0Kz9pa3obOE+pwvPHsAgAGakvA= Date: Wed, 15 Oct 2014 06:59:24 +0000 Message-ID: <4341B239C0EFF9468EE453F9E9F4604D015F35FE@shsmsx102.ccr.corp.intel.com> References: <1411478047-1251-1-git-send-email-jing.d.chen@intel.com> <1411478047-1251-2-git-send-email-jing.d.chen@intel.com> <20784375.UWMeVtoV91@xps13> In-Reply-To: <20784375.UWMeVtoV91@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 06:51:40 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Tuesday, October 14, 2014 10:10 PM > To: Chen, Jing D > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support >=20 > 2014-09-23 21:14, Chen Jing D: > > The change includes several parts: > > 1. Clear pool bitmap when trying to remove specific MAC. > > 2. Define RSS, DCB and VMDQ flags to combine rx_mq_mode. > > 3. Use 'struct' to replace 'union', which to expand the rx_adv_conf > > arguments to better support RSS, DCB and VMDQ. > > 4. Fix bug in rte_eth_dev_config_restore function, which will restore > > all MAC address to default pool. > > 5. Define additional 3 arguments for better VMDQ support. > > > > Signed-off-by: Chen Jing D(Mark) > > Acked-by: Konstantin Ananyev > > Acked-by: Jingjing Wu > > Acked-by: Jijiang Liu > > Acked-by: Huawei Xie >=20 > Whaou, there were a lot of reviewers! > The patch should be really clean. Let's see :) First time I saw you are so humorous. :) >=20 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > /* add address to the hardware */ > > - if (*dev->dev_ops->mac_addr_add) > > + if (*dev->dev_ops->mac_addr_add && > > + dev->data->mac_pool_sel[i] & (1ULL << pool)) > > (*dev->dev_ops->mac_addr_add)(dev, &addr, i, > pool); >=20 > > + /* Update pool bitmap in NIC data structure */ > > + dev->data->mac_pool_sel[index] =3D 0; >=20 > Reset is a better word than "Update" in this case. > But do we really need a comment for that? Accept. >=20 > > +#define ETH_MQ_RX_RSS_FLAG 0x1 > > +#define ETH_MQ_RX_DCB_FLAG 0x2 > > +#define ETH_MQ_RX_VMDQ_FLAG 0x4 >=20 > Need a comment to know where these flags can be used. Accept. >=20 > > 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, > > }; >=20 > Why not simply remove all these combinations and keep only flags? > Please keep it simple. One reason is back-compatibility.=20 Another reason is not all NIC driver support all the combined modes, only l= imited sets driver supported. Under this condition, it's better to use the combination = definition=20 (VMDQ_DCB, DCB_RSS, etc) to let driver check whether it supports. =09 >=20 > > + /**< Specify the queue range belongs to VMDQ pools if VMDQ > applicable */ > > + uint16_t vmdq_queue_base; > > + uint16_t vmdq_queue_num; >=20 > If comment is before, it should be /** not /**<. Accept. >=20 > > + uint16_t vmdq_pool_base; /** < Specify the start pool ID of VMDQ > pools */ >=20 > There is a typo with the space --^ > Please, when writing comments, ask yourself if each word is required > and how it can be shorter. > Example here: /**< first ID of VMDQ pools */ >=20 > Conclusion: NACK > There are only few typos and minor things but it would help to have more > careful reviews. Having a list of people at the beginning of the patch > didn't help in this case. I listed all the code reviewers out to reduce their workload to reply the e= mail, not mean to make it easier to be applied. >=20 > Thanks for your attention > -- > Thomas