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
