[dpdk-dev] [PATCH 1/6] ether: enhancement for VMDQ support

[Chen, Jing D]

> -----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