Re: [dpdk-dev] [PATCH v4] app/testpmd: add option to display extended statistics

[Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>]

On 9/2/21 7:08 PM, Ferruh Yigit wrote:
> On 8/20/2021 2:55 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Add 'display-xstats' option for using in accompanying with Rx/Tx statistics
>> (i.e. 'stats-period' option or 'show port stats' interactive command) to
>> display specified list of extended statistics.
>>
> Overall +1 to the feature.
>
> Just a reminder that same thing can be done via telemetry, custom xstat values
> can be retrieved from any dpdk application (including testpmd) by a telemetry
> client. cc'ed Ciara if more detail is required.
>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>> v4:
>>      - split from patch series
>>      - move xstats information to rte_port structure to avoid
>>        extra global structure
>>
>>   app/test-pmd/cmdline.c                |  55 +++++++++++++
>>   app/test-pmd/config.c                 |  66 ++++++++++++++++
>>   app/test-pmd/parameters.c             |  14 ++++
>>   app/test-pmd/testpmd.c                | 110 ++++++++++++++++++++++++++
>>   app/test-pmd/testpmd.h                |  23 ++++++
>>   doc/guides/testpmd_app_ug/run_app.rst |   5 ++
>>   6 files changed, 273 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 82253bc751..cd538ace30 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -3621,6 +3621,61 @@ cmdline_parse_inst_t cmd_stop = {
>>   
>>   /* *** SET CORELIST and PORTLIST CONFIGURATION *** */
>>   
> Inserting the below function between the above comment and its function makes
> the comment wrong.
> This is in file 'cmdline.c', which has command line functions and static
> functions that are needed for the command.
> Below function is to parse the application parameter, and called by a function
> in 'parameters.c'. Since that is the only consumer of this function, why not
> move this function to 'parameters.c' file and make it static?
It will be done in the next version.
>> +int
>> +parse_xstats_list(const char *in_str, struct rte_eth_xstat_name **xstats,
>> +		  unsigned int *xstats_num)
>> +{
>> +	int max_names_nb, names_nb;
>> +	int stringlen;
>> +	char **names;
>> +	char *str;
>> +	int ret;
>> +	int i;
>> +
>> +	names = NULL;
>> +	str = strdup(in_str);
> 'in_str' is an user input, it is coming from 'lgopts()', can you please double
> check if it is guaranteed that it will be null terminated?
optarg points to argv that's guaranteed to be null terminated.
>
>> +	if (str == NULL) {
>> +		ret = ENOMEM;
> Please return negative error values.
> Only net/sfc has positive error values, since that is the syntax in its base
> code and we let to keep the syntax, but for rest of the DPDK please use negative
> error values.
>
>> +		goto out;
>> +	}
>> +	stringlen = strlen(str);
>> +> +	for (i = 0, max_names_nb = 1; str[i] != '\0'; i++) {
>> +		if (str[i] == ',')
>> +			max_names_nb++;
>> +	}
>> +
>> +	names = calloc(max_names_nb, sizeof(*names));
>> +	if (names == NULL) {
>> +		ret = ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	names_nb = rte_strsplit(str, stringlen, names, max_names_nb, ',');
>> +	if (names_nb < 0) {
>> +		ret = EINVAL;
>> +		goto out;
>> +	}
> Should we check the length of each 'names' to prevent unnecessary allocation for
> the cause user provided something like --display-xstats ',,,,,'?
> I think this check can be done during copy below.
It's good to have, will be done in the next version.
>
>> +
>> +	*xstats = calloc(names_nb, sizeof(**xstats));
>> +	if (*xstats == NULL) {
>> +		ret = ENOMEM;
>> +		goto out;
>> +	}
>> +
> When and how 'xstats' ('xstats_display') is freed?
I'll add free in the next version.
>
>> +	for (i = 0; i < names_nb; i++)
>> +		rte_strscpy((*xstats)[i].name, names[i],
>> +			    sizeof((*xstats)[i].name));
>> +
>> +	*xstats_num = names_nb;
>> +	ret = 0;
>> +
>> +out:
>> +	free(names);
>> +	free(str);
>> +	return ret;
>> +}
>> +
>>   unsigned int
>>   parse_item_list(const char *str, const char *item_name, unsigned int max_items,
>>   		unsigned int *parsed_items, int check_unique_values)
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 31d8ba1b91..ea5b59f54f 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -173,6 +173,70 @@ print_ethaddr(const char *name, struct rte_ether_addr *eth_addr)
>>   	printf("%s%s", name, buf);
>>   }
>>   
>> +static void
>> +nic_xstats_display_periodic(portid_t port_id)
>> +{
>> +	struct xstat_display_info *xstats_info;
>> +	uint64_t *prev_values, *curr_values;
>> +	uint64_t diff_value, value_rate;
>> +	uint64_t *ids, *ids_supp;
>> +	struct timespec cur_time;
>> +	unsigned int i, i_supp;
>> +	size_t ids_supp_sz;
>> +	uint64_t diff_ns;
>> +	int rc;
>> +
>> +	xstats_info = &ports[port_id].xstats_info;
>> +
>> +	ids_supp_sz = xstats_info->ids_supp_sz;
>> +	if (xstats_display_num == 0 || ids_supp_sz == 0)
>> +		return;
>> +
>> +	printf("\n");
>> +
>> +	ids = xstats_info->ids;
>> +	ids_supp = xstats_info->ids_supp;
>> +	prev_values = xstats_info->prev_values;
>> +	curr_values = xstats_info->curr_values;
>> +
>> +	rc = rte_eth_xstats_get_by_id(port_id, ids_supp, curr_values,
>> +				      ids_supp_sz);
>> +	if (rc != (int)ids_supp_sz) {
>> +		fprintf(stderr, "%s: Failed to get values of %zu supported xstats for port %u - return code %d\n",
> Can you break the line after 'stderr, ' to reduce the line length?
>
> Also I think you can drop 'supported' word in this context, because you already
> know all requested xstat is supported and retrieving it is failing, so for this
> log it is failing to get values of xstat.
>
>> +			__func__, ids_supp_sz, port_id, rc);
> Not sure if __func__is required here?
>
>> +		return;
>> +	}
>> +
>> +	diff_ns = 0;
>> +	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
>> +		uint64_t ns;
>> +
>> +		ns = cur_time.tv_sec * NS_PER_SEC;
>> +		ns += cur_time.tv_nsec;
>> +
>> +		if (xstats_info->prev_ns != 0)
>> +			diff_ns = ns - xstats_info->prev_ns;
>> +		xstats_info->prev_ns = ns;
>> +	}
>> +
>> +	printf("%-31s%-17s%s\n", " ", "Value", "Rate (since last show)");
>> +	for (i = i_supp = 0; i < xstats_display_num; i++) {
>> +		if (ids[i] == XSTAT_ID_INVALID)
>> +			continue;
>> +
>> +		diff_value = (curr_values[i_supp] > prev_values[i]) ?
>> +			     (curr_values[i_supp] - prev_values[i]) : 0;
>> +		prev_values[i] = curr_values[i_supp];
>> +		value_rate = diff_ns > 0 ?
>> +				(double)diff_value / diff_ns * NS_PER_SEC : 0;
>> +
>> +		printf("  %-25s%12"PRIu64" %15"PRIu64"\n",
>> +		       xstats_display[i].name, curr_values[i_supp], value_rate);
>> +
>> +		i_supp++;
>> +	}
>> +}
>> +
>>   void
>>   nic_stats_display(portid_t port_id)
>>   {
>> @@ -243,6 +307,8 @@ nic_stats_display(portid_t port_id)
>>   	       PRIu64"          Tx-bps: %12"PRIu64"\n", mpps_rx, mbps_rx * 8,
>>   	       mpps_tx, mbps_tx * 8);
>>   
>> +	nic_xstats_display_periodic(port_id);
>> +
> What do you think to call the function if 'xstats_display_num > 0'? I can see
> there is already check in the function but I assume '--display-xstats' won't be
> set for majority of use cases, and checking it in advance can prevent
> unnecessary function call.
Yes, it would be better.
>
>>   	printf("  %s############################%s\n",
>>   	       nic_stats_border, nic_stats_border);
>>   }
>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>> index 7c13210f04..e78929795c 100644
>> --- a/app/test-pmd/parameters.c
>> +++ b/app/test-pmd/parameters.c
>> @@ -61,6 +61,9 @@ usage(char* progname)
>>   	       "(only if interactive is disabled).\n");
>>   	printf("  --stats-period=PERIOD: statistics will be shown "
>>   	       "every PERIOD seconds (only if interactive is disabled).\n");
>> +	printf("  --display-xstats xstat1[,...]: extended statistics to show. "
>> +	       "Used with --stats-period specified or interactive commands "
>> +	       "that show Rx/Tx statistics (i.e. 'show port stats').\n");
>>   	printf("  --nb-cores=N: set the number of forwarding cores "
>>   	       "(1 <= N <= %d).\n", nb_lcores);
>>   	printf("  --nb-ports=N: set the number of forwarding ports "
>> @@ -535,6 +538,7 @@ launch_args_parse(int argc, char** argv)
>>   #endif
>>   		{ "tx-first",			0, 0, 0 },
>>   		{ "stats-period",		1, 0, 0 },
>> +		{ "display-xstats",		1, 0, 0 },
>>   		{ "nb-cores",			1, 0, 0 },
>>   		{ "nb-ports",			1, 0, 0 },
>>   		{ "coremask",			1, 0, 0 },
>> @@ -692,6 +696,16 @@ launch_args_parse(int argc, char** argv)
>>   				stats_period = n;
>>   				break;
>>   			}
>> +			if (!strcmp(lgopts[opt_idx].name, "display-xstats")) {
>> +				char rc;
>> +
>> +				rc = parse_xstats_list(optarg, &xstats_display,
>> +						       &xstats_display_num);
>> +				if (rc != 0)
>> +					rte_exit(EXIT_FAILURE,
>> +						 "Failed to fill xstats to display: %d\n",
> What about updating the error message something like: 'failed to parse
> display-xstats argument'
>
>> +						 rc);
>> +			}
>>   			if (!strcmp(lgopts[opt_idx].name,
>>   				    "eth-peers-configfile")) {
>>   				if (init_peer_eth_addrs(optarg) != 0)
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 6cbe9ba3c8..69a6a6913c 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -208,6 +208,11 @@ uint32_t param_total_num_mbufs = 0;  /**< number of mbufs in all pools - if
>>                                         * specified on command-line. */
>>   uint16_t stats_period; /**< Period to show statistics (disabled by default) */
>>   
>> +/** Extended statistics to show. */
>> +struct rte_eth_xstat_name *xstats_display;
>> +
>> +unsigned int xstats_display_num; /**< Size of extended statistics to show */
>> +
> what about renaming 'num' as 'size'? no strong opinion.
>
>>   /*
>>    * In container, it cannot terminate the process which running with 'stats-period'
>>    * option. Set flag to exit stats period loop after received SIGINT/SIGTERM.
>> @@ -542,6 +547,12 @@ uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN;
>>   /* Holds the registered mbuf dynamic flags names. */
>>   char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
>>   
>> +/** Fill helper structures for specified port to show extended statistics. */
>> +static void fill_display_xstats_info_for_port(portid_t pi);
>> +
> Can you please eliminate need of function declaration by rearranging the
> placement of static function?
>
>> +/** Fill helper structures for all ports to show extended statistics. */
>> +static void fill_display_xstats_info(void);
>> +
> This declaration seems can't be eliminated because of cross call (cyclic
> dependency) between functions.
I'll do it in the next version.
>
>>   /*
>>    * Helper function to check if socket is already discovered.
>>    * If yes, return positive value. If not, return zero.
>> @@ -2685,6 +2696,8 @@ start_port(portid_t pid)
>>   		}
>>   	}
>>   
>> +	fill_display_xstats_info_for_port(pid);
>> +
> Why do we need to do the 'fill' in each start?
>
> Let's assume we are in the interactive mode and on each stop/start, the
> 'xstat_display_info' will filled again with exact same values, because names get
> via application parameter and they won't change.
>
> Will it work to call this function in port init?
Device configuration may change in stopped state. So, it is just safer 
to resync on start.
>
>>   	printf("Done\n");
>>   	return 0;
>>   }
>> @@ -3719,6 +3732,40 @@ init_port_dcb_config(portid_t pid,
>>   	return 0;
>>   }
>>   
>> +static int
>> +alloc_display_xstats_info(portid_t pi)
> both 'xstats_display' and 'display_xstats' wording seems used for this feature,
> I suggest picking one and stick to it.
>
> And I am not sure about 'xstats_display', there are other commands in testpmd
> that displays the 'xstats', but not sure how to rename it.
Let's stick to 'xstats_display' then.
>> +{
>> +	uint64_t **ids = &ports[pi].xstats_info.ids;
>> +	uint64_t **ids_supp = &ports[pi].xstats_info.ids_supp;
>> +	uint64_t **prev_values = &ports[pi].xstats_info.prev_values;
>> +	uint64_t **curr_values = &ports[pi].xstats_info.curr_values;
>> +
>> +	if (xstats_display_num == 0)
>> +		return 0;
>> +
>> +	*ids = calloc(xstats_display_num, sizeof(**ids));
>> +	if (*ids == NULL)
>> +		return -ENOMEM;
>> +
>> +	*ids_supp = calloc(xstats_display_num, sizeof(**ids_supp));
>> +	if (*ids_supp == NULL)
>> +		return -ENOMEM;
>> +
>> +	*prev_values = calloc(xstats_display_num,
>> +			      sizeof(**prev_values));
>> +	if (*prev_values == NULL)
>> +		return -ENOMEM;
>> +
>> +	*curr_values = calloc(xstats_display_num,
>> +			      sizeof(**curr_values));
>> +	if (*curr_values == NULL)
>> +		return -ENOMEM;
>> +
>> +	ports[pi].xstats_info.allocated = true;
>> +
> We should free these memory at some point ...
I'll add free in the next version.
>
>> +	return 0;
>> +}
>> +
>>   static void
>>   init_port(void)
>>   {
>> @@ -3733,6 +3780,8 @@ init_port(void)
>>   				"rte_zmalloc(%d struct rte_port) failed\n",
>>   				RTE_MAX_ETHPORTS);
>>   	}
>> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
>> +		ports[i].xstats_info.allocated = false;
>>   	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
>>   		LIST_INIT(&ports[i].flow_tunnel_list);
>>   	/* Initialize ports NUMA structures */
>> @@ -3748,6 +3797,67 @@ force_quit(void)
>>   	prompt_exit();
>>   }
>>   
>> +static void
>> +fill_display_xstats_info_for_port(portid_t pi)
>> +{
>> +	unsigned int stat, stat_supp;
>> +	uint64_t *ids, *ids_supp;
>> +	const char *xstat_name;
>> +	struct rte_port *port;
>> +	int rc;
>> +
>> +	if (xstats_display_num == 0)
>> +		return;
>> +
>> +	if (pi == (portid_t)RTE_PORT_ALL) {
>> +		fill_display_xstats_info();
>> +		return;
>> +	}
>> +
>> +	port = &ports[pi];
>> +	if (port->port_status != RTE_PORT_STARTED)
> Why this requirement? Why port has to be started to get the ids of xstat names?

xstats could be extracted from HW and could be available in started 
state only.

It's more robust to require started state.

>
>> +		return;
>> +
>> +	if (!port->xstats_info.allocated && alloc_display_xstats_info(pi) != 0)
>> +		rte_exit(EXIT_FAILURE,
>> +			 "Failed to allocate xstats display memory\n");
>> +
>> +	ids = port->xstats_info.ids;
>> +	ids_supp = port->xstats_info.ids_supp;
>> +
>> +	for (stat = stat_supp = 0; stat < xstats_display_num; stat++) {
>> +		xstat_name = xstats_display[stat].name;
>> +
>> +		rc = rte_eth_xstats_get_id_by_name(pi, xstat_name,
>> +						   ids + stat);
>> +		if (rc != 0) {
>> +			ids[stat] = XSTAT_ID_INVALID;
>> +			fprintf(stderr, "No xstat '%s' on port %u - skip it\n",
>> +				xstat_name, pi);
>> +			continue;
>> +		}
>> +		ids_supp[stat_supp++] = ids[stat];
>> +	}
>> +
>> +	port->xstats_info.ids_supp_sz = stat_supp;
>> +}
>> +
>> +static void
>> +fill_display_xstats_info(void)
>> +{
>> +	portid_t pi;
>> +
>> +	if (xstats_display_num == 0)
>> +		return;
>> +
>> +	RTE_ETH_FOREACH_DEV(pi) {
>> +		if (pi == (portid_t)RTE_PORT_ALL)
>> +			continue;
> Do we really need this check? Can testpmd created more than 'RTE_PORT_ALL' port?
As I see in other places it's not required. I'll fix it.
>
>> +
>> +		fill_display_xstats_info_for_port(pi);
>> +	}
>> +}
>> +
>>   static void
>>   print_stats(void)
>>   {
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index 16a3598e48..68f182944b 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -195,6 +195,19 @@ struct tunnel_ops {
>>   	uint32_t items:1;
>>   };
>>   
>> +/** Information for an extended statistics to show. */
>> +struct xstat_display_info {
>> +	/** IDs of xstats in the order of xstats_display */
>> +	uint64_t *ids;
> I think we can drop 'ids' and just keep 'ids_supp', please see below comment on
> 'XSTAT_ID_INVALID'.
>
>> +	/** Supported xstats IDs in the order of xstats_display */
>> +	uint64_t *ids_supp;
>> +	size_t   ids_supp_sz;
>> +	uint64_t *prev_values;
>> +	uint64_t *curr_values;
>> +	uint64_t prev_ns;
>> +	bool	 allocated;
>> +};
>> +
>>   /**
>>    * The data structure associated with each port.
>>    */
>> @@ -234,6 +247,7 @@ struct rte_port {
>>   	/**< dynamic flags. */
>>   	uint64_t		mbuf_dynf;
>>   	const struct rte_eth_rxtx_callback *tx_set_dynf_cb[RTE_MAX_QUEUES_PER_PORT+1];
>> +	struct xstat_display_info xstats_info;
>>   };
>>   
>>   /**
>> @@ -434,6 +448,13 @@ extern uint32_t param_total_num_mbufs;
>>   
>>   extern uint16_t stats_period;
>>   
>> +extern struct rte_eth_xstat_name *xstats_display;
>> +extern unsigned int xstats_display_num;
>> +
>> +#define XSTAT_ID_INVALID UINT64_MAX
> Why you need this flag? Instead marking invalid xstats, why not just save the
> valid one and ignore invalid ones?
> Briefly, in 'xstat_display_info', drop the 'ids' and just keep 'ids_supp'.
You're right. I'll fix it.
>
>> +
>> +extern struct xstat_display_info xstats_per_port[];
>> +
> Is 'xstats_per_port' residue of previous implementations? It seems it is not needed.
Thanks, I'll remove it in the next version.
>>   extern uint16_t hairpin_mode;
>>   
>>   #ifdef RTE_LIB_LATENCYSTATS
>> @@ -766,6 +787,8 @@ inc_tx_burst_stats(struct fwd_stream *fs, uint16_t nb_tx)
>>   unsigned int parse_item_list(const char *str, const char *item_name,
>>   			unsigned int max_items,
>>   			unsigned int *parsed_items, int check_unique_values);
>> +int parse_xstats_list(const char *in_str, struct rte_eth_xstat_name **xstats,
>> +		      unsigned int *xstats_num);
>>   void launch_args_parse(int argc, char** argv);
>>   void cmdline_read_from_file(const char *filename);
>>   void prompt(void);
>> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
>> index 6061674239..d77afeb633 100644
>> --- a/doc/guides/testpmd_app_ug/run_app.rst
>> +++ b/doc/guides/testpmd_app_ug/run_app.rst
>> @@ -56,6 +56,11 @@ The command line options are:
>>       Display statistics every PERIOD seconds, if interactive mode is disabled.
>>       The default value is 0, which means that the statistics will not be displayed.
>>   
>> +*   ``--display-xstats xstat1[,...]``
>> +
>> +    Display extended statistics every PERIOD seconds as specified in ``--stats-period``
>> +    or when used with interactive commands that show Rx/Tx statistics (i.e. 'show port stats').
>> +
> Can you please highlight 'xstat1' is the xstat name, to prevent it to be
> confused with xstat id?
> Also can you please highlight that list should be comma separated, the short
> 'xstat1[,...]' usage implies this already but better to say it explicitly in the
> document.
>
I'll do it in the next version.
>>   *   ``--nb-cores=N``
>>   
>>       Set the number of forwarding cores,
>>