diff mbox series

[1/3] app/testpmd: fix displaying RSS info

Message ID 20220525173736.3394787-1-ferruh.yigit@xilinx.com (mailing list archive)
State Changes Requested
Delegated to: Andrew Rybchenko
Headers show
Series [1/3] app/testpmd: fix displaying RSS info | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Ferruh Yigit May 25, 2022, 5:37 p.m. UTC
When supported RSS offload flow types are printed via 'show port info #'
command, flow names are get from flow type array which is wrong and
causing some RSS flow types not being displayed.

Instead RSS flow type array should be used. Also helper functions added
and existing code updated to use helpers.

Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
---
Cc: helin.zhang@intel.com

Note:
In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
to check that too.
---
 app/test-pmd/config.c | 92 ++++++++++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 28 deletions(-)

Comments

lihuisong (C) May 26, 2022, 3:52 a.m. UTC | #1
Hi, Ferruh

This patch is the same as the following patch:

http://patches.dpdk.org/project/dpdk/patch/20220429102445.23711-2-lihuisong@huawei.com/

It was delegated to you.😂


在 2022/5/26 1:37, Ferruh Yigit 写道:
> When supported RSS offload flow types are printed via 'show port info #'
> command, flow names are get from flow type array which is wrong and
> causing some RSS flow types not being displayed.
>
> Instead RSS flow type array should be used. Also helper functions added
> and existing code updated to use helpers.
>
> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
> ---
> Cc: helin.zhang@intel.com
>
> Note:
> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
> to check that too.
> ---
>   app/test-pmd/config.c | 92 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 64 insertions(+), 28 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 72d2606d19d5..c353d224ef06 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>   	{ NULL, 0 },
>   };
>   
> +static const char *
> +rsstype_to_str(uint64_t rss_type)
> +{
> +	int 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;
> +}
> +
> +static uint64_t
> +str_to_rsstype(const char *str)
> +{
> +	int i;
> +
> +	for (i = 0; rss_type_table[i].str != NULL; i++) {
> +		if (!strcmp(rss_type_table[i].str, str))
> +			return rss_type_table[i].rss_type;
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct {
>   	enum rte_eth_fec_mode mode;
>   	const char *name;
> @@ -779,19 +805,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_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; rss_types != 0; i++) {
> +			if (rss_types & 1) {
> +				uint64_t rss_type = 1ULL << i;
> +				const char *p = rsstype_to_str(rss_type);
> +
> +				if (p)
> +					printf("  %s\n", p);
> +				else
> +					printf("  user defined 0x%"PRIx64"\n", rss_type);
> +			}
> +			rss_types >>= 1;
>   		}
>   	}
>   
> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>   static void
>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>   {
> +	uint64_t rss_types;
>   	uint8_t i;
>   
>   	if (rss_conf == NULL) {
> @@ -1582,16 +1611,23 @@ rss_config_display(struct rte_flow_action_rss *rss_conf)
>   	}
>   
>   	printf(" types:\n");
> -	if (rss_conf->types == 0) {
> +	rss_types = rss_conf->types;
> +	if (rss_types == 0) {
>   		printf("  none\n");
>   		return;
>   	}
> -	for (i = 0; rss_type_table[i].str; i++) {
> -		if ((rss_conf->types &
> -		    rss_type_table[i].rss_type) ==
> -		    rss_type_table[i].rss_type &&
> -		    rss_type_table[i].rss_type != 0)
> -			printf("  %s\n", rss_type_table[i].str);
> +
> +	for (i = 0; rss_types != 0; i++) {
> +		if (rss_types & 1) {
> +			uint64_t rss_type = 1ULL << i;
> +			const char *p = rsstype_to_str(rss_type);
> +
> +			if (p)
> +				printf("  %s\n", p);
> +			else
> +				printf("  user defined 0x%"PRIx64"\n", rss_type);
> +		}
> +		rss_types >>= 1;
>   	}
>   }
>   
> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id, int show_rss_key)
>   		return;
>   	}
>   	printf("RSS functions:\n ");
> -	for (i = 0; rss_type_table[i].str; i++) {
> -		if (rss_type_table[i].rss_type == 0)
> -			continue;
> -		if ((rss_hf & rss_type_table[i].rss_type) == rss_type_table[i].rss_type)
> -			printf("%s ", rss_type_table[i].str);
> +	for (i = 0; rss_hf != 0; i++) {
> +		if (rss_hf & 1) {
> +			uint64_t rss_type = 1ULL << i;
> +			const char *p = rsstype_to_str(rss_type);
> +			if (p)
> +				printf("%s ", p);
> +			else
> +				printf("0x%"PRIx64" ", rss_type);
> +		}
> +		rss_hf >>= 1;
>   	}
>   	printf("\n");
>   	if (!show_rss_key)
> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>   {
>   	struct rte_eth_rss_conf rss_conf;
>   	int diag;
> -	unsigned int i;
>   
>   	rss_conf.rss_key = NULL;
>   	rss_conf.rss_key_len = 0;
> -	rss_conf.rss_hf = 0;
> -	for (i = 0; rss_type_table[i].str; i++) {
> -		if (!strcmp(rss_type_table[i].str, rss_type))
> -			rss_conf.rss_hf = rss_type_table[i].rss_type;
> -	}
> +	rss_conf.rss_hf = str_to_rsstype(rss_type);
>   	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>   	if (diag == 0) {
>   		rss_conf.rss_key = hash_key;
Ferruh Yigit May 26, 2022, 8:56 a.m. UTC | #2
On 5/26/2022 4:52 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.
> 
> 
> Hi, Ferruh
> 
> This patch is the same as the following patch:
> 
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20220429102445.23711-2-lihuisong%40huawei.com%2F&amp;data=05%7C01%7CFerruh.Yigit%40amd.com%7Cda898bd1b39b438ce3e308da3ecb3123%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637891340148254654%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=kixRff%2BQavl0SYS7woT5YYJELrrocoSoV4sE7TUK9Is%3D&amp;reserved=0 
> 
> 
> It was delegated to you.😂
> 

Yes indeed, I missed it. They are fixing same issue.
But there is slight difference in the implementation, can you please 
review the set? We can share the sign off for final set.

> 
> 在 2022/5/26 1:37, Ferruh Yigit 写道:
>> When supported RSS offload flow types are printed via 'show port info #'
>> command, flow names are get from flow type array which is wrong and
>> causing some RSS flow types not being displayed.
>>
>> Instead RSS flow type array should be used. Also helper functions added
>> and existing code updated to use helpers.
>>
>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>> ---
>> Cc: helin.zhang@intel.com
>>
>> Note:
>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
>> to check that too.
>> ---
>>   app/test-pmd/config.c | 92 ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 72d2606d19d5..c353d224ef06 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>       { NULL, 0 },
>>   };
>>
>> +static const char *
>> +rsstype_to_str(uint64_t rss_type)
>> +{
>> +     int 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;
>> +}
>> +
>> +static uint64_t
>> +str_to_rsstype(const char *str)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>> +             if (!strcmp(rss_type_table[i].str, str))
>> +                     return rss_type_table[i].rss_type;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>   static const struct {
>>       enum rte_eth_fec_mode mode;
>>       const char *name;
>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>> +                     if (rss_types & 1) {
>> +                             uint64_t rss_type = 1ULL << i;
>> +                             const char *p = rsstype_to_str(rss_type);
>> +
>> +                             if (p)
>> +                                     printf("  %s\n", p);
>> +                             else
>> +                                     printf("  user defined 
>> 0x%"PRIx64"\n", rss_type);
>> +                     }
>> +                     rss_types >>= 1;
>>               }
>>       }
>>
>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>>   static void
>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>   {
>> +     uint64_t rss_types;
>>       uint8_t i;
>>
>>       if (rss_conf == NULL) {
>> @@ -1582,16 +1611,23 @@ rss_config_display(struct rte_flow_action_rss 
>> *rss_conf)
>>       }
>>
>>       printf(" types:\n");
>> -     if (rss_conf->types == 0) {
>> +     rss_types = rss_conf->types;
>> +     if (rss_types == 0) {
>>               printf("  none\n");
>>               return;
>>       }
>> -     for (i = 0; rss_type_table[i].str; i++) {
>> -             if ((rss_conf->types &
>> -                 rss_type_table[i].rss_type) ==
>> -                 rss_type_table[i].rss_type &&
>> -                 rss_type_table[i].rss_type != 0)
>> -                     printf("  %s\n", rss_type_table[i].str);
>> +
>> +     for (i = 0; rss_types != 0; i++) {
>> +             if (rss_types & 1) {
>> +                     uint64_t rss_type = 1ULL << i;
>> +                     const char *p = rsstype_to_str(rss_type);
>> +
>> +                     if (p)
>> +                             printf("  %s\n", p);
>> +                     else
>> +                             printf("  user defined 0x%"PRIx64"\n", 
>> rss_type);
>> +             }
>> +             rss_types >>= 1;
>>       }
>>   }
>>
>> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id, int 
>> show_rss_key)
>>               return;
>>       }
>>       printf("RSS functions:\n ");
>> -     for (i = 0; rss_type_table[i].str; i++) {
>> -             if (rss_type_table[i].rss_type == 0)
>> -                     continue;
>> -             if ((rss_hf & rss_type_table[i].rss_type) == 
>> rss_type_table[i].rss_type)
>> -                     printf("%s ", rss_type_table[i].str);
>> +     for (i = 0; rss_hf != 0; i++) {
>> +             if (rss_hf & 1) {
>> +                     uint64_t rss_type = 1ULL << i;
>> +                     const char *p = rsstype_to_str(rss_type);
>> +                     if (p)
>> +                             printf("%s ", p);
>> +                     else
>> +                             printf("0x%"PRIx64" ", rss_type);
>> +             }
>> +             rss_hf >>= 1;
>>       }
>>       printf("\n");
>>       if (!show_rss_key)
>> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id, 
>> char rss_type[], uint8_t *hash_key,
>>   {
>>       struct rte_eth_rss_conf rss_conf;
>>       int diag;
>> -     unsigned int i;
>>
>>       rss_conf.rss_key = NULL;
>>       rss_conf.rss_key_len = 0;
>> -     rss_conf.rss_hf = 0;
>> -     for (i = 0; rss_type_table[i].str; i++) {
>> -             if (!strcmp(rss_type_table[i].str, rss_type))
>> -                     rss_conf.rss_hf = rss_type_table[i].rss_type;
>> -     }
>> +     rss_conf.rss_hf = str_to_rsstype(rss_type);
>>       diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>       if (diag == 0) {
>>               rss_conf.rss_key = hash_key;
lihuisong (C) May 27, 2022, 2:30 a.m. UTC | #3
在 2022/5/26 1:37, Ferruh Yigit 写道:
> When supported RSS offload flow types are printed via 'show port info #'
> command, flow names are get from flow type array which is wrong and
> causing some RSS flow types not being displayed.
>
> Instead RSS flow type array should be used. Also helper functions added
> and existing code updated to use helpers.
>
> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
> ---
> Cc: helin.zhang@intel.com
>
> Note:
> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
> to check that too.
> ---
>   app/test-pmd/config.c | 92 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 64 insertions(+), 28 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 72d2606d19d5..c353d224ef06 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>   	{ NULL, 0 },
>   };
>   
> +static const char *
> +rsstype_to_str(uint64_t rss_type)
> +{
> +	int 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;
> +}
> +
> +static uint64_t
> +str_to_rsstype(const char *str)
> +{
> +	int i;
> +
> +	for (i = 0; rss_type_table[i].str != NULL; i++) {
> +		if (!strcmp(rss_type_table[i].str, str))
> +			return rss_type_table[i].rss_type;
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct {
>   	enum rte_eth_fec_mode mode;
>   	const char *name;
> @@ -779,19 +805,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_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; rss_types != 0; i++) {
> +			if (rss_types & 1) {
> +				uint64_t rss_type = 1ULL << i;
> +				const char *p = rsstype_to_str(rss_type);
> +
> +				if (p)
> +					printf("  %s\n", p);
> +				else
> +					printf("  user defined 0x%"PRIx64"\n", rss_type);
> +			}
> +			rss_types >>= 1;
>   		}
>   	}
>   
> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>   static void
>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>   {
> +	uint64_t rss_types;
>   	uint8_t i;
>   
>   	if (rss_conf == NULL) {
> @@ -1582,16 +1611,23 @@ rss_config_display(struct rte_flow_action_rss *rss_conf)
>   	}
>   
>   	printf(" types:\n");
> -	if (rss_conf->types == 0) {
> +	rss_types = rss_conf->types;
> +	if (rss_types == 0) {
>   		printf("  none\n");
>   		return;
>   	}
> -	for (i = 0; rss_type_table[i].str; i++) {
> -		if ((rss_conf->types &
> -		    rss_type_table[i].rss_type) ==
> -		    rss_type_table[i].rss_type &&
> -		    rss_type_table[i].rss_type != 0)
> -			printf("  %s\n", rss_type_table[i].str);
> +
> +	for (i = 0; rss_types != 0; i++) {
> +		if (rss_types & 1) {
> +			uint64_t rss_type = 1ULL << i;
> +			const char *p = rsstype_to_str(rss_type);
It seems that we can't use one bit to get rss type name.
Because part of name in rss_type_table[] consist of multiple bits.
Maybe it's better to use the original method to display RSS type name.

On the other hand, RSS type name are printed in many places as this 
patch modified.
It is recommended that you encapsulate a funcion to display RSS type.
> +
> +			if (p)
> +				printf("  %s\n", p);
> +			else
> +				printf("  user defined 0x%"PRIx64"\n", rss_type);
> +		}
> +		rss_types >>= 1;
>   	}
>   }
>   
> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id, int show_rss_key)
>   		return;
>   	}
>   	printf("RSS functions:\n ");
> -	for (i = 0; rss_type_table[i].str; i++) {
> -		if (rss_type_table[i].rss_type == 0)
> -			continue;
> -		if ((rss_hf & rss_type_table[i].rss_type) == rss_type_table[i].rss_type)
> -			printf("%s ", rss_type_table[i].str);
> +	for (i = 0; rss_hf != 0; i++) {
> +		if (rss_hf & 1) {
> +			uint64_t rss_type = 1ULL << i;
> +			const char *p = rsstype_to_str(rss_type);
> +			if (p)
> +				printf("%s ", p);
> +			else
> +				printf("0x%"PRIx64" ", rss_type);
> +		}
> +		rss_hf >>= 1;
>   	}
>   	printf("\n");
>   	if (!show_rss_key)
> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>   {
>   	struct rte_eth_rss_conf rss_conf;
>   	int diag;
> -	unsigned int i;
>   
>   	rss_conf.rss_key = NULL;
>   	rss_conf.rss_key_len = 0;
> -	rss_conf.rss_hf = 0;
> -	for (i = 0; rss_type_table[i].str; i++) {
> -		if (!strcmp(rss_type_table[i].str, rss_type))
> -			rss_conf.rss_hf = rss_type_table[i].rss_type;
> -	}
> +	rss_conf.rss_hf = str_to_rsstype(rss_type);
>   	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>   	if (diag == 0) {
>   		rss_conf.rss_key = hash_key;
lihuisong (C) May 27, 2022, 2:48 a.m. UTC | #4
在 2022/5/26 16:56, Ferruh Yigit 写道:
> On 5/26/2022 4:52 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.
>>
>>
>> Hi, Ferruh
>>
>> This patch is the same as the following patch:
>>
>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20220429102445.23711-2-lihuisong%40huawei.com%2F&amp;data=05%7C01%7CFerruh.Yigit%40amd.com%7Cda898bd1b39b438ce3e308da3ecb3123%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637891340148254654%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=kixRff%2BQavl0SYS7woT5YYJELrrocoSoV4sE7TUK9Is%3D&amp;reserved=0 
>>
>>
>> It was delegated to you.😂
>>
>
> Yes indeed, I missed it. They are fixing same issue.
> But there is slight difference in the implementation, can you please 
> review the set? We can share the sign off for final set.
ok.
>
>>
>> 在 2022/5/26 1:37, Ferruh Yigit 写道:
>>> When supported RSS offload flow types are printed via 'show port 
>>> info #'
>>> command, flow names are get from flow type array which is wrong and
>>> causing some RSS flow types not being displayed.
>>>
>>> Instead RSS flow type array should be used. Also helper functions added
>>> and existing code updated to use helpers.
>>>
>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>>> ---
>>> Cc: helin.zhang@intel.com
>>>
>>> Note:
>>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
>>> to check that too.
>>> ---
>>>   app/test-pmd/config.c | 92 
>>> ++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 72d2606d19d5..c353d224ef06 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>>       { NULL, 0 },
>>>   };
>>>
>>> +static const char *
>>> +rsstype_to_str(uint64_t rss_type)
>>> +{
>>> +     int 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;
>>> +}
>>> +
>>> +static uint64_t
>>> +str_to_rsstype(const char *str)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>>> +             if (!strcmp(rss_type_table[i].str, str))
>>> +                     return rss_type_table[i].rss_type;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>   static const struct {
>>>       enum rte_eth_fec_mode mode;
>>>       const char *name;
>>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>>> +                     if (rss_types & 1) {
>>> +                             uint64_t rss_type = 1ULL << i;
>>> +                             const char *p = rsstype_to_str(rss_type);
>>> +
>>> +                             if (p)
>>> +                                     printf("  %s\n", p);
>>> +                             else
>>> +                                     printf("  user defined 
>>> 0x%"PRIx64"\n", rss_type);
>>> +                     }
>>> +                     rss_types >>= 1;
>>>               }
>>>       }
>>>
>>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>>>   static void
>>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>>   {
>>> +     uint64_t rss_types;
>>>       uint8_t i;
>>>
>>>       if (rss_conf == NULL) {
>>> @@ -1582,16 +1611,23 @@ rss_config_display(struct 
>>> rte_flow_action_rss *rss_conf)
>>>       }
>>>
>>>       printf(" types:\n");
>>> -     if (rss_conf->types == 0) {
>>> +     rss_types = rss_conf->types;
>>> +     if (rss_types == 0) {
>>>               printf("  none\n");
>>>               return;
>>>       }
>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>> -             if ((rss_conf->types &
>>> -                 rss_type_table[i].rss_type) ==
>>> -                 rss_type_table[i].rss_type &&
>>> -                 rss_type_table[i].rss_type != 0)
>>> -                     printf("  %s\n", rss_type_table[i].str);
>>> +
>>> +     for (i = 0; rss_types != 0; i++) {
>>> +             if (rss_types & 1) {
>>> +                     uint64_t rss_type = 1ULL << i;
>>> +                     const char *p = rsstype_to_str(rss_type);
>>> +
>>> +                     if (p)
>>> +                             printf("  %s\n", p);
>>> +                     else
>>> +                             printf("  user defined 0x%"PRIx64"\n", 
>>> rss_type);
>>> +             }
>>> +             rss_types >>= 1;
>>>       }
>>>   }
>>>
>>> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id, 
>>> int show_rss_key)
>>>               return;
>>>       }
>>>       printf("RSS functions:\n ");
>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>> -             if (rss_type_table[i].rss_type == 0)
>>> -                     continue;
>>> -             if ((rss_hf & rss_type_table[i].rss_type) == 
>>> rss_type_table[i].rss_type)
>>> -                     printf("%s ", rss_type_table[i].str);
>>> +     for (i = 0; rss_hf != 0; i++) {
>>> +             if (rss_hf & 1) {
>>> +                     uint64_t rss_type = 1ULL << i;
>>> +                     const char *p = rsstype_to_str(rss_type);
>>> +                     if (p)
>>> +                             printf("%s ", p);
>>> +                     else
>>> +                             printf("0x%"PRIx64" ", rss_type);
>>> +             }
>>> +             rss_hf >>= 1;
>>>       }
>>>       printf("\n");
>>>       if (!show_rss_key)
>>> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id, 
>>> char rss_type[], uint8_t *hash_key,
>>>   {
>>>       struct rte_eth_rss_conf rss_conf;
>>>       int diag;
>>> -     unsigned int i;
>>>
>>>       rss_conf.rss_key = NULL;
>>>       rss_conf.rss_key_len = 0;
>>> -     rss_conf.rss_hf = 0;
>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>> -             if (!strcmp(rss_type_table[i].str, rss_type))
>>> -                     rss_conf.rss_hf = rss_type_table[i].rss_type;
>>> -     }
>>> +     rss_conf.rss_hf = str_to_rsstype(rss_type);
>>>       diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>       if (diag == 0) {
>>>               rss_conf.rss_key = hash_key;
>
> .
Ferruh Yigit May 30, 2022, 10:43 a.m. UTC | #5
On 5/27/2022 3:30 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/5/26 1:37, Ferruh Yigit 写道:
>> When supported RSS offload flow types are printed via 'show port info #'
>> command, flow names are get from flow type array which is wrong and
>> causing some RSS flow types not being displayed.
>>
>> Instead RSS flow type array should be used. Also helper functions added
>> and existing code updated to use helpers.
>>
>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>> ---
>> Cc: helin.zhang@intel.com
>>
>> Note:
>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
>> to check that too.
>> ---
>>   app/test-pmd/config.c | 92 ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 72d2606d19d5..c353d224ef06 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>       { NULL, 0 },
>>   };
>>
>> +static const char *
>> +rsstype_to_str(uint64_t rss_type)
>> +{
>> +     int 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;
>> +}
>> +
>> +static uint64_t
>> +str_to_rsstype(const char *str)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>> +             if (!strcmp(rss_type_table[i].str, str))
>> +                     return rss_type_table[i].rss_type;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>   static const struct {
>>       enum rte_eth_fec_mode mode;
>>       const char *name;
>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>> +                     if (rss_types & 1) {
>> +                             uint64_t rss_type = 1ULL << i;
>> +                             const char *p = rsstype_to_str(rss_type);
>> +
>> +                             if (p)
>> +                                     printf("  %s\n", p);
>> +                             else
>> +                                     printf("  user defined 
>> 0x%"PRIx64"\n", rss_type);
>> +                     }
>> +                     rss_types >>= 1;
>>               }
>>       }
>>
>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>>   static void
>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>   {
>> +     uint64_t rss_types;
>>       uint8_t i;
>>
>>       if (rss_conf == NULL) {
>> @@ -1582,16 +1611,23 @@ rss_config_display(struct rte_flow_action_rss 
>> *rss_conf)
>>       }
>>
>>       printf(" types:\n");
>> -     if (rss_conf->types == 0) {
>> +     rss_types = rss_conf->types;
>> +     if (rss_types == 0) {
>>               printf("  none\n");
>>               return;
>>       }
>> -     for (i = 0; rss_type_table[i].str; i++) {
>> -             if ((rss_conf->types &
>> -                 rss_type_table[i].rss_type) ==
>> -                 rss_type_table[i].rss_type &&
>> -                 rss_type_table[i].rss_type != 0)
>> -                     printf("  %s\n", rss_type_table[i].str);
>> +
>> +     for (i = 0; rss_types != 0; i++) {
>> +             if (rss_types & 1) {
>> +                     uint64_t rss_type = 1ULL << i;
>> +                     const char *p = rsstype_to_str(rss_type);
> It seems that we can't use one bit to get rss type name.
> Because part of name in rss_type_table[] consist of multiple bits.
> Maybe it's better to use the original method to display RSS type name.
>  

Right, it doesn't cover 'all' case, but thinking twice how useful 'all' 
case is, it doesn't really cover all options, and for user it is hard to 
know which functions are set when it displays 'all'.
What about to remove 'all' item from 'rss_type_table[]'?

> On the other hand, RSS type name are printed in many places as this
> patch modified.
> It is recommended that you encapsulate a funcion to display RSS type.

Yes but each are formatting slightly different, so to let various 
formatting possible, common function returns string instead of printing 
them. As 3/3 of this set does some formatting.

>> +
>> +                     if (p)
>> +                             printf("  %s\n", p);
>> +                     else
>> +                             printf("  user defined 0x%"PRIx64"\n", 
>> rss_type);
>> +             }
>> +             rss_types >>= 1;
>>       }
>>   }
>>
>> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id, int 
>> show_rss_key)
>>               return;
>>       }
>>       printf("RSS functions:\n ");
>> -     for (i = 0; rss_type_table[i].str; i++) {
>> -             if (rss_type_table[i].rss_type == 0)
>> -                     continue;
>> -             if ((rss_hf & rss_type_table[i].rss_type) == 
>> rss_type_table[i].rss_type)
>> -                     printf("%s ", rss_type_table[i].str);
>> +     for (i = 0; rss_hf != 0; i++) {
>> +             if (rss_hf & 1) {
>> +                     uint64_t rss_type = 1ULL << i;
>> +                     const char *p = rsstype_to_str(rss_type);
>> +                     if (p)
>> +                             printf("%s ", p);
>> +                     else
>> +                             printf("0x%"PRIx64" ", rss_type);
>> +             }
>> +             rss_hf >>= 1;
>>       }
>>       printf("\n");
>>       if (!show_rss_key)
>> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id, 
>> char rss_type[], uint8_t *hash_key,
>>   {
>>       struct rte_eth_rss_conf rss_conf;
>>       int diag;
>> -     unsigned int i;
>>
>>       rss_conf.rss_key = NULL;
>>       rss_conf.rss_key_len = 0;
>> -     rss_conf.rss_hf = 0;
>> -     for (i = 0; rss_type_table[i].str; i++) {
>> -             if (!strcmp(rss_type_table[i].str, rss_type))
>> -                     rss_conf.rss_hf = rss_type_table[i].rss_type;
>> -     }
>> +     rss_conf.rss_hf = str_to_rsstype(rss_type);
>>       diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>       if (diag == 0) {
>>               rss_conf.rss_key = hash_key;
lihuisong (C) May 30, 2022, 12:32 p.m. UTC | #6
在 2022/5/30 18:43, Ferruh Yigit 写道:
> On 5/27/2022 3:30 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/5/26 1:37, Ferruh Yigit 写道:
>>> When supported RSS offload flow types are printed via 'show port 
>>> info #'
>>> command, flow names are get from flow type array which is wrong and
>>> causing some RSS flow types not being displayed.
>>>
>>> Instead RSS flow type array should be used. Also helper functions added
>>> and existing code updated to use helpers.
>>>
>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>>> ---
>>> Cc: helin.zhang@intel.com
>>>
>>> Note:
>>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
>>> to check that too.
>>> ---
>>>   app/test-pmd/config.c | 92 
>>> ++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 72d2606d19d5..c353d224ef06 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>>       { NULL, 0 },
>>>   };
>>>
>>> +static const char *
>>> +rsstype_to_str(uint64_t rss_type)
>>> +{
>>> +     int 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;
>>> +}
>>> +
>>> +static uint64_t
>>> +str_to_rsstype(const char *str)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>>> +             if (!strcmp(rss_type_table[i].str, str))
>>> +                     return rss_type_table[i].rss_type;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>   static const struct {
>>>       enum rte_eth_fec_mode mode;
>>>       const char *name;
>>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>>> +                     if (rss_types & 1) {
>>> +                             uint64_t rss_type = 1ULL << i;
>>> +                             const char *p = rsstype_to_str(rss_type);
>>> +
>>> +                             if (p)
>>> +                                     printf("  %s\n", p);
>>> +                             else
>>> +                                     printf("  user defined 
>>> 0x%"PRIx64"\n", rss_type);
Maybe we need to add a mapping table between RSS offload and name for 
'flow_type_rss_offloads'.
>>> +                     }
>>> +                     rss_types >>= 1;
>>>               }
>>>       }
>>>
>>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>>>   static void
>>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>>   {
>>> +     uint64_t rss_types;
>>>       uint8_t i;
>>>
>>>       if (rss_conf == NULL) {
>>> @@ -1582,16 +1611,23 @@ rss_config_display(struct 
>>> rte_flow_action_rss *rss_conf)
>>>       }
>>>
>>>       printf(" types:\n");
>>> -     if (rss_conf->types == 0) {
>>> +     rss_types = rss_conf->types;
>>> +     if (rss_types == 0) {
>>>               printf("  none\n");
>>>               return;
>>>       }
>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>> -             if ((rss_conf->types &
>>> -                 rss_type_table[i].rss_type) ==
>>> -                 rss_type_table[i].rss_type &&
>>> -                 rss_type_table[i].rss_type != 0)
>>> -                     printf("  %s\n", rss_type_table[i].str);
>>> +
>>> +     for (i = 0; rss_types != 0; i++) {
>>> +             if (rss_types & 1) {
>>> +                     uint64_t rss_type = 1ULL << i;
>>> +                     const char *p = rsstype_to_str(rss_type);
>> It seems that we can't use one bit to get rss type name.
>> Because part of name in rss_type_table[] consist of multiple bits.
>> Maybe it's better to use the original method to display RSS type name.
>>
>
> Right, it doesn't cover 'all' case, but thinking twice how useful 
> 'all' case is, it doesn't really cover all options, and for user it is 
> hard to know which functions are set when it displays 'all'.
> What about to remove 'all' item from 'rss_type_table[]'?
It seems that remove 'all' item isn't the way to resolve this issue.
There are other iterms in 'rss_type_table[], such as, 'ip', 'udp',
'tcp' and so on. The 'rss_type_table[]' should primarily serve
the "show port rss xxx" and "port confg rss xxx" commands.
They all need these.
>
>> On the other hand, RSS type name are printed in many places as this
>> patch modified.
>> It is recommended that you encapsulate a funcion to display RSS type.
>
> Yes but each are formatting slightly different, so to let various 
> formatting possible, common function returns string instead of 
> printing them. As 3/3 of this set does some formatting.

We can unify this display if we add the mapping table mentioned above.

>
>>> +
>>> +                     if (p)
>>> +                             printf("  %s\n", p);
>>> +                     else
>>> +                             printf("  user defined 0x%"PRIx64"\n", 
>>> rss_type);
>>> +             }
>>> +             rss_types >>= 1;
>>>       }
>>>   }
>>>
>>> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id, 
>>> int show_rss_key)
>>>               return;
>>>       }
>>>       printf("RSS functions:\n ");
>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>> -             if (rss_type_table[i].rss_type == 0)
>>> -                     continue;
>>> -             if ((rss_hf & rss_type_table[i].rss_type) == 
>>> rss_type_table[i].rss_type)
>>> -                     printf("%s ", rss_type_table[i].str);
>>> +     for (i = 0; rss_hf != 0; i++) {
>>> +             if (rss_hf & 1) {
>>> +                     uint64_t rss_type = 1ULL << i;
>>> +                     const char *p = rsstype_to_str(rss_type);
>>> +                     if (p)
>>> +                             printf("%s ", p);
>>> +                     else
>>> +                             printf("0x%"PRIx64" ", rss_type);
>>> +             }
>>> +             rss_hf >>= 1;
>>>       }
>>>       printf("\n");
>>>       if (!show_rss_key)
>>> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id, 
>>> char rss_type[], uint8_t *hash_key,
>>>   {
>>>       struct rte_eth_rss_conf rss_conf;
>>>       int diag;
>>> -     unsigned int i;
>>>
>>>       rss_conf.rss_key = NULL;
>>>       rss_conf.rss_key_len = 0;
>>> -     rss_conf.rss_hf = 0;
>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>> -             if (!strcmp(rss_type_table[i].str, rss_type))
>>> -                     rss_conf.rss_hf = rss_type_table[i].rss_type;
>>> -     }
>>> +     rss_conf.rss_hf = str_to_rsstype(rss_type);
>>>       diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>       if (diag == 0) {
>>>               rss_conf.rss_key = hash_key;
>
> .
Ferruh Yigit May 30, 2022, 1:02 p.m. UTC | #7
On 5/30/2022 1:32 PM, 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/5/30 18:43, Ferruh Yigit 写道:
>> On 5/27/2022 3:30 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/5/26 1:37, Ferruh Yigit 写道:
>>>> When supported RSS offload flow types are printed via 'show port
>>>> info #'
>>>> command, flow names are get from flow type array which is wrong and
>>>> causing some RSS flow types not being displayed.
>>>>
>>>> Instead RSS flow type array should be used. Also helper functions added
>>>> and existing code updated to use helpers.
>>>>
>>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>>>> ---
>>>> Cc: helin.zhang@intel.com
>>>>
>>>> Note:
>>>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>>>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may need
>>>> to check that too.
>>>> ---
>>>>   app/test-pmd/config.c | 92
>>>> ++++++++++++++++++++++++++++++-------------
>>>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>> index 72d2606d19d5..c353d224ef06 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>>>       { NULL, 0 },
>>>>   };
>>>>
>>>> +static const char *
>>>> +rsstype_to_str(uint64_t rss_type)
>>>> +{
>>>> +     int 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;
>>>> +}
>>>> +
>>>> +static uint64_t
>>>> +str_to_rsstype(const char *str)
>>>> +{
>>>> +     int i;
>>>> +
>>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>>>> +             if (!strcmp(rss_type_table[i].str, str))
>>>> +                     return rss_type_table[i].rss_type;
>>>> +     }
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>>   static const struct {
>>>>       enum rte_eth_fec_mode mode;
>>>>       const char *name;
>>>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>>>> +                     if (rss_types & 1) {
>>>> +                             uint64_t rss_type = 1ULL << i;
>>>> +                             const char *p = rsstype_to_str(rss_type);
>>>> +
>>>> +                             if (p)
>>>> +                                     printf("  %s\n", p);
>>>> +                             else
>>>> +                                     printf("  user defined
>>>> 0x%"PRIx64"\n", rss_type);
> Maybe we need to add a mapping table between RSS offload and name for
> 'flow_type_rss_offloads'.
>>>> +                     }
>>>> +                     rss_types >>= 1;
>>>>               }
>>>>       }
>>>>
>>>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error *error)
>>>>   static void
>>>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>>>   {
>>>> +     uint64_t rss_types;
>>>>       uint8_t i;
>>>>
>>>>       if (rss_conf == NULL) {
>>>> @@ -1582,16 +1611,23 @@ rss_config_display(struct
>>>> rte_flow_action_rss *rss_conf)
>>>>       }
>>>>
>>>>       printf(" types:\n");
>>>> -     if (rss_conf->types == 0) {
>>>> +     rss_types = rss_conf->types;
>>>> +     if (rss_types == 0) {
>>>>               printf("  none\n");
>>>>               return;
>>>>       }
>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>> -             if ((rss_conf->types &
>>>> -                 rss_type_table[i].rss_type) ==
>>>> -                 rss_type_table[i].rss_type &&
>>>> -                 rss_type_table[i].rss_type != 0)
>>>> -                     printf("  %s\n", rss_type_table[i].str);
>>>> +
>>>> +     for (i = 0; rss_types != 0; i++) {
>>>> +             if (rss_types & 1) {
>>>> +                     uint64_t rss_type = 1ULL << i;
>>>> +                     const char *p = rsstype_to_str(rss_type);
>>> It seems that we can't use one bit to get rss type name.
>>> Because part of name in rss_type_table[] consist of multiple bits.
>>> Maybe it's better to use the original method to display RSS type name.
>>>
>>
>> Right, it doesn't cover 'all' case, but thinking twice how useful
>> 'all' case is, it doesn't really cover all options, and for user it is
>> hard to know which functions are set when it displays 'all'.
>> What about to remove 'all' item from 'rss_type_table[]'?
> It seems that remove 'all' item isn't the way to resolve this issue.
> There are other iterms in 'rss_type_table[], such as, 'ip', 'udp',
> 'tcp' and so on. The 'rss_type_table[]' should primarily serve
> the "show port rss xxx" and "port confg rss xxx" commands.
> They all need these.
 >

You are right, it is not just 'all'.

>>
>>> On the other hand, RSS type name are printed in many places as this
>>> patch modified.
>>> It is recommended that you encapsulate a funcion to display RSS type.
>>
>> Yes but each are formatting slightly different, so to let various
>> formatting possible, common function returns string instead of
>> printing them. As 3/3 of this set does some formatting.
> 
> We can unify this display if we add the mapping table mentioned above.
> 

If it can be unified, agree that this simplifies the code [2].

Both above two changes already exists in your set [1], if you don't mind 
to adapt 2/3 & 3/3 of this set into yours, we can continue with your 
set, is this OK for you?



[1]
https://patches.dpdk.org/project/dpdk/list/?series=22735

[2]
In your 'show_rss_types()', it can get additional parameter for 
formatting, like after how many items to break to next line, this can 
cover existing formatting options, but not sure how elegant a solution 
it is.

>>
>>>> +
>>>> +                     if (p)
>>>> +                             printf("  %s\n", p);
>>>> +                     else
>>>> +                             printf("  user defined 0x%"PRIx64"\n",
>>>> rss_type);
>>>> +             }
>>>> +             rss_types >>= 1;
>>>>       }
>>>>   }
>>>>
>>>> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id,
>>>> int show_rss_key)
>>>>               return;
>>>>       }
>>>>       printf("RSS functions:\n ");
>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>> -             if (rss_type_table[i].rss_type == 0)
>>>> -                     continue;
>>>> -             if ((rss_hf & rss_type_table[i].rss_type) ==
>>>> rss_type_table[i].rss_type)
>>>> -                     printf("%s ", rss_type_table[i].str);
>>>> +     for (i = 0; rss_hf != 0; i++) {
>>>> +             if (rss_hf & 1) {
>>>> +                     uint64_t rss_type = 1ULL << i;
>>>> +                     const char *p = rsstype_to_str(rss_type);
>>>> +                     if (p)
>>>> +                             printf("%s ", p);
>>>> +                     else
>>>> +                             printf("0x%"PRIx64" ", rss_type);
>>>> +             }
>>>> +             rss_hf >>= 1;
>>>>       }
>>>>       printf("\n");
>>>>       if (!show_rss_key)
>>>> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id,
>>>> char rss_type[], uint8_t *hash_key,
>>>>   {
>>>>       struct rte_eth_rss_conf rss_conf;
>>>>       int diag;
>>>> -     unsigned int i;
>>>>
>>>>       rss_conf.rss_key = NULL;
>>>>       rss_conf.rss_key_len = 0;
>>>> -     rss_conf.rss_hf = 0;
>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>> -             if (!strcmp(rss_type_table[i].str, rss_type))
>>>> -                     rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>> -     }
>>>> +     rss_conf.rss_hf = str_to_rsstype(rss_type);
>>>>       diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>       if (diag == 0) {
>>>>               rss_conf.rss_key = hash_key;
>>
>> .
lihuisong (C) May 31, 2022, 2:07 a.m. UTC | #8
在 2022/5/30 21:02, Ferruh Yigit 写道:
> On 5/30/2022 1:32 PM, 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/5/30 18:43, Ferruh Yigit 写道:
>>> On 5/27/2022 3:30 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/5/26 1:37, Ferruh Yigit 写道:
>>>>> When supported RSS offload flow types are printed via 'show port
>>>>> info #'
>>>>> command, flow names are get from flow type array which is wrong and
>>>>> causing some RSS flow types not being displayed.
>>>>>
>>>>> Instead RSS flow type array should be used. Also helper functions 
>>>>> added
>>>>> and existing code updated to use helpers.
>>>>>
>>>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>>>>> ---
>>>>> Cc: helin.zhang@intel.com
>>>>>
>>>>> Note:
>>>>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>>>>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may 
>>>>> need
>>>>> to check that too.
>>>>> ---
>>>>>   app/test-pmd/config.c | 92
>>>>> ++++++++++++++++++++++++++++++-------------
>>>>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>>> index 72d2606d19d5..c353d224ef06 100644
>>>>> --- a/app/test-pmd/config.c
>>>>> +++ b/app/test-pmd/config.c
>>>>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>>>>       { NULL, 0 },
>>>>>   };
>>>>>
>>>>> +static const char *
>>>>> +rsstype_to_str(uint64_t rss_type)
>>>>> +{
>>>>> +     int 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;
>>>>> +}
>>>>> +
>>>>> +static uint64_t
>>>>> +str_to_rsstype(const char *str)
>>>>> +{
>>>>> +     int i;
>>>>> +
>>>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>>>>> +             if (!strcmp(rss_type_table[i].str, str))
>>>>> +                     return rss_type_table[i].rss_type;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>>   static const struct {
>>>>>       enum rte_eth_fec_mode mode;
>>>>>       const char *name;
>>>>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>>>>> +                     if (rss_types & 1) {
>>>>> +                             uint64_t rss_type = 1ULL << i;
>>>>> +                             const char *p = 
>>>>> rsstype_to_str(rss_type);
>>>>> +
>>>>> +                             if (p)
>>>>> +                                     printf("  %s\n", p);
>>>>> +                             else
>>>>> +                                     printf("  user defined
>>>>> 0x%"PRIx64"\n", rss_type);
>> Maybe we need to add a mapping table between RSS offload and name for
>> 'flow_type_rss_offloads'.
>>>>> +                     }
>>>>> +                     rss_types >>= 1;
>>>>>               }
>>>>>       }
>>>>>
>>>>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error 
>>>>> *error)
>>>>>   static void
>>>>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>>>>   {
>>>>> +     uint64_t rss_types;
>>>>>       uint8_t i;
>>>>>
>>>>>       if (rss_conf == NULL) {
>>>>> @@ -1582,16 +1611,23 @@ rss_config_display(struct
>>>>> rte_flow_action_rss *rss_conf)
>>>>>       }
>>>>>
>>>>>       printf(" types:\n");
>>>>> -     if (rss_conf->types == 0) {
>>>>> +     rss_types = rss_conf->types;
>>>>> +     if (rss_types == 0) {
>>>>>               printf("  none\n");
>>>>>               return;
>>>>>       }
>>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>>> -             if ((rss_conf->types &
>>>>> -                 rss_type_table[i].rss_type) ==
>>>>> -                 rss_type_table[i].rss_type &&
>>>>> -                 rss_type_table[i].rss_type != 0)
>>>>> -                     printf("  %s\n", rss_type_table[i].str);
>>>>> +
>>>>> +     for (i = 0; rss_types != 0; i++) {
>>>>> +             if (rss_types & 1) {
>>>>> +                     uint64_t rss_type = 1ULL << i;
>>>>> +                     const char *p = rsstype_to_str(rss_type);
>>>> It seems that we can't use one bit to get rss type name.
>>>> Because part of name in rss_type_table[] consist of multiple bits.
>>>> Maybe it's better to use the original method to display RSS type name.
>>>>
>>>
>>> Right, it doesn't cover 'all' case, but thinking twice how useful
>>> 'all' case is, it doesn't really cover all options, and for user it is
>>> hard to know which functions are set when it displays 'all'.
>>> What about to remove 'all' item from 'rss_type_table[]'?
>> It seems that remove 'all' item isn't the way to resolve this issue.
>> There are other iterms in 'rss_type_table[], such as, 'ip', 'udp',
>> 'tcp' and so on. The 'rss_type_table[]' should primarily serve
>> the "show port rss xxx" and "port confg rss xxx" commands.
>> They all need these.
> >
>
> You are right, it is not just 'all'.
>
>>>
>>>> On the other hand, RSS type name are printed in many places as this
>>>> patch modified.
>>>> It is recommended that you encapsulate a funcion to display RSS type.
>>>
>>> Yes but each are formatting slightly different, so to let various
>>> formatting possible, common function returns string instead of
>>> printing them. As 3/3 of this set does some formatting.
>>
>> We can unify this display if we add the mapping table mentioned above.
>>
>
> If it can be unified, agree that this simplifies the code [2].
Ack.
>
> Both above two changes already exists in your set [1], if you don't 
> mind to adapt 2/3 & 3/3 of this set into yours, we can continue with 
> your set, is this OK for you?
ok. It's my pleasure to work with you.
>
>
>
> [1]
> https://patches.dpdk.org/project/dpdk/list/?series=22735
>
> [2]
> In your 'show_rss_types()', it can get additional parameter for 
> formatting, like after how many items to break to next line, this can 
> cover existing formatting options, but not sure how elegant a solution 
> it is.
>
>>>
>>>>> +
>>>>> +                     if (p)
>>>>> +                             printf("  %s\n", p);
>>>>> +                     else
>>>>> +                             printf("  user defined 0x%"PRIx64"\n",
>>>>> rss_type);
>>>>> +             }
>>>>> +             rss_types >>= 1;
>>>>>       }
>>>>>   }
>>>>>
>>>>> @@ -3823,11 +3859,16 @@ port_rss_hash_conf_show(portid_t port_id,
>>>>> int show_rss_key)
>>>>>               return;
>>>>>       }
>>>>>       printf("RSS functions:\n ");
>>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>>> -             if (rss_type_table[i].rss_type == 0)
>>>>> -                     continue;
>>>>> -             if ((rss_hf & rss_type_table[i].rss_type) ==
>>>>> rss_type_table[i].rss_type)
>>>>> -                     printf("%s ", rss_type_table[i].str);
>>>>> +     for (i = 0; rss_hf != 0; i++) {
>>>>> +             if (rss_hf & 1) {
>>>>> +                     uint64_t rss_type = 1ULL << i;
>>>>> +                     const char *p = rsstype_to_str(rss_type);
>>>>> +                     if (p)
>>>>> +                             printf("%s ", p);
>>>>> +                     else
>>>>> +                             printf("0x%"PRIx64" ", rss_type);
>>>>> +             }
>>>>> +             rss_hf >>= 1;
>>>>>       }
>>>>>       printf("\n");
>>>>>       if (!show_rss_key)
>>>>> @@ -3844,15 +3885,10 @@ port_rss_hash_key_update(portid_t port_id,
>>>>> char rss_type[], uint8_t *hash_key,
>>>>>   {
>>>>>       struct rte_eth_rss_conf rss_conf;
>>>>>       int diag;
>>>>> -     unsigned int i;
>>>>>
>>>>>       rss_conf.rss_key = NULL;
>>>>>       rss_conf.rss_key_len = 0;
>>>>> -     rss_conf.rss_hf = 0;
>>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>>> -             if (!strcmp(rss_type_table[i].str, rss_type))
>>>>> -                     rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>> -     }
>>>>> +     rss_conf.rss_hf = str_to_rsstype(rss_type);
>>>>>       diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>>       if (diag == 0) {
>>>>>               rss_conf.rss_key = hash_key;
>>>
>>> .
>
> .
Andrew Rybchenko May 31, 2022, 4:35 p.m. UTC | #9
On 5/31/22 05:07, lihuisong (C) wrote:
> 
> 在 2022/5/30 21:02, Ferruh Yigit 写道:
>> On 5/30/2022 1:32 PM, 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/5/30 18:43, Ferruh Yigit 写道:
>>>> On 5/27/2022 3:30 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/5/26 1:37, Ferruh Yigit 写道:
>>>>>> When supported RSS offload flow types are printed via 'show port
>>>>>> info #'
>>>>>> command, flow names are get from flow type array which is wrong and
>>>>>> causing some RSS flow types not being displayed.
>>>>>>
>>>>>> Instead RSS flow type array should be used. Also helper functions 
>>>>>> added
>>>>>> and existing code updated to use helpers.
>>>>>>
>>>>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
>>>>>> ---
>>>>>> Cc: helin.zhang@intel.com
>>>>>>
>>>>>> Note:
>>>>>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros
>>>>>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may 
>>>>>> need
>>>>>> to check that too.
>>>>>> ---
>>>>>>   app/test-pmd/config.c | 92
>>>>>> ++++++++++++++++++++++++++++++-------------
>>>>>>   1 file changed, 64 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>>>> index 72d2606d19d5..c353d224ef06 100644
>>>>>> --- a/app/test-pmd/config.c
>>>>>> +++ b/app/test-pmd/config.c
>>>>>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = {
>>>>>>       { NULL, 0 },
>>>>>>   };
>>>>>>
>>>>>> +static const char *
>>>>>> +rsstype_to_str(uint64_t rss_type)
>>>>>> +{
>>>>>> +     int 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;
>>>>>> +}
>>>>>> +
>>>>>> +static uint64_t
>>>>>> +str_to_rsstype(const char *str)
>>>>>> +{
>>>>>> +     int i;
>>>>>> +
>>>>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) {
>>>>>> +             if (!strcmp(rss_type_table[i].str, str))
>>>>>> +                     return rss_type_table[i].rss_type;
>>>>>> +     }
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>>   static const struct {
>>>>>>       enum rte_eth_fec_mode mode;
>>>>>>       const char *name;
>>>>>> @@ -779,19 +805,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_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; rss_types != 0; i++) {
>>>>>> +                     if (rss_types & 1) {
>>>>>> +                             uint64_t rss_type = 1ULL << i;
>>>>>> +                             const char *p = 
>>>>>> rsstype_to_str(rss_type);
>>>>>> +
>>>>>> +                             if (p)
>>>>>> +                                     printf("  %s\n", p);
>>>>>> +                             else
>>>>>> +                                     printf("  user defined
>>>>>> 0x%"PRIx64"\n", rss_type);
>>> Maybe we need to add a mapping table between RSS offload and name for
>>> 'flow_type_rss_offloads'.
>>>>>> +                     }
>>>>>> +                     rss_types >>= 1;
>>>>>>               }
>>>>>>       }
>>>>>>
>>>>>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error 
>>>>>> *error)
>>>>>>   static void
>>>>>>   rss_config_display(struct rte_flow_action_rss *rss_conf)
>>>>>>   {
>>>>>> +     uint64_t rss_types;
>>>>>>       uint8_t i;
>>>>>>
>>>>>>       if (rss_conf == NULL) {
>>>>>> @@ -1582,16 +1611,23 @@ rss_config_display(struct
>>>>>> rte_flow_action_rss *rss_conf)
>>>>>>       }
>>>>>>
>>>>>>       printf(" types:\n");
>>>>>> -     if (rss_conf->types == 0) {
>>>>>> +     rss_types = rss_conf->types;
>>>>>> +     if (rss_types == 0) {
>>>>>>               printf("  none\n");
>>>>>>               return;
>>>>>>       }
>>>>>> -     for (i = 0; rss_type_table[i].str; i++) {
>>>>>> -             if ((rss_conf->types &
>>>>>> -                 rss_type_table[i].rss_type) ==
>>>>>> -                 rss_type_table[i].rss_type &&
>>>>>> -                 rss_type_table[i].rss_type != 0)
>>>>>> -                     printf("  %s\n", rss_type_table[i].str);
>>>>>> +
>>>>>> +     for (i = 0; rss_types != 0; i++) {
>>>>>> +             if (rss_types & 1) {
>>>>>> +                     uint64_t rss_type = 1ULL << i;
>>>>>> +                     const char *p = rsstype_to_str(rss_type);
>>>>> It seems that we can't use one bit to get rss type name.
>>>>> Because part of name in rss_type_table[] consist of multiple bits.
>>>>> Maybe it's better to use the original method to display RSS type name.
>>>>>
>>>>
>>>> Right, it doesn't cover 'all' case, but thinking twice how useful
>>>> 'all' case is, it doesn't really cover all options, and for user it is
>>>> hard to know which functions are set when it displays 'all'.
>>>> What about to remove 'all' item from 'rss_type_table[]'?
>>> It seems that remove 'all' item isn't the way to resolve this issue.
>>> There are other iterms in 'rss_type_table[], such as, 'ip', 'udp',
>>> 'tcp' and so on. The 'rss_type_table[]' should primarily serve
>>> the "show port rss xxx" and "port confg rss xxx" commands.
>>> They all need these.
>> >
>>
>> You are right, it is not just 'all'.
>>
>>>>
>>>>> On the other hand, RSS type name are printed in many places as this
>>>>> patch modified.
>>>>> It is recommended that you encapsulate a funcion to display RSS type.
>>>>
>>>> Yes but each are formatting slightly different, so to let various
>>>> formatting possible, common function returns string instead of
>>>> printing them. As 3/3 of this set does some formatting.
>>>
>>> We can unify this display if we add the mapping table mentioned above.
>>>
>>
>> If it can be unified, agree that this simplifies the code [2].
> Ack.
>>
>> Both above two changes already exists in your set [1], if you don't 
>> mind to adapt 2/3 & 3/3 of this set into yours, we can continue with 
>> your set, is this OK for you?
> ok. It's my pleasure to work with you.
>>
>>
>>
>> [1]
>> https://patches.dpdk.org/project/dpdk/list/?series=22735
>>
>> [2]
>> In your 'show_rss_types()', it can get additional parameter for 
>> formatting, like after how many items to break to next line, this can 
>> cover existing formatting options, but not sure how elegant a solution 
>> it is.

So, I'm waiting for a new version from lihuisong.
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 72d2606d19d5..c353d224ef06 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -147,6 +147,32 @@  const struct rss_type_info rss_type_table[] = {
 	{ NULL, 0 },
 };
 
+static const char *
+rsstype_to_str(uint64_t rss_type)
+{
+	int 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;
+}
+
+static uint64_t
+str_to_rsstype(const char *str)
+{
+	int i;
+
+	for (i = 0; rss_type_table[i].str != NULL; i++) {
+		if (!strcmp(rss_type_table[i].str, str))
+			return rss_type_table[i].rss_type;
+	}
+
+	return 0;
+}
+
 static const struct {
 	enum rte_eth_fec_mode mode;
 	const char *name;
@@ -779,19 +805,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_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; rss_types != 0; i++) {
+			if (rss_types & 1) {
+				uint64_t rss_type = 1ULL << i;
+				const char *p = rsstype_to_str(rss_type);
+
+				if (p)
+					printf("  %s\n", p);
+				else
+					printf("  user defined 0x%"PRIx64"\n", rss_type);
+			}
+			rss_types >>= 1;
 		}
 	}
 
@@ -1547,6 +1575,7 @@  port_flow_complain(struct rte_flow_error *error)
 static void
 rss_config_display(struct rte_flow_action_rss *rss_conf)
 {
+	uint64_t rss_types;
 	uint8_t i;
 
 	if (rss_conf == NULL) {
@@ -1582,16 +1611,23 @@  rss_config_display(struct rte_flow_action_rss *rss_conf)
 	}
 
 	printf(" types:\n");
-	if (rss_conf->types == 0) {
+	rss_types = rss_conf->types;
+	if (rss_types == 0) {
 		printf("  none\n");
 		return;
 	}
-	for (i = 0; rss_type_table[i].str; i++) {
-		if ((rss_conf->types &
-		    rss_type_table[i].rss_type) ==
-		    rss_type_table[i].rss_type &&
-		    rss_type_table[i].rss_type != 0)
-			printf("  %s\n", rss_type_table[i].str);
+
+	for (i = 0; rss_types != 0; i++) {
+		if (rss_types & 1) {
+			uint64_t rss_type = 1ULL << i;
+			const char *p = rsstype_to_str(rss_type);
+
+			if (p)
+				printf("  %s\n", p);
+			else
+				printf("  user defined 0x%"PRIx64"\n", rss_type);
+		}
+		rss_types >>= 1;
 	}
 }
 
@@ -3823,11 +3859,16 @@  port_rss_hash_conf_show(portid_t port_id, int show_rss_key)
 		return;
 	}
 	printf("RSS functions:\n ");
-	for (i = 0; rss_type_table[i].str; i++) {
-		if (rss_type_table[i].rss_type == 0)
-			continue;
-		if ((rss_hf & rss_type_table[i].rss_type) == rss_type_table[i].rss_type)
-			printf("%s ", rss_type_table[i].str);
+	for (i = 0; rss_hf != 0; i++) {
+		if (rss_hf & 1) {
+			uint64_t rss_type = 1ULL << i;
+			const char *p = rsstype_to_str(rss_type);
+			if (p)
+				printf("%s ", p);
+			else
+				printf("0x%"PRIx64" ", rss_type);
+		}
+		rss_hf >>= 1;
 	}
 	printf("\n");
 	if (!show_rss_key)
@@ -3844,15 +3885,10 @@  port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
 {
 	struct rte_eth_rss_conf rss_conf;
 	int diag;
-	unsigned int i;
 
 	rss_conf.rss_key = NULL;
 	rss_conf.rss_key_len = 0;
-	rss_conf.rss_hf = 0;
-	for (i = 0; rss_type_table[i].str; i++) {
-		if (!strcmp(rss_type_table[i].str, rss_type))
-			rss_conf.rss_hf = rss_type_table[i].rss_type;
-	}
+	rss_conf.rss_hf = str_to_rsstype(rss_type);
 	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
 	if (diag == 0) {
 		rss_conf.rss_key = hash_key;