[dpdk-dev,v7,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>
---
 MAINTAINERS                                        |   4 +
 config/common_base                                 |   5 +
 doc/api/doxy-api-index.md                          |   1 +
 doc/api/doxy-api.conf                              |   1 +
 doc/guides/rel_notes/release_17_02.rst             |   5 +
 lib/Makefile                                       |   1 +
 lib/librte_bitratestats/Makefile                   |  53 +++++++++
 lib/librte_bitratestats/rte_bitrate.c              | 132 +++++++++++++++++++++
 lib/librte_bitratestats/rte_bitrate.h              |  80 +++++++++++++
 .../rte_bitratestats_version.map                   |   9 ++
 mk/rte.app.mk                                      |   1 +
 11 files changed, 292 insertions(+)
 create mode 100644 lib/librte_bitratestats/Makefile
 create mode 100644 lib/librte_bitratestats/rte_bitrate.c
 create mode 100644 lib/librte_bitratestats/rte_bitrate.h
 create mode 100644 lib/librte_bitratestats/rte_bitratestats_version.map
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Monday, January 16, 2017 4:20 PM
> To: dev@dpdk.org
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
> Subject: [dpdk-dev] [PATCH v7 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>

Comments inline

> ---
>  MAINTAINERS                                        |   4 +
>  config/common_base                                 |   5 +
>  doc/api/doxy-api-index.md                          |   1 +
>  doc/api/doxy-api.conf                              |   1 +
>  doc/guides/rel_notes/release_17_02.rst             |   5 +
>  lib/Makefile                                       |   1 +
>  lib/librte_bitratestats/Makefile                   |  53 +++++++++
>  lib/librte_bitratestats/rte_bitrate.c              | 132 +++++++++++++++++++++
>  lib/librte_bitratestats/rte_bitrate.h              |  80 +++++++++++++
>  .../rte_bitratestats_version.map                   |   9 ++
>  mk/rte.app.mk                                      |   1 +
>  11 files changed, 292 insertions(+)
>  create mode 100644 lib/librte_bitratestats/Makefile
>  create mode 100644 lib/librte_bitratestats/rte_bitrate.c
>  create mode 100644 lib/librte_bitratestats/rte_bitrate.h
>  create mode 100644 lib/librte_bitratestats/rte_bitratestats_version.map
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4a19497..6cd9896 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -601,6 +601,10 @@ M: Remy Horton <remy.horton@intel.com>
>  F: lib/librte_metrics/
>  F: doc/guides/sample_app_ug/keep_alive.rst
> 
> +Bit-rate statistica
> +M: Remy Horton <remy.horton@intel.com>
> +F: lib/librte_bitratestats/
> +
> 
>  Test Applications
>  -----------------
> diff --git a/config/common_base b/config/common_base
> index 0eb3866..decebe5 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -598,3 +598,8 @@ CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS=n
>  # Compile the device metrics library
>  #
>  CONFIG_RTE_LIBRTE_METRICS=y
> +
> +#
> +# Compile the bitrate statistics library
> +#
> +CONFIG_RTE_LIBRTE_BITRATE=y
> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index 94f0f69..5e194b0 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -151,4 +151,5 @@ There are many libraries, so their headers may be grouped by topics:
>    [ABI compat]         (@ref rte_compat.h),
>    [keepalive]          (@ref rte_keepalive.h),
>    [Device Metrics]     (@ref rte_metrics.h),
> +  [Bitrate Statistics] (@ref rte_bitrate.h),
>    [version]            (@ref rte_version.h)
> diff --git a/doc/api/doxy-api.conf b/doc/api/doxy-api.conf
> index 194b670..6e6ab5c 100644
> --- a/doc/api/doxy-api.conf
> +++ b/doc/api/doxy-api.conf
> @@ -35,6 +35,7 @@ INPUT                   = doc/api/doxy-api-index.md \
>                            lib/librte_eal/common/include \
>                            lib/librte_eal/common/include/generic \
>                            lib/librte_acl \
> +                          lib/librte_bitratestats \
>                            lib/librte_cfgfile \
>                            lib/librte_cmdline \
>                            lib/librte_compat \
> diff --git a/doc/guides/rel_notes/release_17_02.rst
> b/doc/guides/rel_notes/release_17_02.rst
> index 4fca29b..44012c8 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -40,6 +40,11 @@ New Features
>       intended to provide a reporting mechanism that is independent of the
>       ethdev library.
> 
> +   * **Added bit-rate calculation library.**
> +
> +     A library that can be used to calculate device bit-rates. Calculated
> +     bitrates are reported using the metrics library.
> +
>       This section is a comment. do not overwrite or remove it.
>       Also, make sure to start the actual text at the margin.
>       =========================================================
> diff --git a/lib/Makefile b/lib/Makefile
> index 5d85dcf..e211bc0 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -59,6 +59,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += librte_pipeline
>  DIRS-$(CONFIG_RTE_LIBRTE_REORDER) += librte_reorder
>  DIRS-$(CONFIG_RTE_LIBRTE_PDUMP) += librte_pdump
>  DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics
> +DIRS-$(CONFIG_RTE_LIBRTE_BITRATE) += librte_bitratestats
> 
>  ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
>  DIRS-$(CONFIG_RTE_LIBRTE_KNI) += librte_kni
> diff --git a/lib/librte_bitratestats/Makefile b/lib/librte_bitratestats/Makefile
> new file mode 100644
> index 0000000..b725d4e
> --- /dev/null
> +++ b/lib/librte_bitratestats/Makefile
> @@ -0,0 +1,53 @@
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2016 Intel Corporation. All rights reserved.
> +#   All rights reserved.

Add -2017


> +#
> +#   Redistribution and use in source and binary forms, with or without
> +#   modification, are permitted provided that the following conditions
> +#   are met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +#       notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above copyright
> +#       notice, this list of conditions and the following disclaimer in
> +#       the documentation and/or other materials provided with the
> +#       distribution.
> +#     * Neither the name of Intel Corporation nor the names of its
> +#       contributors may be used to endorse or promote products derived
> +#       from this software without specific prior written permission.
> +#
> +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +# library name
> +LIB = librte_bitratestats.a
> +
> +CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR)
> +
> +EXPORT_MAP := rte_bitratestats_version.map
> +
> +LIBABIVER := 1
> +
> +# all source are stored in SRCS-y
> +SRCS-$(CONFIG_RTE_LIBRTE_BITRATE) := rte_bitrate.c
> +
> +# Install header file
> +SYMLINK-$(CONFIG_RTE_LIBRTE_BITRATE)-include += rte_bitrate.h
> +
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_BITRATE) += lib/librte_eal
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_BITRATE) += lib/librte_ether
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_BITRATE) += lib/librte_metrics
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_bitratestats/rte_bitrate.c b/lib/librte_bitratestats/rte_bitrate.c
> new file mode 100644
> index 0000000..c8d9a20
> --- /dev/null
> +++ b/lib/librte_bitratestats/rte_bitrate.c
> @@ -0,0 +1,132 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   All rights reserved.

