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

[Ananyev, Konstantin]

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?

>  	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+=...) {...}
?  

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

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