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

["lihuisong (C)" <lihuisong@huawei.com>]

在 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.
>
>> +                       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
>>
>
> .