From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <jing.d.chen@intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
by dpdk.org (Postfix) with ESMTP id 4AEF47E93
for <dev@dpdk.org>; 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" <jing.d.chen@intel.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
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" <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 <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: 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