[dpdk-dev,v7,2/6] app/proc_info: add metrics displaying

  From: Reshma Pattan <reshma.pattan@intel.com>

Modify the dpdk-procinfo process to display the newly added metrics.
Added new command line option "--metrics" to display metrics.

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 app/proc_info/main.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Monday, January 16, 2017 4:19 PM
> To: dev@dpdk.org
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; Thomas Monjalon <thomas.monjalon@6wind.com>
> Subject: [dpdk-dev] [PATCH v7 2/6] app/proc_info: add metrics displaying
> 
> From: Reshma Pattan <reshma.pattan@intel.com>
> 
> Modify the dpdk-procinfo process to display the newly added metrics.
> Added new command line option "--metrics" to display metrics.
> 
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> Signed-off-by: Remy Horton <remy.horton@intel.com>

Note that current git HEAD has a bug in secondary process - this has already
been raised and fixed in patches on the ML. In testing, I manually fixed the
secondary proc issue, after which testing here worked successfully.

Comments inline.

> ---
>  app/proc_info/main.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/app/proc_info/main.c b/app/proc_info/main.c
> index 2c56d10..e5d626e 100644
> --- a/app/proc_info/main.c
> +++ b/app/proc_info/main.c
> @@ -57,6 +57,7 @@
>  #include <rte_atomic.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_string_fns.h>
> +#include <rte_metrics.h>
> 
>  /* Maximum long option length for option parsing. */
>  #define MAX_LONG_OPT_SZ 64
> @@ -68,6 +69,8 @@ static uint32_t enabled_port_mask;
>  static uint32_t enable_stats;
>  /**< Enable xstats. */
>  static uint32_t enable_xstats;
> +/**< Enable metrics. */
> +static uint32_t enable_metrics;
>  /**< Enable stats reset. */
>  static uint32_t reset_stats;
>  /**< Enable xstats reset. */
> @@ -85,6 +88,8 @@ proc_info_usage(const char *prgname)
>  		"  --stats: to display port statistics, enabled by default\n"
>  		"  --xstats: to display extended port statistics, disabled by "
>  			"default\n"
> +		"  --metrics: to display derived metrics of the ports, disabled by "
> +			"default\n"
>  		"  --stats-reset: to reset port statistics\n"
>  		"  --xstats-reset: to reset port extended statistics\n",

I presume there is no need for a --metrics-reset as the values are reset when printed, correct?

>  		prgname);
> @@ -127,6 +132,7 @@ proc_info_parse_args(int argc, char **argv)
>  		{"stats", 0, NULL, 0},
>  		{"stats-reset", 0, NULL, 0},
>  		{"xstats", 0, NULL, 0},
> +		{"metrics", 0, NULL, 0},
>  		{"xstats-reset", 0, NULL, 0},
>  		{NULL, 0, 0, 0}
>  	};
> @@ -159,6 +165,10 @@ proc_info_parse_args(int argc, char **argv)
>  			else if (!strncmp(long_option[option_index].name, "xstats",
>  					MAX_LONG_OPT_SZ))
>  				enable_xstats = 1;
> +			else if (!strncmp(long_option[option_index].name,
> +					"metrics",
> +					MAX_LONG_OPT_SZ))
> +				enable_metrics = 1;
>  			/* Reset stats */
>  			if (!strncmp(long_option[option_index].name, "stats-reset",
>  					MAX_LONG_OPT_SZ))
> @@ -301,6 +311,63 @@ nic_xstats_clear(uint8_t port_id)
>  	printf("\n  NIC extended statistics for port %d cleared\n", port_id);
>  }
> 
> +static void
> +metrics_display(int port_id)
> +{
> +	struct rte_metric_value *metrics;
> +	struct rte_metric_name *names;
> +	int len, ret;
> +	static const char *nic_stats_border = "########################";
> +
> +	memset(&metrics, 0, sizeof(struct rte_metric_value));
> +	len = rte_metrics_get_names(NULL, 0);
> +	if (len < 0) {
> +		printf("Cannot get metrics count\n");
> +		return;
> +	}
> +
> +	metrics = malloc(sizeof(struct rte_metric_value) * len);
> +	if (metrics == NULL) {
> +		printf("Cannot allocate memory for metrics\n");
> +		return;
> +	}

Is malloc() favoured over rte_malloc() for a reason?

> +
> +	names =  malloc(sizeof(struct rte_metric_name) * len);
> +	if (names == NULL) {
> +		printf("Cannot allocate memory for metrcis names\n");
> +		free(metrics);
> +		return;
> +	}
> +
> +	if (len != rte_metrics_get_names(names, len)) {
> +		printf("Cannot get metrics names\n");
> +		free(metrics);
> +		free(names);
> +		return;
> +	}
> +
> +	if (port_id == RTE_METRICS_GLOBAL)
> +		printf("###### Non port specific metrics  #########\n");
> +	else
> +		printf("###### metrics for port %-2d #########\n", port_id);
> +	printf("%s############################\n", nic_stats_border);
> +	ret = rte_metrics_get_values(port_id, metrics, len);
> +	if (ret < 0 || ret > len) {
> +		printf("Cannot get metrics values\n");
> +		free(metrics);
> +		free(names);
> +		return;
> +	}
> +
> +	int i;
> +	for (i = 0; i < len; i++)
> +		printf("%s: %"PRIu64"\n", names[i].name, metrics[i].value);
> +
> +	printf("%s############################\n", nic_stats_border);
> +	free(metrics);
> +	free(names);
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -360,8 +427,14 @@ main(int argc, char **argv)
>  				nic_stats_clear(i);
>  			else if (reset_xstats)
>  				nic_xstats_clear(i);
> +			else if (enable_metrics)
> +				metrics_display(i);
>  		}
>  	}
> 
> +	/* print port independent stats */
> +	if (enable_metrics)
> +		metrics_display(RTE_METRICS_GLOBAL);
> +
>  	return 0;
>  }
> --
> 2.5.5
  

On 17/01/2017 11:08, Van Haaren, Harry wrote:
[..]
>> +		"  --metrics: to display derived metrics of the ports, disabled by "
>> +			"default\n"
[..]
> I presume there is no need for a --metrics-reset as the values are reset when printed, correct?

Reset is not (currently) supported, as some of the expected future use 
of metrics is informational values that are not continually updated.


> Is malloc() favoured over rte_malloc() for a reason?

None I am aware of. There is a recent-ish preference for rte_malloc() 
over malloc() in new code, so will update..

..Remy