diff mbox series

[v2,5/5] ethdev: telemetry xstats support hide zero

Message ID 20230111120630.31172-6-fengchengwen@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2,1/5] dmadev: support stats reset telemetry command |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

fengchengwen Jan. 11, 2023, 12:06 p.m. UTC 
  The number of xstats may be large, after the hide zero option is added,
only non-zero values can be displayed.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Comments

Bruce Richardson Jan. 11, 2023, 2:08 p.m. UTC | #1 
On Wed, Jan 11, 2023 at 12:06:30PM +0000, Chengwen Feng wrote:
> The number of xstats may be large, after the hide zero option is added,
> only non-zero values can be displayed.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 2fc593b865..77cacc0829 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>  {
>  	struct rte_eth_xstat *eth_xstats;
>  	struct rte_eth_xstat_name *xstat_names;
> +	char *end_param, *hide_param;
>  	int port_id, num_xstats;
> +	int hide_zero = 0;
>  	int i, ret;
> -	char *end_param;
>  
>  	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>  		return -1;
>  
>  	port_id = strtoul(params, &end_param, 0);
> -	if (*end_param != '\0')
> -		RTE_ETHDEV_LOG(NOTICE,
> -			"Extra parameters passed to ethdev telemetry command, ignoring\n");
>  	if (!rte_eth_dev_is_valid_port(port_id))
>  		return -1;
>  
> +	if (*end_param != '\0') {
> +		hide_param = strtok(end_param, ",");
> +		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
> +			return -EINVAL;
> +		hide_zero = strtoul(hide_param, &end_param, 0);
> +		if (*end_param != '\0')
> +			RTE_ETHDEV_LOG(NOTICE,
> +				"Extra parameters passed to ethdev telemetry command, ignoring\n");
> +		if (hide_zero != 0 && hide_zero != 1) {
> +			hide_zero = !!hide_zero;
> +			RTE_ETHDEV_LOG(NOTICE,
> +				"Hide zero parameter is non-boolean, cast to boolean\n");
> +		}
> +	}
> +

I'm not sure about this adding of an extra flag as a 0/1 value. That would
seem to make it confusing with a queue number or extra port number. For
such an optional parameter, I think a string value would be clearer. A
number of alternatives I'd suggest - in order of my preference (least
preferred to most preferred):

* have the extra parameter as a literal string "hide_zeros" which is
  matched against rather than looking for 0/1, or alternatively

* add a new command /ethdev/xstats_nonzero which does this, the same
  callback can be reusued, just checking the command given, or finally,

* if this is primarily for the benefit of users using the telemetry script
  to interactively view stats, then the functionality could be implemented
  in the script itself rather than in the backend. We could add some
  setting, or extra flag to the display code, to possibly filter out zeros
  in the display.

Thoughts?

/Bruce
fengchengwen Jan. 20, 2023, 3:51 a.m. UTC | #2 
Hi Bruce,

On 2023/1/11 22:08, Bruce Richardson wrote:
> On Wed, Jan 11, 2023 at 12:06:30PM +0000, Chengwen Feng wrote:
>> The number of xstats may be large, after the hide zero option is added,
>> only non-zero values can be displayed.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 2fc593b865..77cacc0829 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -5870,20 +5870,33 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>>  {
>>  	struct rte_eth_xstat *eth_xstats;
>>  	struct rte_eth_xstat_name *xstat_names;
>> +	char *end_param, *hide_param;
>>  	int port_id, num_xstats;
>> +	int hide_zero = 0;
>>  	int i, ret;
>> -	char *end_param;
>>  
>>  	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>>  		return -1;
>>  
>>  	port_id = strtoul(params, &end_param, 0);
>> -	if (*end_param != '\0')
>> -		RTE_ETHDEV_LOG(NOTICE,
>> -			"Extra parameters passed to ethdev telemetry command, ignoring\n");
>>  	if (!rte_eth_dev_is_valid_port(port_id))
>>  		return -1;
>>  
>> +	if (*end_param != '\0') {
>> +		hide_param = strtok(end_param, ",");
>> +		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
>> +			return -EINVAL;
>> +		hide_zero = strtoul(hide_param, &end_param, 0);
>> +		if (*end_param != '\0')
>> +			RTE_ETHDEV_LOG(NOTICE,
>> +				"Extra parameters passed to ethdev telemetry command, ignoring\n");
>> +		if (hide_zero != 0 && hide_zero != 1) {
>> +			hide_zero = !!hide_zero;
>> +			RTE_ETHDEV_LOG(NOTICE,
>> +				"Hide zero parameter is non-boolean, cast to boolean\n");
>> +		}
>> +	}
>> +
> 
> I'm not sure about this adding of an extra flag as a 0/1 value. That would
> seem to make it confusing with a queue number or extra port number. For
> such an optional parameter, I think a string value would be clearer. A
> number of alternatives I'd suggest - in order of my preference (least
> preferred to most preferred):
> 
> * have the extra parameter as a literal string "hide_zeros" which is
>   matched against rather than looking for 0/1, or alternatively

