Re: [PATCH V5 1/7] app/testpmd: fix supported RSS offload display

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

On 6/25/2022 3:12 AM, lihuisong (C) wrote:
> 
> 在 2022/6/24 21:01, Ferruh Yigit 写道:
>> On 6/24/2022 8:23 AM, Huisong Li wrote:
>>>
>>> The 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 for i40e or ixgbe. This patch
>>> uses the RTE_ETH_RSS_* bits to display supported RSS offload of PMD.
>>>
>>> In addition, offloads that are not in rss_type_table[] should be 
>>> displayed
>>> as "unknown offload xxx", instead of "user defined 63". So this patch 
>>> fixes
>>> it.
>>>
>>
>> There is something as "user defined" RSS type, so please keep it as it 
>> is.
>> For more details please check:
>> Commit 8b94c81e3341 ("app/testpmd: port info prints dynamically mapped 
>> flow types")
>> Commit 5a4806d304e0 ("app/testpmd: support updating pctype mapping")
>>
>> Simply this is to allow doing RSS on user defined protocols, supported 
>> by plugging like Intel DDP.
> All right.
>>
>>> 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  | 40 ++++++++++++++++++++++++++--------------
>>>   app/test-pmd/testpmd.h |  2 ++
>>>   2 files changed, 28 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 62833fe97c..36a828307c 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -66,8 +66,6 @@
>>>
>>>   #define NS_PER_SEC 1E9
>>>
>>> -static char *flowtype_to_str(uint16_t flow_type);
>>> -
>>>   static const struct {
>>>          enum tx_pkt_split split;
>>>          const char *name;
>>> @@ -675,6 +673,19 @@ print_dev_capabilities(uint64_t capabilities)
>>>          }
>>>   }
>>>
>>> +const char *
>>> +rsstypes_to_str(uint64_t rss_type)
>>> +{
>>> +       uint16_t i;
>>> +
>>> +       for (i = 0; rss_type_table[i].str != NULL; i++) {
>>> +               if (rss_type_table[i].rss_type == rss_type)
>>> +                       return rss_type_table[i].str;
>>> +       }
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>>   void
>>>   port_infos_display(portid_t port_id)
>>>   {
>>> @@ -779,19 +790,20 @@ 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);
>>
>> This logic is wrong, as we talked before some RSS types can be 
>> multiple bit, with about logic you can't catch them.
>>
>> The logic in the V2 of this set [1] is correct, which walks through 
>> 'rss_type_table[]' array and check if any value in that array exists 
>> in 'flow_type_rss_offloads'.
>>
>> [1]
>> https://patchwork.dpdk.org/project/dpdk/patch/20220425092523.52338-2-lihuisong@huawei.com/ 
>>
> Here is what I think. They have different purposes. The logic of current 
> patch
> is to retain the original display behavior that is single bit offload and
> "user defined xx". However, the logic in the V2 has changed the behavior.
> I don't think this patch should change its original behavior. And it is 
> better
> to print offload by single bit. In this way, the parsed offload 
> capability is
> more accurate and convenient to use.

OK, lets keep original behavior.
Since 'rss_type_table[]' array has both single bit and combined bit 
entries, same array can work for both purposes.

>>
>>> +                       if ((rss_offload_types & rss_offload) != 0) {
>>> +                               const char *p = 
>>> rsstypes_to_str(rss_offload);
>>> +                               if (p)
>>> +                                       printf("  %s\n", p);
>>> +                               else
>>> +                                       printf(" 
>>> unknown_offload(BIT(%u))\n",
>>> +                                              i);
>>> +                       }
>>>                  }
>>>          }
>>>
>>> @@ -5604,6 +5616,8 @@ set_record_burst_stats(uint8_t on_off)
>>>          record_burst_stats = on_off;
>>>   }
>>>
>>> +#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
>>> +
>>>   static char*
>>>   flowtype_to_str(uint16_t flow_type)
>>>   {
>>> @@ -5647,8 +5661,6 @@ flowtype_to_str(uint16_t flow_type)
>>>          return NULL;
>>>   }
>>>
>>> -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
>>> -
>>>   static inline void
>>>   print_fdir_mask(struct rte_eth_fdir_masks *mask)
>>>   {
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index eeefb5e70f..195488b602 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -1199,6 +1199,8 @@ extern int flow_parse(const char *src, void 
>>> *result, unsigned int size,
>>>                        struct rte_flow_item **pattern,
>>>                        struct rte_flow_action **actions);
>>>
>>> +const char *rsstypes_to_str(uint64_t rss_type);
>>> +
>>>   /* For registering driver specific testpmd commands. */
>>>   struct testpmd_driver_commands {
>>>          TAILQ_ENTRY(testpmd_driver_commands) next;
>>> -- 
>>> 2.33.0
>>>
>>
>> .