-2017

> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <rte_common.h>
> +#include <rte_ethdev.h>
> +#include <rte_malloc.h>
> +#include <rte_metrics.h>
> +#include <rte_bitrate.h>
> +
> +/*
> + * Persistent bit-rate data.
> + * @internal
> + */
> +struct rte_stats_bitrate_s {
> +	uint64_t last_ibytes;
> +	uint64_t last_obytes;
> +	uint64_t peak_ibits;
> +	uint64_t peak_obits;
> +	uint64_t ewma_ibits;
> +	uint64_t ewma_obits;
> +};

Nit: same as previous metrics patch, any reason for the _s at the end? Same for following structs:

> +
> +struct rte_stats_bitrates_s {
> +	struct rte_stats_bitrate_s port_stats[RTE_MAX_ETHPORTS];
> +	uint16_t id_stats_set;
> +};
> +
> +struct rte_stats_bitrates_s *
> +rte_stats_bitrate_create(void)
> +{
> +	return rte_zmalloc(NULL, sizeof(struct rte_stats_bitrates_s),
> +		RTE_CACHE_LINE_SIZE);
> +}

Is the socket relevant here? Perhaps pass socket_id to the function, and use rte_zmalloc_socket(). This function has no way of initializing bitrate structs on two different sockets, using one a single setup thread.

