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

  From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

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) <jing.d.chen@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
Acked-by: Jijiang Liu <jijiang.liu@intel.com>
Acked-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_ether/rte_ethdev.c |   12 +++++++-----
 lib/librte_ether/rte_ethdev.h |   39 ++++++++++++++++++++++++++++-----------
 2 files changed, 35 insertions(+), 16 deletions(-)
  

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) <jing.d.chen@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> Acked-by: Jijiang Liu <jijiang.liu@intel.com>
> Acked-by: Huawei Xie <huawei.xie@intel.com>

Whaou, there were a lot of reviewers!
The patch should be really clean. Let's see :)

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

> +	/* Update pool bitmap in NIC data structure */
> +	dev->data->mac_pool_sel[index] = 0;

Reset is a better word than "Update" in this case.
But do we really need a comment for that?

> +#define ETH_MQ_RX_RSS_FLAG  0x1
> +#define ETH_MQ_RX_DCB_FLAG  0x2
> +#define ETH_MQ_RX_VMDQ_FLAG 0x4

Need a comment to know where these flags can be used.

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

> +	/**< Specify the queue range belongs to VMDQ pools if VMDQ applicable */
> +	uint16_t vmdq_queue_base;
> +	uint16_t vmdq_queue_num;

If comment is before, it should be /** not /**<.

> +	uint16_t vmdq_pool_base; /** < Specify the start pool ID of VMDQ pools */

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

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.

Thanks for your attention
  

> -----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
> 
> 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) <jing.d.chen@intel.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> > Acked-by: Jijiang Liu <jijiang.liu@intel.com>
> > Acked-by: Huawei Xie <huawei.xie@intel.com>
> 
> Whaou, there were a lot of reviewers!
> The patch should be really clean. Let's see :)

First time I saw you are so humorous. :)

> 
> > --- 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);
> 
> > +	/* Update pool bitmap in NIC data structure */
> > +	dev->data->mac_pool_sel[index] = 0;
> 
> Reset is a better word than "Update" in this case.
> But do we really need a comment for that?

Accept.

> 
> > +#define ETH_MQ_RX_RSS_FLAG  0x1
> > +#define ETH_MQ_RX_DCB_FLAG  0x2
> > +#define ETH_MQ_RX_VMDQ_FLAG 0x4
> 
> Need a comment to know where these flags can be used.

Accept.

> 
> >  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. 
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.
	
> 
> > +	/**< Specify the queue range belongs to VMDQ pools if VMDQ
> applicable */
> > +	uint16_t vmdq_queue_base;
> > +	uint16_t vmdq_queue_num;
> 
> If comment is before, it should be /** not /**<.

Accept.

> 
> > +	uint16_t vmdq_pool_base; /** < Specify the start pool ID of VMDQ
> pools */
> 
> 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 */
> 
> 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 email,
not mean to make it easier to be applied.

> 
> Thanks for your attention
> --
> Thomas
  

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.
  

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


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
  

2014-10-15 09:47, Chen, Jing D:
> From: Thomas Monjalon [mailto: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).
[...]
> case ETH_MQ_RX_VMDQ_FLAG |  ETH_MQ_RX_DCB_FLAG:
[...] 
> 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.

I think that defining all combinations of flags is more 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.

If nobody else agree with my point of view, I'll accept yours.
  

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

v2:
- Fix a few typos.
- Add comments for RX mq mode flags.
- Remove '\n' from some log messages.
- Remove 'Acked-by' in commit log.

v1:
Define extra VMDQ arguments to expand VMDQ configuration. This also
includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
defects in rte_ether library.

Add full VMDQ support in i40e PMD driver. renamed some functions, setup
VMDQ VSI after it's enabled in application. It also make some improvement
on macaddr add/delete to support setting multiple macaddr for single or
multiple pools.

Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
configure/switch queues belonging to VMDQ pools.


