[dpdk-dev] [PATCH 1/4] ixgbe: 512 entries RSS table on x550

Lu, Wenzhuo wenzhuo.lu at intel.com
Fri Oct 16 03:15:22 CEST 2015


Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, October 16, 2015 6:19 AM
> To: Lu, Wenzhuo; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/4] ixgbe: 512 entries RSS table on x550
> 
> Hi Wenzhuo,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Wenzhuo Lu
> > Sent: Monday, September 28, 2015 8:52 AM
> > To: dev at dpdk.org
> > Subject: [dpdk-dev] [PATCH 1/4] ixgbe: 512 entries RSS table on x550
> >
> > Comparing with the older NICs, x550's RSS redirection table is
> > enlarged to 512 entries. As the original code is for the NICs which
> > have a 128 entries RSS table, it means only part of the RSS table is
> > set on x550. So, RSS cannot work as expected on x550, it doesn't redirect the
> packets evenly.
> > This patch configs the entries beyond 128 on x550 to let RSS work
> > well, and also update the query and update functions to support 512 entries.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 106
> ++++++++++++++++++++++++++++++++++-----
> >  drivers/net/ixgbe/ixgbe_rxtx.c   |  18 +++++--
> >  2 files changed, 109 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index ec2918c..a1ef26f 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -2397,7 +2397,17 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *dev_info)
> >  				ETH_TXQ_FLAGS_NOOFFLOADS,
> >  	};
> >  	dev_info->hash_key_size = IXGBE_HKEY_MAX_INDEX * sizeof(uint32_t);
> > -	dev_info->reta_size = ETH_RSS_RETA_SIZE_128;
> > +
> > +	switch (hw->mac.type) {
> > +	case ixgbe_mac_X550:
> > +	case ixgbe_mac_X550EM_x:
> > +		dev_info->reta_size = ETH_RSS_RETA_SIZE_512;
> > +		break;
> > +	default:
> > +		dev_info->reta_size = ETH_RSS_RETA_SIZE_128;
> > +		break;
> > +	}
> > +
> 
> Instead of adding switch(hw->mac_type) in dozen of places, why not to create a
> new function: get_reta_size(hw) and use it whenever it is needed?
Thanks for the comments. I'll create a function for it.