> +
> +int
> +rte_stats_bitrate_reg(struct rte_stats_bitrates_s *bitrate_data)
> +{
> +	const char * const names[] = {
> +		"mean_bits_in", "mean_bits_out",
> +		"peak_bits_in", "peak_bits_out",
> +	};
> +	int return_value;
> +
> +	return_value = rte_metrics_reg_metrics(&names[0], 4);
> +	if (return_value >= 0)
> +		bitrate_data->id_stats_set = return_value;
> +	return return_value;
> +}
> +
> +int
> +rte_stats_bitrate_calc(struct rte_stats_bitrates_s *bitrate_data,
> +	uint8_t port_id)
> +{
> +	struct rte_stats_bitrate_s *port_data;
> +	struct rte_eth_stats eth_stats;
> +	int ret_code;
> +	uint64_t cnt_bits;
> +	int64_t delta;
> +	const int64_t alpha_percent = 20;
> +	uint64_t values[4];
> +
> +	ret_code = rte_eth_stats_get(port_id, &eth_stats);
> +	if (ret_code != 0)
> +		return ret_code;
> +
> +	port_data = &bitrate_data->port_stats[port_id];
> +
> +	/* Incoming bitrate. This is an iteratively calculated EWMA
> +	 * (Expomentially Weighted Moving Average) that uses a
> +	 * weighting factor of alpha_percent.
> +	 */
> +	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;

The integer +50 feels a bit odd, I'm not opposed to this if it works though.
The latency stats uses a float alpha multiplier, is that a
cleaner solution? The two libraries should probably be consistent anyway.

> +
> +	/* 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;

Same float multiplier comment.

> +	port_data->ewma_obits += delta;
> +
> +	values[0] = port_data->ewma_ibits;
> +	values[1] = port_data->ewma_obits;
> +	values[2] = port_data->peak_ibits;
> +	values[3] = port_data->peak_obits;
> +	rte_metrics_update_metrics(port_id, bitrate_data->id_stats_set,
> +		values, 4);
> +	return 0;
> +}
> diff --git a/lib/librte_bitratestats/rte_bitrate.h b/lib/librte_bitratestats/rte_bitrate.h
> new file mode 100644
> index 0000000..bc87c5e
> --- /dev/null
> +++ b/lib/librte_bitratestats/rte_bitrate.h
> @@ -0,0 +1,80 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   All rights reserved.

-2017

> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +
> +/**
> + *  Bitrate statistics data structure.
> + *  This data structure is intentionally opaque.
> + */
> +struct rte_stats_bitrates_s;

_s question as previously highlighted.

> +
> +
> +/**
> + * Allocate a bitrate statistics structure
> + *
> + * @return
> + *   - Pointer to structure on success
> + *   - NULL on error (zmalloc failure)
> + */
> +struct rte_stats_bitrates_s *rte_stats_bitrate_create(void);
> +
> +
> +/**
> + * Register bitrate statistics with the metric library.
> + *
> + * @param bitrate_data
> + *   Pointer allocated by rte_stats_create()
> + *
> + * @return
> + *   Zero on success
> + *   Negative on error
> + */
> +int rte_stats_bitrate_reg(struct rte_stats_bitrates_s *bitrate_data);
> +
> +
> +/**
> + * Calculate statistics for current time window. The period with which
> + * this function is called should be the intended sampling window width.
> + *
> + * @param bitrate_data
> + *   Bitrate statistics data pointer
> + *
> + * @param port_id
> + *   Port id to calculate statistics for
> + *
> + * @return
> + *  - Zero on success
> + *  - Negative value on error
> + */
> +int rte_stats_bitrate_calc(struct rte_stats_bitrates_s *bitrate_data,
> +	uint8_t port_id);
> diff --git a/lib/librte_bitratestats/rte_bitratestats_version.map
> b/lib/librte_bitratestats/rte_bitratestats_version.map
> new file mode 100644
> index 0000000..66f232f
> --- /dev/null
> +++ b/lib/librte_bitratestats/rte_bitratestats_version.map
> @@ -0,0 +1,9 @@
> +DPDK_17.02 {
> +	global:
> +
> +	rte_stats_bitrate_calc;
> +	rte_stats_bitrate_create;
> +	rte_stats_bitrate_reg;
> +
> +	local: *;
> +};
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index 40fcf33..6aac5ac 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -99,6 +99,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_EAL)            += -lrte_eal
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_CMDLINE)        += -lrte_cmdline
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_METRICS)        += -lrte_metrics
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_BITRATE)        += -lrte_bitratestats
> 
> 
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BOND)       += -lrte_pmd_bond
> --
> 2.5.5
  

On 17/01/2017 11:16, Van Haaren, Harry wrote:
[..]
>> +struct rte_stats_bitrates_s *
>> +rte_stats_bitrate_create(void)
>> +{
>> +	return rte_zmalloc(NULL, sizeof(struct rte_stats_bitrates_s),
>> +		RTE_CACHE_LINE_SIZE);
>> +}
>
> Is the socket relevant here? Perhaps pass socket_id to the function,
> and use rte_zmalloc_socket(). This function has no way of
> initializing bitrate structs on two different sockets, using one a
> single setup thread.

Not an issue in this case. It is expected that the thread that calls 
this creator will also be the thread that clocks the library (i.e. 
calling rte_stats_bitrate_calc()), and that one instance handles all 
ports. The memory block is not accessed outside of rte_stats_bitrate_calc().


>> +	/* 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;
>
> The integer +50 feels a bit odd, I'm not opposed to this if it works though.

It was based on feedback regarding roundoffs when doing integer divides. 
Something about rounding to nearest integer rather than towards zero if 
I remember correctly..


>> +/**
>> + *  Bitrate statistics data structure.
>> + *  This data structure is intentionally opaque.
>> + */
>> +struct rte_stats_bitrates_s;
>
> _s question as previously highlighted.

Since this is a public structure, removed the _s prefix in this case. 
Looking over the coding guidelines I can't see anything explicitly 
against the notation, but existing structures have not used it.

..Remy