> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, October 15, 2014 4:11 PM
> To: Chen, Jing D
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support
>
> 2014-10-15 06:59, Chen, Jing D:
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > enum rte_eth_rx_mq_mode {
> > > > - ETH_MQ_RX_NONE = 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 = 0,
> > > > +
> > > > + /**< For RX side, only RSS is on */
> > > > + ETH_MQ_RX_RSS = ETH_MQ_RX_RSS_FLAG,
> > > > + /**< For RX side,only DCB is on. */
> > > > + ETH_MQ_RX_DCB = ETH_MQ_RX_DCB_FLAG,
> > > > + /**< Both DCB and RSS enable */
> > > > + ETH_MQ_RX_DCB_RSS = ETH_MQ_RX_RSS_FLAG |
> > > ETH_MQ_RX_DCB_FLAG,
> > > > +
> > > > + /**< Only VMDQ, no RSS nor DCB */
> > > > + ETH_MQ_RX_VMDQ_ONLY = ETH_MQ_RX_VMDQ_FLAG,
> > > > + /**< RSS mode with VMDQ */
> > > > + ETH_MQ_RX_VMDQ_RSS = ETH_MQ_RX_RSS_FLAG |
> > > ETH_MQ_RX_VMDQ_FLAG,
> > > > + /**< Use VMDQ+DCB to route traffic to queues */
> > > > + ETH_MQ_RX_VMDQ_DCB = ETH_MQ_RX_VMDQ_FLAG |
> > > ETH_MQ_RX_DCB_FLAG,
> > > > + /**< Enable both VMDQ and DCB in VMDq */
> > > > + ETH_MQ_RX_VMDQ_DCB_RSS = 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.
>
> I understand but I think we should prefer cleanup.
> As there is no way to advertise deprecation of flags, it should be
> simply removed.
>
> > Another reason is not all NIC driver support all the combined modes, only
> limited sets
> > driver supported. Under this condition, it's better to use the combination
> definition
> > (VMDQ_DCB, DCB_RSS, etc) to let driver check whether it supports.
>
> 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
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 can
use simple flag while
old driver (IXGBE/IGB) can use original definition.
>
> > > 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
> email,
> > not mean to make it easier to be applied.
>
> 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 the
> review task is generally seriously achieved.
>
> --
> Thomas