Chen Jing D(Mark) (6):
  ether: enhancement for VMDQ support
  igb: change for VMDQ arguments expansion
  ixgbe: change for VMDQ arguments expansion
  i40e: add VMDQ support
  i40e: macaddr add/del enhancement
  i40e: Add full VMDQ pools support

 config/common_linuxapp              |    1 +
 lib/librte_ether/rte_ethdev.c       |   12 +-
 lib/librte_ether/rte_ethdev.h       |   43 +++-
 lib/librte_pmd_e1000/igb_ethdev.c   |    3 +
 lib/librte_pmd_i40e/i40e_ethdev.c   |  499 ++++++++++++++++++++++++++---------
 lib/librte_pmd_i40e/i40e_ethdev.h   |   21 ++-
 lib/librte_pmd_i40e/i40e_rxtx.c     |  125 +++++++--
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    1 +
 8 files changed, 536 insertions(+), 169 deletions(-)
  

Tested-by: Min Cao <min.cao@intel.com>

This patch has been verified on fortville and it is ready to be integrated to dpdk.org.

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chen Jing D(Mark)
Sent: Thursday, October 16, 2014 6:07 PM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH v2 0/6] i40e VMDQ support

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

v2:
- Fix a few typos.
- Add comments for RX mq mode flags.
- Remove '\n' from some log messages.
- Remove 'Acked-by' in commit log.

v1:
Define extra VMDQ arguments to expand VMDQ configuration. This also
includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
defects in rte_ether library.

Add full VMDQ support in i40e PMD driver. renamed some functions, setup
VMDQ VSI after it's enabled in application. It also make some improvement
on macaddr add/delete to support setting multiple macaddr for single or
multiple pools.

Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
configure/switch queues belonging to VMDQ pools.


Chen Jing D(Mark) (6):
  ether: enhancement for VMDQ support
  igb: change for VMDQ arguments expansion
  ixgbe: change for VMDQ arguments expansion
  i40e: add VMDQ support
  i40e: macaddr add/del enhancement
  i40e: Add full VMDQ pools support

 config/common_linuxapp              |    1 +
 lib/librte_ether/rte_ethdev.c       |   12 +-
 lib/librte_ether/rte_ethdev.h       |   43 +++-
 lib/librte_pmd_e1000/igb_ethdev.c   |    3 +
 lib/librte_pmd_i40e/i40e_ethdev.c   |  499 ++++++++++++++++++++++++++---------
 lib/librte_pmd_i40e/i40e_ethdev.h   |   21 ++-
 lib/librte_pmd_i40e/i40e_rxtx.c     |  125 +++++++--
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    1 +
 8 files changed, 536 insertions(+), 169 deletions(-)
  

Hi,

Any comments on this patch?

> -----Original Message-----
> From: Chen, Jing D
> Sent: Thursday, October 16, 2014 6:07 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; thomas.monjalon@6wind.com; Chen, Jing D
> Subject: [PATCH v2 0/6] i40e VMDQ support
> 
> From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
> 
> v2:
> - Fix a few typos.
> - Add comments for RX mq mode flags.
> - Remove '\n' from some log messages.
> - Remove 'Acked-by' in commit log.
> 
> v1:
> Define extra VMDQ arguments to expand VMDQ configuration. This also
> includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
> defects in rte_ether library.
> 
> Add full VMDQ support in i40e PMD driver. renamed some functions, setup
> VMDQ VSI after it's enabled in application. It also make some improvement
> on macaddr add/delete to support setting multiple macaddr for single or
> multiple pools.
> 
> Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
> configure/switch queues belonging to VMDQ pools.
> 
> 
> Chen Jing D(Mark) (6):
>   ether: enhancement for VMDQ support
>   igb: change for VMDQ arguments expansion
>   ixgbe: change for VMDQ arguments expansion
>   i40e: add VMDQ support
>   i40e: macaddr add/del enhancement
>   i40e: Add full VMDQ pools support
> 
>  config/common_linuxapp              |    1 +
>  lib/librte_ether/rte_ethdev.c       |   12 +-
>  lib/librte_ether/rte_ethdev.h       |   43 +++-
>  lib/librte_pmd_e1000/igb_ethdev.c   |    3 +
>  lib/librte_pmd_i40e/i40e_ethdev.c   |  499
> ++++++++++++++++++++++++++---------
>  lib/librte_pmd_i40e/i40e_ethdev.h   |   21 ++-
>  lib/librte_pmd_i40e/i40e_rxtx.c     |  125 +++++++--
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    1 +
>  8 files changed, 536 insertions(+), 169 deletions(-)
> 
> --
> 1.7.7.6
  

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