add string "hide_zeros" requires more characters, it may not user friendly.

> 
> * add a new command /ethdev/xstats_nonzero which does this, the same
>   callback can be reusued, just checking the command given, or finally,

The new command may cause confusion.

> 
> * if this is primarily for the benefit of users using the telemetry script
>   to interactively view stats, then the functionality could be implemented
>   in the script itself rather than in the backend. We could add some
>   setting, or extra flag to the display code, to possibly filter out zeros
>   in the display.

This may causes the display logic to be coupled with the command name.

> 
> Thoughts?

I prefer add optional parameter, and the optional parameter should be simpler, just a number here.

And for clearer expression, I reword the help string in V4:
Returns the extended stats for a port. Parameters: int port_id, bool hide_zero (Optional, non-zero indicates hide zero xstats)

Thanks.

> 
> /Bruce
> .
>
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 2fc593b865..77cacc0829 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5870,20 +5870,33 @@  eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 {
 	struct rte_eth_xstat *eth_xstats;
 	struct rte_eth_xstat_name *xstat_names;
+	char *end_param, *hide_param;
 	int port_id, num_xstats;
+	int hide_zero = 0;
 	int i, ret;
-	char *end_param;
 
 	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
 		return -1;
 
 	port_id = strtoul(params, &end_param, 0);
-	if (*end_param != '\0')
-		RTE_ETHDEV_LOG(NOTICE,
-			"Extra parameters passed to ethdev telemetry command, ignoring\n");
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
+	if (*end_param != '\0') {
+		hide_param = strtok(end_param, ",");
+		if (!hide_param || strlen(hide_param) == 0 || !isdigit(*hide_param))
+			return -EINVAL;
+		hide_zero = strtoul(hide_param, &end_param, 0);
+		if (*end_param != '\0')
+			RTE_ETHDEV_LOG(NOTICE,
+				"Extra parameters passed to ethdev telemetry command, ignoring\n");
+		if (hide_zero != 0 && hide_zero != 1) {
+			hide_zero = !!hide_zero;
+			RTE_ETHDEV_LOG(NOTICE,
+				"Hide zero parameter is non-boolean, cast to boolean\n");
+		}
+	}
+
 	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
 	if (num_xstats < 0)
 		return -1;
@@ -5908,9 +5921,12 @@  eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	}
 
 	rte_tel_data_start_dict(d);
-	for (i = 0; i < num_xstats; i++)
+	for (i = 0; i < num_xstats; i++) {
+		if (hide_zero && eth_xstats[i].value == 0)
+			continue;
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	}
 	free(eth_xstats);
 	return 0;
 }
@@ -6354,7 +6370,7 @@  RTE_INIT(ethdev_init_telemetry)
 	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id");
+			"Returns the extended stats for a port. Parameters: int port_id, hide_zero (Optional, specify whether to hide zero values)");
 	rte_telemetry_register_cmd("/ethdev/xstats_reset", eth_dev_handle_port_xstats_reset,
 			"Reset the extended stats for a port. Parameters: int port_id");
 #ifndef RTE_EXEC_ENV_WINDOWS