Re: [PATCH V3 app/testpmd 1/4] app/testpmd: fix supported RSS offload display

[Ferruh Yigit <ferruh.yigit@xilinx.com>]

On 6/9/2022 4:25 AM, lihuisong (C) wrote:
> CAUTION: This message has originated from an External Source. Please use 
> proper judgment and caution when opening attachments, clicking links, or 
> responding to this email.
> 
> 
> 在 2022/6/7 23:45, Ferruh Yigit 写道:
>> On 6/7/2022 9:32 AM, Huisong Li wrote:
>>
>>>
>>> And rte_eth_dev_info.flow_type_rss_offloads is populated in terms of
>>> RTE_ETH_RSS_* bits. If PMD sets RTE_ETH_RSS_L3_SRC_ONLY to
>>> dev_info->flow_type_rss_offloads. testpmd will display "user defined 63"
>>> when run 'show port info 0'. Because testpmd use flowtype_to_str()
>>> to display the supported RSS offload of PMD. In fact, the function is
>>> used to display flow type in FDIR commands. This patch uses the
>>> RTE_ETH_RSS_* bits to display supported RSS offload of PMD.
>>>
>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>>> ---
>>>   app/test-pmd/config.c | 85 ++++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 75 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 72d2606d19..d290ca3a06 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -86,6 +86,56 @@ static const struct {
>>>          },
>>>   };
>>>
>>> +const struct rss_type_info rss_offload_table[] = {
>>> +       {"ipv4", RTE_ETH_RSS_IPV4},
>>> +       {"ipv4-frag", RTE_ETH_RSS_FRAG_IPV4},
>>> +       {"ipv4-tcp", RTE_ETH_RSS_NONFRAG_IPV4_TCP},
>>> +       {"ipv4-udp", RTE_ETH_RSS_NONFRAG_IPV4_UDP},
>>> +       {"ipv4-sctp", RTE_ETH_RSS_NONFRAG_IPV4_SCTP},
>>> +       {"ipv4-other", RTE_ETH_RSS_NONFRAG_IPV4_OTHER},
>>> +       {"ipv6", RTE_ETH_RSS_IPV6},
>>> +       {"ipv6-frag", RTE_ETH_RSS_FRAG_IPV6},
>>> +       {"ipv6-tcp", RTE_ETH_RSS_NONFRAG_IPV6_TCP},
>>> +       {"ipv6-udp", RTE_ETH_RSS_NONFRAG_IPV6_UDP},
>>> +       {"ipv6-sctp", RTE_ETH_RSS_NONFRAG_IPV6_SCTP},
>>> +       {"ipv6-other", RTE_ETH_RSS_NONFRAG_IPV6_OTHER},
>>> +       {"l2_payload", RTE_ETH_RSS_L2_PAYLOAD},
>>> +       {"ipv6-ex", RTE_ETH_RSS_IPV6_EX},
>>> +       {"ipv6-tcp-ex", RTE_ETH_RSS_IPV6_TCP_EX},
>>> +       {"ipv6-udp-ex", RTE_ETH_RSS_IPV6_UDP_EX},
>>> +       {"port", RTE_ETH_RSS_PORT},
>>> +       {"vxlan", RTE_ETH_RSS_VXLAN},
>>> +       {"geneve", RTE_ETH_RSS_GENEVE},
>>> +       {"nvgre", RTE_ETH_RSS_NVGRE},
>>> +       {"gtpu", RTE_ETH_RSS_GTPU},
>>> +       {"eth", RTE_ETH_RSS_ETH},
>>> +       {"s-vlan", RTE_ETH_RSS_S_VLAN},
>>> +       {"c-vlan", RTE_ETH_RSS_C_VLAN},
>>> +       {"esp", RTE_ETH_RSS_ESP},
>>> +       {"ah", RTE_ETH_RSS_AH},
>>> +       {"l2tpv3", RTE_ETH_RSS_L2TPV3},
>>> +       {"pfcp", RTE_ETH_RSS_PFCP},
>>> +       {"pppoe", RTE_ETH_RSS_PPPOE},
>>> +       {"ecpri", RTE_ETH_RSS_ECPRI},
>>> +       {"mpls", RTE_ETH_RSS_MPLS},
>>> +       {"ipv4-chksum", RTE_ETH_RSS_IPV4_CHKSUM},
>>> +       {"l4-chksum", RTE_ETH_RSS_L4_CHKSUM},
>>> +       {"l2tpv2", RTE_ETH_RSS_L2TPV2},
>>> +       {"l3-pre96", RTE_ETH_RSS_L3_PRE96},
>>> +       {"l3-pre64", RTE_ETH_RSS_L3_PRE64},
>>> +       {"l3-pre56", RTE_ETH_RSS_L3_PRE56},
>>> +       {"l3-pre48", RTE_ETH_RSS_L3_PRE48},
>>> +       {"l3-pre40", RTE_ETH_RSS_L3_PRE40},
>>> +       {"l3-pre32", RTE_ETH_RSS_L3_PRE32},
>>> +       {"l2-dst-only", RTE_ETH_RSS_L2_DST_ONLY},
>>> +       {"l2-src-only", RTE_ETH_RSS_L2_SRC_ONLY},
>>> +       {"l4-dst-only", RTE_ETH_RSS_L4_DST_ONLY},
>>> +       {"l4-src-only", RTE_ETH_RSS_L4_SRC_ONLY},
>>> +       {"l3-dst-only", RTE_ETH_RSS_L3_DST_ONLY},
>>> +       {"l3-src-only", RTE_ETH_RSS_L3_SRC_ONLY},
>>> +       {NULL, 0},
>>> +};
>>> +
>>
>> Hi Huisong,
>>
>> Why not reusing existing 'rss_type_table[]', but adding a new one?
>> Is it to have each individual RSS type instead of grouping
>> 'rss_type_table[]' has? If so, since command "port config all rss ..."
>> using the grouping, I think it makes sense to have grouping.
>>
> The 'rss_offload_table[]' includes all defined RSS offloads in ethdev
> layer, every iterm has one bit,
> and is part of 'rss_type_table[]'. Some iterms in 'rss_type_table[]'
> consist of multiple bits. If we
> want to display all RSS offloads PMD supported and resolve undefined
> offload, we have to check
> 'flow_type_rss_offloads' bit by bit. However, "port config all rss ..."
> and "show port 0 rss-hash"
> commands supports the configuration and display of multiple offload bits
> at a time. So it is necessary
> to introduce this 'rss_offload_table[]' to display
> 'flow_type_rss_offloads'. That's what I think.