v3:
- Fix comments style.
- Simplify words in comments.
- Add variable defintion for BSD config file.
- Code rebase to latest DPDK repo.

v2:
- Fix a few typos.
- Add comments for RX mq mode flags.
- Remove '\n' from some log messages.
- Remove 'Acked-by' in commit log.

v1:
Define extra VMDQ arguments to expand VMDQ configuration. This also
includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
defects in rte_ether library.

Add full VMDQ support in i40e PMD driver. renamed some functions, setup
VMDQ VSI after it's enabled in application. It also make some improvement
on macaddr add/delete to support setting multiple macaddr for single or
multiple pools.

Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
configure/switch queues belonging to VMDQ pools.


Chen Jing D(Mark) (6):
  ether: enhancement for VMDQ support
  igb: change for VMDQ arguments expansion
  ixgbe: change for VMDQ arguments expansion
  i40e: add VMDQ support
  i40e: macaddr add/del enhancement
  i40e: Add full VMDQ pools support

 config/common_bsdapp                |    1 +
 config/common_linuxapp              |    1 +
 lib/librte_ether/rte_ethdev.c       |    6 +-
 lib/librte_ether/rte_ethdev.h       |   41 ++-
 lib/librte_pmd_e1000/igb_ethdev.c   |    3 +
 lib/librte_pmd_i40e/i40e_ethdev.c   |  498 ++++++++++++++++++++++++++---------
 lib/librte_pmd_i40e/i40e_ethdev.h   |   21 ++-
 lib/librte_pmd_i40e/i40e_rxtx.c     |  125 +++++++--
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    1 +
 9 files changed, 532 insertions(+), 165 deletions(-)
  

> From: Chen, Jing D
> Sent: Tuesday, November 04, 2014 10:01 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; thomas.monjalon@6wind.com; De Lara Guarch, Pablo; Chen, Jing D
> Subject: [PATCH v3 0/6] i40e VMDQ support
> 
> From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
> 
> v3:
> - Fix comments style.
> - Simplify words in comments.
> - Add variable defintion for BSD config file.
> - Code rebase to latest DPDK repo.
> 
> v2:
> - Fix a few typos.
> - Add comments for RX mq mode flags.
> - Remove '\n' from some log messages.
> - Remove 'Acked-by' in commit log.
> 
> v1:
> Define extra VMDQ arguments to expand VMDQ configuration. This also
> includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
> defects in rte_ether library.
> 
> Add full VMDQ support in i40e PMD driver. renamed some functions, setup
> VMDQ VSI after it's enabled in application. It also make some improvement
> on macaddr add/delete to support setting multiple macaddr for single or
> multiple pools.
> 
> Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
> configure/switch queues belonging to VMDQ pools.
> 
> 
> Chen Jing D(Mark) (6):
>   ether: enhancement for VMDQ support
>   igb: change for VMDQ arguments expansion
>   ixgbe: change for VMDQ arguments expansion
>   i40e: add VMDQ support
>   i40e: macaddr add/del enhancement
>   i40e: Add full VMDQ pools support
> 
>  config/common_bsdapp                |    1 +
>  config/common_linuxapp              |    1 +
>  lib/librte_ether/rte_ethdev.c       |    6 +-
>  lib/librte_ether/rte_ethdev.h       |   41 ++-
>  lib/librte_pmd_e1000/igb_ethdev.c   |    3 +
>  lib/librte_pmd_i40e/i40e_ethdev.c   |  498 ++++++++++++++++++++++++++---------
>  lib/librte_pmd_i40e/i40e_ethdev.h   |   21 ++-
>  lib/librte_pmd_i40e/i40e_rxtx.c     |  125 +++++++--
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    1 +
>  9 files changed, 532 insertions(+), 165 deletions(-)
> 
> --
> 1.7.7.6

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
  

