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