> 
> >  	dev_info->flow_type_rss_offloads = IXGBE_RSS_OFFLOAD_ALL;  }
> >
> > @@ -3205,14 +3215,29 @@ ixgbe_dev_rss_reta_update(struct rte_eth_dev
> *dev,
> >  	struct ixgbe_hw *hw =
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> >  	PMD_INIT_FUNC_TRACE();
> > -	if (reta_size != ETH_RSS_RETA_SIZE_128) {
> > -		PMD_DRV_LOG(ERR, "The size of hash lookup table configured
> "
> > -			"(%d) doesn't match the number hardware can
> supported "
> > -			"(%d)\n", reta_size, ETH_RSS_RETA_SIZE_128);
> > -		return -EINVAL;
> > +	switch (hw->mac.type) {
> > +	case ixgbe_mac_X550:
> > +	case ixgbe_mac_X550EM_x:
> > +		if (reta_size != ETH_RSS_RETA_SIZE_512) {
> > +			PMD_DRV_LOG(ERR, "The size of hash lookup table "
> > +					"configured (%d) doesn't match the "
> > +					"number hardware can supported
> (%d)\n",
> > +					reta_size, ETH_RSS_RETA_SIZE_512);
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		if (reta_size != ETH_RSS_RETA_SIZE_128) {
> > +			PMD_DRV_LOG(ERR, "The size of hash lookup table "
> > +					"configured (%d) doesn't match the "
> > +					"number hardware can supported
> (%d)\n",
> > +					reta_size, ETH_RSS_RETA_SIZE_128);
> > +			return -EINVAL;
> > +		}
> > +		break;
> >  	}
> >
> > -	for (i = 0; i < reta_size; i += IXGBE_4_BIT_WIDTH) {
> > +	for (i = 0; i < ETH_RSS_RETA_SIZE_128; i += IXGBE_4_BIT_WIDTH) {
> >  		idx = i / RTE_RETA_GROUP_SIZE;
> >  		shift = i % RTE_RETA_GROUP_SIZE;
> >  		mask = (uint8_t)((reta_conf[idx].mask >> shift) & @@ -3234,6
> > +3259,30 @@ ixgbe_dev_rss_reta_update(struct rte_eth_dev *dev,
> >  		IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
> >  	}
> >
> > +	for (i = ETH_RSS_RETA_SIZE_128; i < reta_size; i +=
> IXGBE_4_BIT_WIDTH) {
> > +		idx = i / RTE_RETA_GROUP_SIZE;
> > +		shift = i % RTE_RETA_GROUP_SIZE;
> > +		mask = (uint8_t)((reta_conf[idx].mask >> shift) &
> > +						IXGBE_4_BIT_MASK);
> > +		if (!mask)
> > +			continue;
> > +		if (mask == IXGBE_4_BIT_MASK)
> > +			r = 0;
> > +		else
> > +			r = IXGBE_READ_REG(hw,
> > +				IXGBE_ERETA((i - ETH_RSS_RETA_SIZE_128) >>
> 2));
> > +		for (j = 0, reta = 0; j < IXGBE_4_BIT_WIDTH; j++) {
> > +			if (mask & (0x1 << j))
> > +				reta |= reta_conf[idx].reta[shift + j] <<
> > +							(CHAR_BIT * j);
> > +			else
> > +				reta |= r & (IXGBE_8_BIT_MASK <<
> > +						(CHAR_BIT * j));
> > +		}
> > +		IXGBE_WRITE_REG(hw,
> > +			IXGBE_ERETA((i - ETH_RSS_RETA_SIZE_128) >> 2), reta);
> > +	}
> > +
> 
> Is there any good reason to create a second loop and duplicate loops body?
> Why not just:
> 
> if (reta_size != get_reta_size(hw) {return -EINVAL;} for (i=0; I != reta_size; i+=...)
> {...} ?
Because the different registers are used. I don't want add a judgement in the loop.
But this's not a performance sensitive scenario, so, It's also OK to merge them.
The same for the similar comments below.

> 
> >  	return 0;
> >  }
> >
> > @@ -3248,11 +3297,26 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev
> *dev,
> >  	struct ixgbe_hw *hw =
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> >  	PMD_INIT_FUNC_TRACE();
> > -	if (reta_size != ETH_RSS_RETA_SIZE_128) {
> > -		PMD_DRV_LOG(ERR, "The size of hash lookup table configured
> "
> > -			"(%d) doesn't match the number hardware can
> supported "
> > -				"(%d)\n", reta_size, ETH_RSS_RETA_SIZE_128);
> > -		return -EINVAL;
> > +	switch (hw->mac.type) {
> > +	case ixgbe_mac_X550:
> > +	case ixgbe_mac_X550EM_x:
> > +		if (reta_size != ETH_RSS_RETA_SIZE_512) {
> > +			PMD_DRV_LOG(ERR, "The size of hash lookup table "
> > +					" configured (%d) doesn't match the "
> > +					"number hardware can supported
> (%d)\n",
> > +					reta_size, ETH_RSS_RETA_SIZE_512);
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		if (reta_size != ETH_RSS_RETA_SIZE_128) {
> > +			PMD_DRV_LOG(ERR, "The size of hash lookup table "
> > +					" configured (%d) doesn't match the "
> > +					"number hardware can supported
> (%d)\n",
> > +					reta_size, ETH_RSS_RETA_SIZE_128);
> > +			return -EINVAL;
> > +		}
> > +		break;
> >  	}
> >
> >  	for (i = 0; i < ETH_RSS_RETA_SIZE_128; i += IXGBE_4_BIT_WIDTH) { @@
> > -3272,6 +3336,24 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev,
> >  		}
> >  	}
> >
> > +	for (i = ETH_RSS_RETA_SIZE_128; i < reta_size; i +=
> IXGBE_4_BIT_WIDTH) {
> > +		idx = i / RTE_RETA_GROUP_SIZE;
> > +		shift = i % RTE_RETA_GROUP_SIZE;
> > +		mask = (uint8_t)((reta_conf[idx].mask >> shift) &
> > +						IXGBE_4_BIT_MASK);
> > +		if (!mask)
> > +			continue;
> > +
> > +		reta = IXGBE_READ_REG(hw,
> > +				IXGBE_ERETA((i - ETH_RSS_RETA_SIZE_128) >>
> 2));
> > +		for (j = 0; j < IXGBE_4_BIT_WIDTH; j++) {
> > +			if (mask & (0x1 << j))
> > +				reta_conf[idx].reta[shift + j] =
> > +					((reta >> (CHAR_BIT * j)) &
> > +						IXGBE_8_BIT_MASK);
> > +		}
> > +	}
> > +
> 
> Same as above - don't see any need to create an extra loop with identical body.
> Pls merge them into one.
> 
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index a598a72..a746ae7 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -2790,7 +2790,7 @@ ixgbe_rss_configure(struct rte_eth_dev *dev)  {
> >  	struct rte_eth_rss_conf rss_conf;
> >  	struct ixgbe_hw *hw;
> > -	uint32_t reta;
> > +	uint32_t reta = 0;
> >  	uint16_t i;
> >  	uint16_t j;
> >
> > @@ -2802,8 +2802,7 @@ ixgbe_rss_configure(struct rte_eth_dev *dev)
> >  	 * The byte-swap is needed because NIC registers are in
> >  	 * little-endian order.
> >  	 */
> > -	reta = 0;
> 
> What was wrong with that one?
I just init it where it's defined.

> 
> > -	for (i = 0, j = 0; i < 128; i++, j++) {
> > +	for (i = 0, j = 0; i < ETH_RSS_RETA_SIZE_128; i++, j++) {
> >  		if (j == dev->data->nb_rx_queues)
> >  			j = 0;
> >  		reta = (reta << 8) | j;
> > @@ -2812,6 +2811,19 @@ ixgbe_rss_configure(struct rte_eth_dev *dev)
> >  					rte_bswap32(reta));
> >  	}
> >
> > +	if ((hw->mac.type == ixgbe_mac_X550) ||
> > +		(hw->mac.type == ixgbe_mac_X550EM_x)) {
> > +		for (i = 0, j = 0; i < (ETH_RSS_RETA_SIZE_512 -
> ETH_RSS_RETA_SIZE_128);
> > +			i++, j++) {
> > +			if (j == dev->data->nb_rx_queues)
> > +				j = 0;
> > +			reta = (reta << 8) | j;
> > +			if ((i & 3) == 3)
> > +				IXGBE_WRITE_REG(hw, IXGBE_ERETA(i >> 2),
> > +						rte_bswap32(reta));
> > +		}
> > +	}
> > +
> 
> Same as above.
> Seems no need to have 2 identical loops, pls merge into one.
> 
> Konstantin
> 
> >  	/*
> >  	 * Configure the RSS key and the RSS protocols used to compute
> >  	 * the RSS hash of input packets.
> > --
> > 1.9.3



More information about the dev mailing list