> > v3:
> > - Fix comments style.
> > - Simplify words in comments.
> > - Add variable defintion for BSD config file.
> > - Code rebase to latest DPDK repo.
> > 
> > v2:
> > - Fix a few typos.
> > - Add comments for RX mq mode flags.
> > - Remove '\n' from some log messages.
> > - Remove 'Acked-by' in commit log.
> > 
> > v1:
> > Define extra VMDQ arguments to expand VMDQ configuration. This also
> > includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
> > defects in rte_ether library.
> > 
> > Add full VMDQ support in i40e PMD driver. renamed some functions, setup
> > VMDQ VSI after it's enabled in application. It also make some improvement
> > on macaddr add/delete to support setting multiple macaddr for single or
> > multiple pools.
> > 
> > Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
> > configure/switch queues belonging to VMDQ pools.
> > 
> > Chen Jing D(Mark) (6):
> >   ether: enhancement for VMDQ support
> >   igb: change for VMDQ arguments expansion
> >   ixgbe: change for VMDQ arguments expansion
> >   i40e: add VMDQ support
> >   i40e: macaddr add/del enhancement
> >   i40e: Add full VMDQ pools support
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied

It will need to be well explained in the programmer's guide.

Thanks
  

Tested-by: Min Cao <min.cao@intel.com>

Patch name: 		i40e VMDQ support
Brief description: 	
Test Flag: 			Tested-by
Tester name: 		min.cao@intel.com
Result summary:		total 1 cases, 1 passed, 0 failed

Test Case 1:		
Name:				perf_vmdq_performance
Environment:		OS: Fedora20 3.11.10-301.fc20.x86_64
				gcc (GCC) 4.8.2
				CPU: Intel(R) Xeon(R) CPU E5-2680 0 @ 2.70GHz
				NIC: Fortville eagle
Test result:		PASSED

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chen Jing D(Mark)
Sent: Tuesday, November 04, 2014 6:01 PM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH v3 0/6] i40e VMDQ support

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

v3:
- Fix comments style.
- Simplify words in comments.
- Add variable defintion for BSD config file.
- Code rebase to latest DPDK repo.

v2:
- Fix a few typos.
- Add comments for RX mq mode flags.
- Remove '\n' from some log messages.
- Remove 'Acked-by' in commit log.

v1:
Define extra VMDQ arguments to expand VMDQ configuration. This also
includes change in igb and ixgbe PMD driver. In the meanwhile, fix 2
defects in rte_ether library.

Add full VMDQ support in i40e PMD driver. renamed some functions, setup
VMDQ VSI after it's enabled in application. It also make some improvement
on macaddr add/delete to support setting multiple macaddr for single or
multiple pools.

Finally, change i40e rx/tx_queue_setup and dev_start/stop functions to
configure/switch queues belonging to VMDQ pools.


Chen Jing D(Mark) (6):
  ether: enhancement for VMDQ support
  igb: change for VMDQ arguments expansion
  ixgbe: change for VMDQ arguments expansion
  i40e: add VMDQ support
  i40e: macaddr add/del enhancement
  i40e: Add full VMDQ pools support

 config/common_bsdapp                |    1 +
 config/common_linuxapp              |    1 +
 lib/librte_ether/rte_ethdev.c       |    6 +-
 lib/librte_ether/rte_ethdev.h       |   41 ++-
 lib/librte_pmd_e1000/igb_ethdev.c   |    3 +
 lib/librte_pmd_i40e/i40e_ethdev.c   |  498 ++++++++++++++++++++++++++---------
 lib/librte_pmd_i40e/i40e_ethdev.h   |   21 ++-
 lib/librte_pmd_i40e/i40e_rxtx.c     |  125 +++++++--
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    1 +
 9 files changed, 532 insertions(+), 165 deletions(-)