Re: [dpdk-dev] [PATCH v12 3/6] lib: add bitrate statistics library

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Monday, March 27, 2017 9:22 PM
> To: dev@dpdk.org
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
> Subject: [dpdk-dev] [PATCH v12 3/6] lib: add bitrate statistics library
> 
> This patch adds a library that calculates peak and average data-rate
> statistics. For ethernet devices. These statistics are reported using the
> metrics library.
> 
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> ---
> diff --git a/doc/guides/rel_notes/release_17_02.rst
> b/doc/guides/rel_notes/release_17_02.rst
> index 8bd706f..63786df 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -353,6 +353,7 @@ The libraries prepended with a plus sign were
> incremented in this version.
>  .. code-block:: diff
> 
>       librte_acl.so.2
> +   + librte_bitratestats.so.1

Should be removed here and added to 17.05.rst.

> a/lib/librte_bitratestats/Makefile b/lib/librte_bitratestats/Makefile
> new file mode 100644
> index 0000000..743b62c
> --- /dev/null
> +++ b/lib/librte_bitratestats/Makefile
> @@ -0,0 +1,53 @@
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2016-2017 Intel Corporation. All rights reserved.
> +#   All rights reserved.
> +#

Year correction here and other places

> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR)
> +

Adding -O3 flags?

> +int
> +rte_stats_bitrate_reg(struct rte_stats_bitrates *bitrate_data) {
> +	const char * const names[] = {
> +		"ewma_bits_in", "ewma_bits_out",
> +		"mean_bits_in", "mean_bits_out",
> +		"peak_bits_in", "peak_bits_out",
> +	};
> +	int return_value;
> +
> +	return_value = rte_metrics_reg_names(&names[0], 6);

Can we passing size of namesInstead of hard coded 6 here and in some other places below?

> +int
> +rte_stats_bitrate_calc(struct rte_stats_bitrates *bitrate_data,
> +	uint8_t port_id)
> +{
> +	/* Incoming bitrate. This is an iteratively calculated EWMA
> +	 * (Expomentially Weighted Moving Average) that uses a
> +	 * weighting factor of alpha_percent. An unsmoothed mean
> +	 * for just the current time delta is also calculated for the
> +	 * benefit of people who don't understand signal processing.
> +	 */
> +	cnt_bits = (eth_stats.ibytes - port_data->last_ibytes) << 3;
> +	port_data->last_ibytes = eth_stats.ibytes;
> +	if (cnt_bits > port_data->peak_ibits)
> +		port_data->peak_ibits = cnt_bits;
> +	delta = cnt_bits;
> +	delta -= port_data->ewma_ibits;
> +	/* The +-50 fixes integer rounding during divison */
> +	if (delta > 0)
> +		delta = (delta * alpha_percent + 50) / 100;
> +	else
> +		delta = (delta * alpha_percent - 50) / 100;
> +	port_data->ewma_ibits += delta;
> +	port_data->mean_ibits = cnt_bits;
> +
> +	/* Outgoing bitrate (also EWMA) */
> +	cnt_bits = (eth_stats.obytes - port_data->last_obytes) << 3;
> +	port_data->last_obytes = eth_stats.obytes;
> +	if (cnt_bits > port_data->peak_obits)
> +		port_data->peak_obits = cnt_bits;
> +	delta = cnt_bits;
> +	delta -= port_data->ewma_obits;
> +	delta = (delta * alpha_percent + 50) / 100;

Like input bitrate calculation, should we also add if else case for delta being < 0 and >0.

> diff --git a/lib/librte_bitratestats/rte_bitrate.h
> b/lib/librte_bitratestats/rte_bitrate.h
> new file mode 100644
> index 0000000..564e4f7
> --- /dev/null
> +++ b/lib/librte_bitratestats/rte_bitrate.h
> @@ -0,0 +1,80 @@
> +/*-

Include guards are missing for this file  #ifndef _RTE_BITRATE_H_ & #define _RTE_BITRATE_H_