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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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 = 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).

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