'rss_type_table[]' has both group and individual types, so show 
functions displays both.

A sample of grouping is UDP, a few UDP flags compined under UDP,
#define RTE_ETH_RSS_UDP ( \
         RTE_ETH_RSS_NONFRAG_IPV4_UDP | \
         RTE_ETH_RSS_NONFRAG_IPV6_UDP | \
         RTE_ETH_RSS_IPV6_UDP_EX)

Above mentioned commands use 'RTE_ETH_RSS_UDP' to set "udp" type RSS, 
and when displying instead of displaying each above type separately, I 
think displaying as "udp" makes sense.

And again I believe having two different arrays is prone to error for 
future.

>> Another thing is having two different array with very similar content
>> is easy to confuse and can cause diversion on arrays and generate bugs.
>>
> Add some notes about the use of the 'rss_offload_table[]'?
>>
>> As mentioned from "port config all rss ..." command, it seems it is
>> also not using 'rss_type_table[]', but all string to RSS type matching
>> done within the function ('cmd_config_rss_parsed()') duplicating what
>> we have in 'rss_type_table[]'. Again this has concern to diverge
>> between set and show functions.
>> If you have time for it, can you make an additional patch to update
>> 'cmd_config_rss_parsed()' to use new 'str_to_rsstypes()' function?
> All right. Let's fix it.

Thanks, and if you want it can be a separate patch on top of this one, 
it doesn't have to be part of this set if you don't have time for it.

>>
>> Thanks,
>> ferruh
>>
>>>   const struct rss_type_info rss_type_table[] = {
>>>          { "all", RTE_ETH_RSS_ETH | RTE_ETH_RSS_VLAN | RTE_ETH_RSS_IP
>>> | RTE_ETH_RSS_TCP |
>>>                  RTE_ETH_RSS_UDP | RTE_ETH_RSS_SCTP |
>>> RTE_ETH_RSS_L2_PAYLOAD |
>>> @@ -675,6 +725,19 @@ print_dev_capabilities(uint64_t capabilities)
>>>          }
>>>   }
>>>
>>> +static const char *
>>> +rss_offload_to_str(uint64_t rss_offload)
>>> +{
>>> +       uint16_t i;
>>> +
>>> +       for (i = 0; rss_offload_table[i].str != NULL; i++) {
>>> +               if (rss_offload_table[i].rss_type == rss_offload)
>>> +                       return rss_offload_table[i].str;
>>> +       }
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>>   void
>>>   port_infos_display(portid_t port_id)
>>>   {
>>> @@ -779,19 +842,21 @@ port_infos_display(portid_t port_id)
>>>          if (!dev_info.flow_type_rss_offloads)
>>>                  printf("No RSS offload flow type is supported.\n");
>>>          else {
>>> +               uint64_t rss_offload_types =
>>> dev_info.flow_type_rss_offloads;
>>>                  uint16_t i;
>>> -               char *p;
>>>
>>>                  printf("Supported RSS offload flow types:\n");
>>> -               for (i = RTE_ETH_FLOW_UNKNOWN + 1;
>>> -                    i < sizeof(dev_info.flow_type_rss_offloads) *
>>> CHAR_BIT; i++) {
>>> -                       if (!(dev_info.flow_type_rss_offloads & (1ULL
>>> << i)))
>>> -                               continue;
>>> -                       p = flowtype_to_str(i);
>>> -                       if (p)
>>> -                               printf("  %s\n", p);
>>> -                       else
>>> -                               printf("  user defined %d\n", i);
>>> +               for (i = 0; i < sizeof(rss_offload_types) * CHAR_BIT;
>>> i++) {
>>> +                       uint64_t rss_offload = RTE_BIT64(i);
>>> +                       if ((rss_offload_types & rss_offload) != 0) {
>>> +                               const char *p =
>>> + rss_offload_to_str(rss_offload);
>>> +                               if (p)
>>> +                                       printf("  %s\n", p);
>>> +                               else
>>> +                                       printf("  user defined
>>> 0x%"PRIx64"\n",
>>> +                                              rss_offload);
>>> +                       }
>>
>> If you go with 'rss_type_table[]', you need to change above logic as
>> you did in your v2.
>>
>>>                  }
>>>          }
>>>
>>> -- 
>>> 2.33.0
>>>
>>
>> .