[dpdk-dev] [PATCH 0/5] add apistats function

[dpdk-dev] [PATCH 0/5] add apistats function

[Hideyuki Yamashita]

In general, DPDK application consumes CPU usage because it polls
incoming packets using rx_burst API in infinite loop.
This makes difficult to estimate how much CPU usage is really
used to send/receive packets by the DPDK application.

For example, even if no incoming packets arriving, CPU usage
looks nearly 100% when observed by top command.

It is beneficial if developers can observe real CPU usage of the
DPDK application.
Such information can be exported to monitoring application like
prometheus/graphana and shows CPU usage graphically.

To achieve above, this patch set provides apistats functionality.
apistats provides the followiing two counters for each lcore.
- rx_burst_counts[RTE_MAX_LCORE]
- tx_burst_counts[RTE_MAX_LCORE]
Those accumulates rx_burst/tx_burst counts since the application starts.

By using those values, developers can roughly estimate CPU usage.
Let us assume a DPDK application is simply forwarding packets.
It calls tx_burst only if it receive packets.
If rx_burst_counts=1000 and tx_burst_count=1000 during certain
period of time, one can assume CPU usage is 100%.
If rx_burst_counts=1000 and tx_burst_count=100 during certain
period of time, one can assume CPU usage is 10%.
Here we assumes that tx_burst_count equals counts which rx_burst function
really receives incoming packets.


This patch set provides the following.
- basic API counting functionality(apistats) into librte_ethdev
- add code to testpmd to accumulate counter information
- add code to proc-info to retrieve above mentioned counter information
- add description in proc-info document about --apistats parameter
- modify MAINTAINERS file for apistats.c and apistats.h

Hideyuki Yamashita (5):
  maintainers: update maintainers file for apistats
  app/proc-info: add to use apistats
  app/test-pmd: add to use apistats
  docs: add description of apistats parameter into proc-info
  librte_ethdev: add to use apistats

 MAINTAINERS                      |  3 ++
 app/proc-info/main.c             | 46 +++++++++++++++++++++++
 app/test-pmd/testpmd.c           |  4 ++
 doc/guides/tools/proc_info.rst   | 10 ++++-
 lib/librte_ethdev/meson.build    |  6 ++-
 lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h   |  7 ++++
 lib/librte_ethdev/version.map    |  5 +++
 9 files changed, 205 insertions(+), 4 deletions(-)
 create mode 100644 lib/librte_ethdev/rte_apistats.c
 create mode 100644 lib/librte_ethdev/rte_apistats.h

-- 
2.18.0

[dpdk-dev] [PATCH 1/5] maintainers: update maintainers file for apistats

[Hideyuki Yamashita]

This patch adds maintainer of rte_apistats.c and rte_apistats.h.

Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index eafe9f8..dba2acf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -489,6 +489,9 @@ F: drivers/raw/skeleton/
 F: app/test/test_rawdev.c
 F: doc/guides/prog_guide/rawdev.rst
 
+API stats API -EXPERIMENTAL
+M: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>
+F: lib/librte_ethdev/rte_apistats.*
 
 Memory Pool Drivers
 -------------------
-- 
2.18.0

[dpdk-dev] [PATCH 2/5] app/proc-info: add to use apistats

[Hideyuki Yamashita]

This patch modifies to use apistats by proc-info app.
- change on main.c to call apistats functions
- change on Makefile to include experimental API

Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
---
 app/proc-info/main.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index d743209..d384b13 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -37,6 +37,7 @@
 #include <rte_cryptodev.h>
 #include <rte_tm.h>
 #include <rte_hexdump.h>
+#include <rte_apistats.h>
 
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64
@@ -93,6 +94,8 @@ static char *mempool_name;
 /**< Enable iter mempool. */
 static uint32_t enable_iter_mempool;
 static char *mempool_iter_name;
+/**< Enable api stats. */
+static uint32_t enable_apistats;
 
 /**< display usage */
 static void
@@ -109,6 +112,7 @@ proc_info_usage(const char *prgname)
 		"  --xstats-name NAME: to display single xstat id by NAME\n"
 		"  --xstats-ids IDLIST: to display xstat values by id. "
 			"The argument is comma-separated list of xstat ids to print out.\n"
+		"  --apistats: to display api statistics, disabled by default\n"
 		"  --stats-reset: to reset port statistics\n"
 		"  --xstats-reset: to reset port extended statistics\n"
 		"  --collectd-format: to print statistics to STDOUT in expected by collectd format\n"
@@ -218,6 +222,7 @@ proc_info_parse_args(int argc, char **argv)
 		{"xstats-name", required_argument, NULL, 1},
 		{"collectd-format", 0, NULL, 0},
 		{"xstats-ids", 1, NULL, 1},
+		{"apistats", 0, NULL, 0},
 		{"host-id", 0, NULL, 0},
 		{"show-port", 0, NULL, 0},
 		{"show-tm", 0, NULL, 0},
@@ -266,6 +271,9 @@ proc_info_parse_args(int argc, char **argv)
 			else if (!strncmp(long_option[option_index].name, "xstats-reset",
 					MAX_LONG_OPT_SZ))
 				reset_xstats = 1;
+			else if (!strncmp(long_option[option_index].name, "apistats",
+					MAX_LONG_OPT_SZ))
+				enable_apistats = 1;
 			else if (!strncmp(long_option[option_index].name,
 					"show-port", MAX_LONG_OPT_SZ))
 				enable_shw_port = 1;
@@ -1349,6 +1357,40 @@ iter_mempool(char *name)
 	}
 }
 
+static void
+display_apistats(void)
+{
+	static const char *api_stats_border = "########################";
+	uint16_t i;
+	struct rte_apistats t0_count, t1_count;
+	memcpy(&t0_count, rte_apicounts, sizeof(struct rte_apistats));
+	sleep(1);
+	memcpy(&t1_count, rte_apicounts, sizeof(struct rte_apistats));
+	for (i = 0; i < RTE_MAX_LCORE; i++) {
+		if (t1_count.lcoreid_list[i] != 0) {
+			uint64_t rx_count_delta = t1_count.rx_burst_counts[i]
+				- t0_count.rx_burst_counts[i];
+			uint64_t tx_count_delta = t1_count.tx_burst_counts[i]
+				- t0_count.tx_burst_counts[i];
+			printf("\n  %s api statistics for lcoreid: %d %s\n",
+				api_stats_border, i, api_stats_border);
+			printf("  rx_burst_count: %13"PRIu64""
+				" rx_burst_count_delta: %13"PRIu64"\n",
+				t1_count.rx_burst_counts[i],
+				rx_count_delta);
+			printf("  tx_burst_count: %13"PRIu64""
+				" tx_burst_count_delta: %13"PRIu64"\n",
+				t1_count.tx_burst_counts[i],
+				tx_count_delta);
+			printf("  tx/rx_rate: %3.2f\n",
+				(rx_count_delta) ? (double)tx_count_delta
+				/ rx_count_delta : 0.0);
+			printf("  %s################################%s\n",
+				api_stats_border, api_stats_border);
+		}
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1389,6 +1431,8 @@ main(int argc, char **argv)
 	if (!rte_eal_primary_proc_alive(NULL))
 		rte_exit(EXIT_FAILURE, "No primary DPDK process is running.\n");
 
+	rte_apistats_init();
+
 	/* parse app arguments */
 	ret = proc_info_parse_args(argc, argv);
 	if (ret < 0)
@@ -1454,6 +1498,8 @@ main(int argc, char **argv)
 		show_mempool(mempool_name);
 	if (enable_iter_mempool)
 		iter_mempool(mempool_iter_name);
+	if (enable_apistats)
+		display_apistats();
 
 	RTE_ETH_FOREACH_DEV(i)
 		rte_eth_dev_close(i);
-- 
2.18.0

[dpdk-dev] [PATCH 3/5] app/test-pmd: add to use apistats

[Hideyuki Yamashita]

This patch modifies to use apistats by testpmd app.
- change on testpmd.c to call apistats functions to accumlate stats info

Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
---
 app/test-pmd/testpmd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 33fc0fd..e782bfe 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -60,6 +60,8 @@
 #ifdef RTE_LIB_LATENCYSTATS
 #include <rte_latencystats.h>
 #endif
+#include <rte_apistats.h>
+
 
 #include "testpmd.h"
 
@@ -3958,6 +3960,8 @@ main(int argc, char** argv)
 	}
 #endif
 
+	rte_apistats_init();
+
 	/* Setup bitrate stats */
 #ifdef RTE_LIB_BITRATESTATS
 	if (bitrate_enabled != 0) {
-- 
2.18.0

[dpdk-dev] [PATCH 4/5] docs: add description of apistats parameter into proc-info

[Hideyuki Yamashita]

This patch modifies document of proc-info to introduce "--apistats"
parameter.

Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
---
 doc/guides/tools/proc_info.rst | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/doc/guides/tools/proc_info.rst b/doc/guides/tools/proc_info.rst
index 9772d97..4c7b79e 100644
--- a/doc/guides/tools/proc_info.rst
+++ b/doc/guides/tools/proc_info.rst
@@ -18,8 +18,9 @@ The application has a number of command line options:
 .. code-block:: console
 
    ./<build_dir>/app/dpdk-procinfo -- -m | [-p PORTMASK] [--stats | --xstats |
-   --stats-reset | --xstats-reset] [ --show-port | --show-tm | --show-crypto |
-   --show-ring[=name] | --show-mempool[=name] | --iter-mempool=name ]
+   --stats-reset | --xstats-reset | --apistats ] [ --show-port | --show-tm |
+   --show-crypto | --show-ring[=name] | --show-mempool[=name] |
+   --iter-mempool=name ]
 
 Parameters
 ~~~~~~~~~~
@@ -41,6 +42,11 @@ no port mask is specified, the generic stats are reset for all DPDK ports.
 The xstats-reset parameter controls the resetting of extended port statistics.
 If no port mask is specified xstats are reset for all DPDK ports.
 
+**--apistats**
+The apistats parameter controls rx_burst/tx_burst API invocation counter
+statistics per core. If no port mask is specified apistats are printed for all
+DPDK ports.
+
 **-m**: Print DPDK memory information.
 
 **--show-port**
-- 
2.18.0

[dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Hideyuki Yamashita]

This patch modifies to use apistats by librte_ethdev.

Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
---
 lib/librte_ethdev/meson.build    |  6 ++-
 lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h   |  7 ++++
 lib/librte_ethdev/version.map    |  5 +++
 5 files changed, 144 insertions(+), 2 deletions(-)
 create mode 100644 lib/librte_ethdev/rte_apistats.c
 create mode 100644 lib/librte_ethdev/rte_apistats.h

diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
index e4b6102..d03e784 100644
--- a/lib/librte_ethdev/meson.build
+++ b/lib/librte_ethdev/meson.build
@@ -8,7 +8,8 @@ sources = files('ethdev_private.c',
 	'rte_ethdev.c',
 	'rte_flow.c',
 	'rte_mtr.c',
-	'rte_tm.c')
+	'rte_tm.c' ,
+        'rte_apistats.c')
 
 headers = files('rte_ethdev.h',
 	'rte_ethdev_driver.h',
@@ -24,6 +25,7 @@ headers = files('rte_ethdev.h',
 	'rte_mtr.h',
 	'rte_mtr_driver.h',
 	'rte_tm.h',
-	'rte_tm_driver.h')
+	'rte_tm_driver.h',
+        'rte_apistats.h')
 
 deps += ['net', 'kvargs', 'meter', 'telemetry']
diff --git a/lib/librte_ethdev/rte_apistats.c b/lib/librte_ethdev/rte_apistats.c
new file mode 100644
index 0000000..c4bde34
--- /dev/null
+++ b/lib/librte_ethdev/rte_apistats.c
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 NTT TechnoCross Corporation
+ */
+
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <string.h>
+#include <rte_log.h>
+#include <rte_memzone.h>
+#include <rte_lcore.h>
+
+#include "rte_apistats.h"
+
+/* Macros for printing using RTE_LOG */
+#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
+
+#define MZ_APISTATS "rte_apistats"
+
+struct rte_apistats *rte_apicounts;
+
+int rte_apistats_init(void)
+{
+	int i;
+	const struct rte_memzone *mz = NULL;
+	const unsigned int flags = 0;
+
+	/** Allocate stats in shared memory fo multi process support */
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		mz = rte_memzone_lookup(MZ_APISTATS);
+		if (mz == NULL) {
+			RTE_LOG(ERR, APISTATS, "Cannot get info structure\n");
+			return -1;
+		}
+		rte_apicounts = mz->addr;
+	} else {
+		/* RTE_PROC_PRIMARY */
+		mz = rte_memzone_reserve(MZ_APISTATS, sizeof(*rte_apicounts),
+			rte_socket_id(), flags);
+		if (mz == NULL) {
+			RTE_LOG(ERR, APISTATS, "Cannot reserve memory zone\n");
+			return -ENOMEM;
+		}
+		rte_apicounts = mz->addr;
+		memset(rte_apicounts, 0, sizeof(*rte_apicounts));
+	}
+
+	/* set up array for data */
+	RTE_LCORE_FOREACH(i) {
+		rte_apicounts->lcoreid_list[i] = 1;
+		RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n", i);
+	}
+	return 0;
+}
+
+int rte_apistats_uninit(void)
+{
+	const struct rte_memzone *mz = NULL;
+	/* free up the memzone */
+	mz = rte_memzone_lookup(MZ_APISTATS);
+	if (mz)
+		rte_memzone_free(mz);
+	return 0;
+}
diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h
new file mode 100644
index 0000000..afea50e
--- /dev/null
+++ b/lib/librte_ethdev/rte_apistats.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 NTT TechnoCross Corporation
+ */
+
+#ifndef _RTE_APISTATS_H_
+#define _RTE_APISTATS_H_
+
+/**
+ * @file
+ * RTE apistats
+ *
+ * library to provide rte_rx_burst/tx_burst api stats.
+ */
+
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <rte_compat.h>
+#include <rte_lcore.h>
+
+/**
+ * A structure for rte_rx_burst/tx_burst api statistics.
+ */
+struct rte_apistats {
+	int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
+	/**< Total rte_rx_burst call counts */
+	uint64_t rx_burst_counts[RTE_MAX_LCORE];
+
+	/**< Total rte_tx_burst call counts */
+	uint64_t tx_burst_counts[RTE_MAX_LCORE];
+};
+
+extern struct rte_apistats *rte_apicounts;
+
+/**
+ *  Initialize rte_rx_burst/tx_burst call count area.
+ *  @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ *  @return
+ *   -1     : On error
+ *   -ENOMEM: On error
+ *    0     : On success
+ */
+__rte_experimental
+int rte_apistats_init(void);
+
+/**
+ *  Clean up and free memory.
+ *  @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ *  @return
+ *   -1: On error
+ *    0: On success
+ */
+__rte_experimental
+int rte_apistats_uninit(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_APISTATS_H_ */
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index f5f8919..bef9bc6 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -160,6 +160,7 @@ extern "C" {
 
 #include "rte_ethdev_trace_fp.h"
 #include "rte_dev_info.h"
+#include <rte_apistats.h>
 
 extern int rte_eth_dev_logtype;
 
@@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
 				     rx_pkts, nb_pkts);
 
+	int lcore_id = rte_lcore_id();
+	rte_apicounts->rx_burst_counts[lcore_id]++;
+
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
 	struct rte_eth_rxtx_callback *cb;
 
@@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
 	}
 #endif
 
+	int lcore_id = rte_lcore_id();
+	rte_apicounts->tx_burst_counts[lcore_id]++;
+
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
 	struct rte_eth_rxtx_callback *cb;
 
diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
index d3f5410..adea432 100644
--- a/lib/librte_ethdev/version.map
+++ b/lib/librte_ethdev/version.map
@@ -240,6 +240,11 @@ EXPERIMENTAL {
 	rte_flow_get_restore_info;
 	rte_flow_tunnel_action_decap_release;
 	rte_flow_tunnel_item_release;
+
+        # added in 21.02
+        rte_apistats_init;
+        rte_apistats_uninit;
+        rte_apicounts;
 };
 
 INTERNAL {
-- 
2.18.0

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Ferruh Yigit]

On 12/4/2020 7:51 AM, Hideyuki Yamashita wrote:
> In general, DPDK application consumes CPU usage because it polls
> incoming packets using rx_burst API in infinite loop.
> This makes difficult to estimate how much CPU usage is really
> used to send/receive packets by the DPDK application.
> 
> For example, even if no incoming packets arriving, CPU usage
> looks nearly 100% when observed by top command.
> 
> It is beneficial if developers can observe real CPU usage of the
> DPDK application.
> Such information can be exported to monitoring application like
> prometheus/graphana and shows CPU usage graphically.
> 
> To achieve above, this patch set provides apistats functionality.
> apistats provides the followiing two counters for each lcore.
> - rx_burst_counts[RTE_MAX_LCORE]
> - tx_burst_counts[RTE_MAX_LCORE]
> Those accumulates rx_burst/tx_burst counts since the application starts.
> 
> By using those values, developers can roughly estimate CPU usage.
> Let us assume a DPDK application is simply forwarding packets.
> It calls tx_burst only if it receive packets.
> If rx_burst_counts=1000 and tx_burst_count=1000 during certain
> period of time, one can assume CPU usage is 100%.
> If rx_burst_counts=1000 and tx_burst_count=100 during certain
> period of time, one can assume CPU usage is 10%.
> Here we assumes that tx_burst_count equals counts which rx_burst function
> really receives incoming packets.
> 
> 
> This patch set provides the following.
> - basic API counting functionality(apistats) into librte_ethdev
> - add code to testpmd to accumulate counter information
> - add code to proc-info to retrieve above mentioned counter information
> - add description in proc-info document about --apistats parameter
> - modify MAINTAINERS file for apistats.c and apistats.h
> 
> Hideyuki Yamashita (5):
>    maintainers: update maintainers file for apistats
>    app/proc-info: add to use apistats
>    app/test-pmd: add to use apistats
>    docs: add description of apistats parameter into proc-info
>    librte_ethdev: add to use apistats
> 
>   MAINTAINERS                      |  3 ++
>   app/proc-info/main.c             | 46 +++++++++++++++++++++++
>   app/test-pmd/testpmd.c           |  4 ++
>   doc/guides/tools/proc_info.rst   | 10 ++++-
>   lib/librte_ethdev/meson.build    |  6 ++-
>   lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
>   lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
>   lib/librte_ethdev/rte_ethdev.h   |  7 ++++
>   lib/librte_ethdev/version.map    |  5 +++
>   9 files changed, 205 insertions(+), 4 deletions(-)
>   create mode 100644 lib/librte_ethdev/rte_apistats.c
>   create mode 100644 lib/librte_ethdev/rte_apistats.h
> 

cc'ed Dave Hunt. As far as I remember he did something for same goal in the 
past, but in a different way, he can comment better.

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[David Hunt]

On 4/12/2020 7:51 AM, Hideyuki Yamashita wrote:
> In general, DPDK application consumes CPU usage because it polls
> incoming packets using rx_burst API in infinite loop.
> This makes difficult to estimate how much CPU usage is really
> used to send/receive packets by the DPDK application.
>
> For example, even if no incoming packets arriving, CPU usage
> looks nearly 100% when observed by top command.
>
> It is beneficial if developers can observe real CPU usage of the
> DPDK application.
> Such information can be exported to monitoring application like
> prometheus/graphana and shows CPU usage graphically.
>
> To achieve above, this patch set provides apistats functionality.
> apistats provides the followiing two counters for each lcore.
> - rx_burst_counts[RTE_MAX_LCORE]
> - tx_burst_counts[RTE_MAX_LCORE]
> Those accumulates rx_burst/tx_burst counts since the application starts.
>
> By using those values, developers can roughly estimate CPU usage.
> Let us assume a DPDK application is simply forwarding packets.
> It calls tx_burst only if it receive packets.
> If rx_burst_counts=1000 and tx_burst_count=1000 during certain
> period of time, one can assume CPU usage is 100%.
> If rx_burst_counts=1000 and tx_burst_count=100 during certain
> period of time, one can assume CPU usage is 10%.
> Here we assumes that tx_burst_count equals counts which rx_burst function
> really receives incoming packets.
>
>
> This patch set provides the following.
> - basic API counting functionality(apistats) into librte_ethdev
> - add code to testpmd to accumulate counter information
> - add code to proc-info to retrieve above mentioned counter information
> - add description in proc-info document about --apistats parameter
> - modify MAINTAINERS file for apistats.c and apistats.h
>
> Hideyuki Yamashita (5):
>    maintainers: update maintainers file for apistats
>    app/proc-info: add to use apistats
>    app/test-pmd: add to use apistats
>    docs: add description of apistats parameter into proc-info
>    librte_ethdev: add to use apistats
>
>   MAINTAINERS                      |  3 ++
>   app/proc-info/main.c             | 46 +++++++++++++++++++++++
>   app/test-pmd/testpmd.c           |  4 ++
>   doc/guides/tools/proc_info.rst   | 10 ++++-
>   lib/librte_ethdev/meson.build    |  6 ++-
>   lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
>   lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
>   lib/librte_ethdev/rte_ethdev.h   |  7 ++++
>   lib/librte_ethdev/version.map    |  5 +++
>   9 files changed, 205 insertions(+), 4 deletions(-)
>   create mode 100644 lib/librte_ethdev/rte_apistats.c
>   create mode 100644 lib/librte_ethdev/rte_apistats.h


Hi Hideyuki,

I have a few questions on the patch set.

How does this compare to the mechanism added to l3fwd-power which counts 
the number of empty, partial and full polls, and uses them to calculate 
busyness? We saw pretty good tracking of busyness using those metrics. I 
would be concerned that just looking at the numebr of rx_bursts and 
tx_bursts may be limited to only a few use-cases. The l3fwd-power 
example uses branchless increments to capture empty, non-empty, full, 
and non-full polls.

Why not use the existing telemetry library to store the stats? It would 
be good if whatever metrics were counted were made available in a 
standard way, so that external entities such as collectd could pick them 
up, rather than having to parse the new struct. The l3fwd-power example 
registers the necessary new metrics, and exposes them through the 
telemetry library.

And a comment on the patch set in general: The order of the patch set 
seems reversed. The earlier patch do not compile, because they depend on 
rte_apistats.h, which is introduced in the final patch. Each patch as it 
is applied needs to build successfully.

Also, I would suggest a different name. rte_apistats seems very generic 
and could apply to anything. How about something like rte_ethdev_stats.h 
or rte_burst_stats.h?

Rgds,
Dave.

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Varghese, Vipin]

snipped
> +
> +int rte_apistats_init(void)
> +{
> +	int i;
> +	const struct rte_memzone *mz = NULL;
> +	const unsigned int flags = 0;
> +
> +	/** Allocate stats in shared memory fo multi process support */
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		mz = rte_memzone_lookup(MZ_APISTATS);
> +		if (mz == NULL) {
> +			RTE_LOG(ERR, APISTATS, "Cannot get info
> structure\n");
Could be more readable if the LOG is modified `failed to lookup memory for %s by Secondary!, MZ_APISTATS `

> +			return -1;
> +		}
> +		rte_apicounts = mz->addr;
> +	} else {
> +		/* RTE_PROC_PRIMARY */
> +		mz = rte_memzone_reserve(MZ_APISTATS,
> sizeof(*rte_apicounts),
Would rte_memzone_reserve_aligned be better use if you are creating per instance of lcore data.

> +			rte_socket_id(), flags);
> +		if (mz == NULL) {
> +			RTE_LOG(ERR, APISTATS, "Cannot reserve memory
> zone\n");
> +			return -ENOMEM;
Could be more readable if the LOG is modified `failed to allocate memory for %s in Primary!, MZ_APISTATS `

> +		}
> +		rte_apicounts = mz->addr;
> +		memset(rte_apicounts, 0, sizeof(*rte_apicounts));
> +	}
> +
> +	/* set up array for data */
> +	RTE_LCORE_FOREACH(i) {
Suggestion, since there would be lcore from different NUMA, Memzone_reserve from current socketid will not be the best approach. Requesting for re-look if per socketed stats for lcore is to be maintained.

> +		rte_apicounts->lcoreid_list[i] = 1;
> +		RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n",
> i);
> +	}
> +	return 0;
> +}
> +
> +int rte_apistats_uninit(void)
> +{
> +	const struct rte_memzone *mz = NULL;
> +	/* free up the memzone */
> +	mz = rte_memzone_lookup(MZ_APISTATS);
> +	if (mz)
> +		rte_memzone_free(mz);
Highly recommend to split the behavior for secondary and primary. Memory allocation is done primary and secondary only looks up. Hence it would be wise to free memory in primary only.

> +	return 0;
> +}
> diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h
> new file mode 100644
> index 0000000..afea50e
> --- /dev/null
> +++ b/lib/librte_ethdev/rte_apistats.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 NTT TechnoCross Corporation  */
> +
> +#ifndef _RTE_APISTATS_H_
> +#define _RTE_APISTATS_H_
> +
> +/**
> + * @file
> + * RTE apistats
> + *
> + * library to provide rte_rx_burst/tx_burst api stats.
> + */
> +
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_compat.h>
> +#include <rte_lcore.h>
> +
> +/**
> + * A structure for rte_rx_burst/tx_burst api statistics.
> + */
> +struct rte_apistats {
> +	int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
> +	/**< Total rte_rx_burst call counts */
> +	uint64_t rx_burst_counts[RTE_MAX_LCORE];
> +
> +	/**< Total rte_tx_burst call counts */
> +	uint64_t tx_burst_counts[RTE_MAX_LCORE]; };
Looks like the struct elements are not bifurcated based on cacheline. Requesting to avoid overlap if each core are is going to update per lcoreid.

> +
> +extern struct rte_apistats *rte_apicounts;
> +
> +/**
> + *  Initialize rte_rx_burst/tx_burst call count area.
> + *  @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + *  @return
> + *   -1     : On error
> + *   -ENOMEM: On error
> + *    0     : On success
> + */
> +__rte_experimental
> +int rte_apistats_init(void);
> +
> +/**
> + *  Clean up and free memory.
> + *  @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + *  @return
> + *   -1: On error
> + *    0: On success
> + */
> +__rte_experimental
> +int rte_apistats_uninit(void);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_APISTATS_H_ */
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index f5f8919..bef9bc6 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -160,6 +160,7 @@ extern "C" {
> 
>  #include "rte_ethdev_trace_fp.h"
>  #include "rte_dev_info.h"
> +#include <rte_apistats.h>
> 
>  extern int rte_eth_dev_logtype;
> 
> @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> queue_id,
>  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>  				     rx_pkts, nb_pkts);
> 
> +	int lcore_id = rte_lcore_id();
> +	rte_apicounts->rx_burst_counts[lcore_id]++;
> +
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>  	struct rte_eth_rxtx_callback *cb;
> 
> @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> queue_id,
>  	}
>  #endif
> 
> +	int lcore_id = rte_lcore_id();
> +	rte_apicounts->tx_burst_counts[lcore_id]++;
As per the current code, the feature is enabled by default. Should not be this an option to turn on or turn off via compiler flag? 

> +
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>  	struct rte_eth_rxtx_callback *cb;
> 
> diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> index d3f5410..adea432 100644
> --- a/lib/librte_ethdev/version.map
> +++ b/lib/librte_ethdev/version.map
> @@ -240,6 +240,11 @@ EXPERIMENTAL {
>  	rte_flow_get_restore_info;
>  	rte_flow_tunnel_action_decap_release;
>  	rte_flow_tunnel_item_release;
> +
> +        # added in 21.02
> +        rte_apistats_init;
> +        rte_apistats_uninit;
> +        rte_apicounts;
>  };
> 
>  INTERNAL {
> --
> 2.18.0

Re: [dpdk-dev] [PATCH 4/5] docs: add description of apistats parameter into proc-info

[Varghese, Vipin]

@Tahhan, Maryam and @Pattan, Reshma should not documentation and code change of proc-info be in the same patch request?

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Hideyuki Yamashita
> Sent: Friday, December 4, 2020 1:21 PM
> To: Tahhan, Maryam <maryam.tahhan@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>
> Cc: dev@dpdk.org; Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>;
> Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> Subject: [dpdk-dev] [PATCH 4/5] docs: add description of apistats parameter
> into proc-info
> 
> This patch modifies document of proc-info to introduce "--apistats"
> parameter.
> 
> Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> ---
>  doc/guides/tools/proc_info.rst | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/tools/proc_info.rst b/doc/guides/tools/proc_info.rst
> index 9772d97..4c7b79e 100644
> --- a/doc/guides/tools/proc_info.rst
> +++ b/doc/guides/tools/proc_info.rst
> @@ -18,8 +18,9 @@ The application has a number of command line options:
>  .. code-block:: console
> 
>     ./<build_dir>/app/dpdk-procinfo -- -m | [-p PORTMASK] [--stats | --xstats |
> -   --stats-reset | --xstats-reset] [ --show-port | --show-tm | --show-crypto |
> -   --show-ring[=name] | --show-mempool[=name] | --iter-mempool=name ]
> +   --stats-reset | --xstats-reset | --apistats ] [ --show-port | --show-tm |
> +   --show-crypto | --show-ring[=name] | --show-mempool[=name] |
> +   --iter-mempool=name ]
> 
>  Parameters
>  ~~~~~~~~~~
> @@ -41,6 +42,11 @@ no port mask is specified, the generic stats are reset for
> all DPDK ports.
>  The xstats-reset parameter controls the resetting of extended port statistics.
>  If no port mask is specified xstats are reset for all DPDK ports.
> 
> +**--apistats**
> +The apistats parameter controls rx_burst/tx_burst API invocation
> +counter statistics per core. If no port mask is specified apistats are
> +printed for all DPDK ports.
> +
>  **-m**: Print DPDK memory information.
> 
>  **--show-port**
> --
> 2.18.0

Re: [dpdk-dev] [PATCH 3/5] app/test-pmd: add to use apistats

[Varghese, Vipin]

snipped
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -60,6 +60,8 @@
>  #ifdef RTE_LIB_LATENCYSTATS
>  #include <rte_latencystats.h>
>  #endif
> +#include <rte_apistats.h>
> +
> 
>  #include "testpmd.h"
> 
> @@ -3958,6 +3960,8 @@ main(int argc, char** argv)
>  	}
>  #endif
> 
> +	rte_apistats_init();
Suggestion, should there be am user argument or option to enable or disable the feature?

> +
>  	/* Setup bitrate stats */
>  #ifdef RTE_LIB_BITRATESTATS
>  	if (bitrate_enabled != 0) {
> --
> 2.18.0

Re: [dpdk-dev] [PATCH 2/5] app/proc-info: add to use apistats

[Varghese, Vipin]

snipped
>  		"  --xstats-ids IDLIST: to display xstat values by id. "
>  			"The argument is comma-separated list of xstat ids to
> print out.\n"
> +		"  --apistats: to display api statistics, disabled by default\n"
As per the code base the logic ` rte_apicounts->rx_burst_counts[lcore_id]++; and rte_apicounts->tx_burst_counts[lcore_id]++;`. This expect `apistats_init` isn primary before `proc-info`. So I have 2 quereies

1. with this logic does all dpdk-examples will be updated to invoke `apistats_init`?
2. what is mechanism in place to inform all dpdk user that `just like rte_eal_init one has to explicitly invoke apistats_init too`?

since I am not clear with the code flow, please feel free to explain the logic or expectation.

snipped
> 
> +static void
> +display_apistats(void)
> +{
> +	static const char *api_stats_border = "########################";
> +	uint16_t i;
> +	struct rte_apistats t0_count, t1_count;
> +	memcpy(&t0_count, rte_apicounts, sizeof(struct rte_apistats));
Any specific reason not to use `rte_memcpy` and invoke system library call `memcpy`

> +	sleep(1);
> +	memcpy(&t1_count, rte_apicounts, sizeof(struct rte_apistats));
> +	for (i = 0; i < RTE_MAX_LCORE; i++) {
I am against the idea of iterating RTE_MAX_LCORE, as for any valid DPDK primary application the actual core count may vary between `1 to RTE_MAX_LCORE`

> +		if (t1_count.lcoreid_list[i] != 0) {
> +			uint64_t rx_count_delta = t1_count.rx_burst_counts[i]
> +				- t0_count.rx_burst_counts[i];
> +			uint64_t tx_count_delta = t1_count.tx_burst_counts[i]
> +				- t0_count.tx_burst_counts[i];
> +			printf("\n  %s api statistics for lcoreid: %d %s\n",
> +				api_stats_border, i, api_stats_border);
Suggestion: Since most of DPDK application uses dedicated core to RX and TX, would not it easier only display the lcores which does the operation. That is skip lcores where `count_delta` are 0?

> +			printf("  rx_burst_count: %13"PRIu64""
> +				" rx_burst_count_delta: %13"PRIu64"\n",
> +				t1_count.rx_burst_counts[i],
> +				rx_count_delta);
> +			printf("  tx_burst_count: %13"PRIu64""
> +				" tx_burst_count_delta: %13"PRIu64"\n",
> +				t1_count.tx_burst_counts[i],
> +				tx_count_delta);
> +			printf("  tx/rx_rate: %3.2f\n",
> +				(rx_count_delta) ? (double)tx_count_delta
> +				/ rx_count_delta : 0.0);

snipped

>  	if (enable_iter_mempool)
>  		iter_mempool(mempool_iter_name);
> +	if (enable_apistats)
> +		display_apistats();
> 
>  	RTE_ETH_FOREACH_DEV(i)
>  		rte_eth_dev_close(i);
> --
> 2.18.0

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Varghese, Vipin]

Sharing an alternate approach, if RX-TX callbacks are enabled in DPDK (which is by default). One can register a callback handler to update counters with the following information as `port-queue pair, lcoreid, total rx burst request, total empty rx burst, 1-8 pks, 9-16 pkts, 16-32 pkts`. Callback handlers can be selectively enabled or disabled too.

Can you please help me understand how `rte_apistats` would be different or pros of having it as library (that needs to be added to linking and running in case of using DPDK applications)?

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of David Hunt
> Sent: Friday, December 4, 2020 3:51 PM
> To: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/5] add apistats function
> 
> 
> On 4/12/2020 7:51 AM, Hideyuki Yamashita wrote:
> > In general, DPDK application consumes CPU usage because it polls
> > incoming packets using rx_burst API in infinite loop.
> > This makes difficult to estimate how much CPU usage is really used to
> > send/receive packets by the DPDK application.
> >
> > For example, even if no incoming packets arriving, CPU usage looks
> > nearly 100% when observed by top command.
> >
> > It is beneficial if developers can observe real CPU usage of the DPDK
> > application.
> > Such information can be exported to monitoring application like
> > prometheus/graphana and shows CPU usage graphically.
> >
> > To achieve above, this patch set provides apistats functionality.
> > apistats provides the followiing two counters for each lcore.
> > - rx_burst_counts[RTE_MAX_LCORE]
> > - tx_burst_counts[RTE_MAX_LCORE]
> > Those accumulates rx_burst/tx_burst counts since the application starts.
> >
> > By using those values, developers can roughly estimate CPU usage.
> > Let us assume a DPDK application is simply forwarding packets.
> > It calls tx_burst only if it receive packets.
> > If rx_burst_counts=1000 and tx_burst_count=1000 during certain period
> > of time, one can assume CPU usage is 100%.
> > If rx_burst_counts=1000 and tx_burst_count=100 during certain period
> > of time, one can assume CPU usage is 10%.
> > Here we assumes that tx_burst_count equals counts which rx_burst
> > function really receives incoming packets.
> >
> >
> > This patch set provides the following.
> > - basic API counting functionality(apistats) into librte_ethdev
> > - add code to testpmd to accumulate counter information
> > - add code to proc-info to retrieve above mentioned counter
> > information
> > - add description in proc-info document about --apistats parameter
> > - modify MAINTAINERS file for apistats.c and apistats.h
> >
> > Hideyuki Yamashita (5):
> >    maintainers: update maintainers file for apistats
> >    app/proc-info: add to use apistats
> >    app/test-pmd: add to use apistats
> >    docs: add description of apistats parameter into proc-info
> >    librte_ethdev: add to use apistats
> >
> >   MAINTAINERS                      |  3 ++
> >   app/proc-info/main.c             | 46 +++++++++++++++++++++++
> >   app/test-pmd/testpmd.c           |  4 ++
> >   doc/guides/tools/proc_info.rst   | 10 ++++-
> >   lib/librte_ethdev/meson.build    |  6 ++-
> >   lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/rte_ethdev.h   |  7 ++++
> >   lib/librte_ethdev/version.map    |  5 +++
> >   9 files changed, 205 insertions(+), 4 deletions(-)
> >   create mode 100644 lib/librte_ethdev/rte_apistats.c
> >   create mode 100644 lib/librte_ethdev/rte_apistats.h
> 
> 
> Hi Hideyuki,
> 
> I have a few questions on the patch set.
> 
> How does this compare to the mechanism added to l3fwd-power which counts
> the number of empty, partial and full polls, and uses them to calculate
> busyness? We saw pretty good tracking of busyness using those metrics. I
> would be concerned that just looking at the numebr of rx_bursts and tx_bursts
> may be limited to only a few use-cases. The l3fwd-power example uses
> branchless increments to capture empty, non-empty, full, and non-full polls.
> 
> Why not use the existing telemetry library to store the stats? It would be good
> if whatever metrics were counted were made available in a standard way, so
> that external entities such as collectd could pick them up, rather than having to
> parse the new struct. The l3fwd-power example registers the necessary new
> metrics, and exposes them through the telemetry library.
> 
> And a comment on the patch set in general: The order of the patch set seems
> reversed. The earlier patch do not compile, because they depend on
> rte_apistats.h, which is introduced in the final patch. Each patch as it is applied
> needs to build successfully.
> 
> Also, I would suggest a different name. rte_apistats seems very generic and
> could apply to anything. How about something like rte_ethdev_stats.h or
> rte_burst_stats.h?
> 
> Rgds,
> Dave.
> 
> 
> 
> 
> 
> 

Re: [dpdk-dev] [PATCH 1/5] maintainers: update maintainers file for apistats

[Varghese, Vipin]

Look like I am facing `mail delivery` issues to `ntt-tx.co.jp`.

```
The following message to <yamashtia.hideyuki@ntt-tx.co.jp> was undeliverable.
The reason for the problem:
5.1.0 - Unknown address error 550-'5.1.1 <yamashtia.hideyuki@ntt-tx.co.jp>: Recipient address rejected: User unknown in relay recipient table'
```

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Hideyuki Yamashita
> Sent: Friday, December 4, 2020 1:21 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>;
> Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> Subject: [dpdk-dev] [PATCH 1/5] maintainers: update maintainers file for
> apistats
> 
> This patch adds maintainer of rte_apistats.c and rte_apistats.h.
> 
> Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> ---
>  MAINTAINERS | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eafe9f8..dba2acf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -489,6 +489,9 @@ F: drivers/raw/skeleton/
>  F: app/test/test_rawdev.c
>  F: doc/guides/prog_guide/rawdev.rst
> 
> +API stats API -EXPERIMENTAL
> +M: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>
> +F: lib/librte_ethdev/rte_apistats.*
> 
>  Memory Pool Drivers
>  -------------------
> --
> 2.18.0

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Stephen Hemminger]

On Fri, 04 Dec 2020 16:51:09 +0900
Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp> wrote:

> +
> +/* Macros for printing using RTE_LOG */
> +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
> +

Please don't use static logtypes.
Better to allocate a dynamic logtype value and use that.

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Thomas Monjalon]

04/12/2020 08:51, Hideyuki Yamashita:
> In general, DPDK application consumes CPU usage because it polls
> incoming packets using rx_burst API in infinite loop.
> This makes difficult to estimate how much CPU usage is really
> used to send/receive packets by the DPDK application.
> 
> For example, even if no incoming packets arriving, CPU usage
> looks nearly 100% when observed by top command.
> 
> It is beneficial if developers can observe real CPU usage of the
> DPDK application.
> Such information can be exported to monitoring application like
> prometheus/graphana and shows CPU usage graphically.
> 
> To achieve above, this patch set provides apistats functionality.
> apistats provides the followiing two counters for each lcore.
> - rx_burst_counts[RTE_MAX_LCORE]
> - tx_burst_counts[RTE_MAX_LCORE]
> Those accumulates rx_burst/tx_burst counts since the application starts.

Please could you compare with what rte_jobstats offers?
http://code.dpdk.org/dpdk/latest/source/lib/librte_jobstats/rte_jobstats.h

I feel all of this shares the same goals as librte_power work.

[...]
> - basic API counting functionality(apistats) into librte_ethdev

Could it be it be accessible via rte_telemetry?
http://code.dpdk.org/dpdk/latest/source/lib/librte_telemetry/rte_telemetry.h

Re: [dpdk-dev] [PATCH 4/5] docs: add description of apistats parameter into proc-info

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin <vipin.varghese@intel.com>
> @Tahhan, Maryam and @Pattan, Reshma should not documentation and code
> change of proc-info be in the same patch request?

If code changes and doc changes are related, yes they should be combined in one patch. That is what the case here I guess.

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Ananyev, Konstantin]

Hi,

> 
> This patch modifies to use apistats by librte_ethdev.
> 
> Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> ---
>  lib/librte_ethdev/meson.build    |  6 ++-
>  lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h   |  7 ++++
>  lib/librte_ethdev/version.map    |  5 +++
>  5 files changed, 144 insertions(+), 2 deletions(-)
>  create mode 100644 lib/librte_ethdev/rte_apistats.c
>  create mode 100644 lib/librte_ethdev/rte_apistats.h
> 
> diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
> index e4b6102..d03e784 100644
> --- a/lib/librte_ethdev/meson.build
> +++ b/lib/librte_ethdev/meson.build
> @@ -8,7 +8,8 @@ sources = files('ethdev_private.c',
>  	'rte_ethdev.c',
>  	'rte_flow.c',
>  	'rte_mtr.c',
> -	'rte_tm.c')
> +	'rte_tm.c' ,
> +        'rte_apistats.c')
> 
>  headers = files('rte_ethdev.h',
>  	'rte_ethdev_driver.h',
> @@ -24,6 +25,7 @@ headers = files('rte_ethdev.h',
>  	'rte_mtr.h',
>  	'rte_mtr_driver.h',
>  	'rte_tm.h',
> -	'rte_tm_driver.h')
> +	'rte_tm_driver.h',
> +        'rte_apistats.h')
> 
>  deps += ['net', 'kvargs', 'meter', 'telemetry']
> diff --git a/lib/librte_ethdev/rte_apistats.c b/lib/librte_ethdev/rte_apistats.c
> new file mode 100644
> index 0000000..c4bde34
> --- /dev/null
> +++ b/lib/librte_ethdev/rte_apistats.c
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 NTT TechnoCross Corporation
> + */
> +
> +
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <string.h>
> +#include <rte_log.h>
> +#include <rte_memzone.h>
> +#include <rte_lcore.h>
> +
> +#include "rte_apistats.h"
> +
> +/* Macros for printing using RTE_LOG */
> +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
> +
> +#define MZ_APISTATS "rte_apistats"
> +
> +struct rte_apistats *rte_apicounts;
> +
> +int rte_apistats_init(void)
> +{
> +	int i;
> +	const struct rte_memzone *mz = NULL;
> +	const unsigned int flags = 0;
> +
> +	/** Allocate stats in shared memory fo multi process support */
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		mz = rte_memzone_lookup(MZ_APISTATS);
> +		if (mz == NULL) {
> +			RTE_LOG(ERR, APISTATS, "Cannot get info structure\n");
> +			return -1;
> +		}
> +		rte_apicounts = mz->addr;
> +	} else {
> +		/* RTE_PROC_PRIMARY */
> +		mz = rte_memzone_reserve(MZ_APISTATS, sizeof(*rte_apicounts),
> +			rte_socket_id(), flags);
> +		if (mz == NULL) {
> +			RTE_LOG(ERR, APISTATS, "Cannot reserve memory zone\n");
> +			return -ENOMEM;
> +		}
> +		rte_apicounts = mz->addr;
> +		memset(rte_apicounts, 0, sizeof(*rte_apicounts));
> +	}
> +
> +	/* set up array for data */
> +	RTE_LCORE_FOREACH(i) {
> +		rte_apicounts->lcoreid_list[i] = 1;
> +		RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n", i);
> +	}
> +	return 0;
> +}
> +
> +int rte_apistats_uninit(void)
> +{
> +	const struct rte_memzone *mz = NULL;
> +	/* free up the memzone */
> +	mz = rte_memzone_lookup(MZ_APISTATS);
> +	if (mz)
> +		rte_memzone_free(mz);
> +	return 0;
> +}
> diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h
> new file mode 100644
> index 0000000..afea50e
> --- /dev/null
> +++ b/lib/librte_ethdev/rte_apistats.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 NTT TechnoCross Corporation
> + */
> +
> +#ifndef _RTE_APISTATS_H_
> +#define _RTE_APISTATS_H_
> +
> +/**
> + * @file
> + * RTE apistats
> + *
> + * library to provide rte_rx_burst/tx_burst api stats.
> + */
> +
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_compat.h>
> +#include <rte_lcore.h>
> +
> +/**
> + * A structure for rte_rx_burst/tx_burst api statistics.
> + */
> +struct rte_apistats {
> +	int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
> +	/**< Total rte_rx_burst call counts */
> +	uint64_t rx_burst_counts[RTE_MAX_LCORE];
> +
> +	/**< Total rte_tx_burst call counts */
> +	uint64_t tx_burst_counts[RTE_MAX_LCORE];
> +};
> +
> +extern struct rte_apistats *rte_apicounts;
> +
> +/**
> + *  Initialize rte_rx_burst/tx_burst call count area.
> + *  @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + *  @return
> + *   -1     : On error
> + *   -ENOMEM: On error
> + *    0     : On success
> + */
> +__rte_experimental
> +int rte_apistats_init(void);
> +
> +/**
> + *  Clean up and free memory.
> + *  @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + *  @return
> + *   -1: On error
> + *    0: On success
> + */
> +__rte_experimental
> +int rte_apistats_uninit(void);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_APISTATS_H_ */
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index f5f8919..bef9bc6 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -160,6 +160,7 @@ extern "C" {
> 
>  #include "rte_ethdev_trace_fp.h"
>  #include "rte_dev_info.h"
> +#include <rte_apistats.h>
> 
>  extern int rte_eth_dev_logtype;
> 
> @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>  				     rx_pkts, nb_pkts);
> 
> +	int lcore_id = rte_lcore_id();
> +	rte_apicounts->rx_burst_counts[lcore_id]++;

There are few problems with current implementation:
1.  rx_burst() can be called by non-EAL thread (rte_lcore_id() == -1)
    In that case it would cause a crash.
2. Because of the layout of struct rte_apistats it would cause significant
    performance degradation (false cache-lines sharing).

As a generic one: such sort of statistics can be easily collected by the app itself.
Either by just incrementing counters before rx/tx_burst function call directly or
via rx/tx callbacks.
So I don't see any point to push it inside ethdev layer.  
Konstantin

>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>  	struct rte_eth_rxtx_callback *cb;
> 
> @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>  	}
>  #endif
> 
> +	int lcore_id = rte_lcore_id();
> +	rte_apicounts->tx_burst_counts[lcore_id]++;
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>  	struct rte_eth_rxtx_callback *cb;
> 
> diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> index d3f5410..adea432 100644
> --- a/lib/librte_ethdev/version.map
> +++ b/lib/librte_ethdev/version.map
> @@ -240,6 +240,11 @@ EXPERIMENTAL {
>  	rte_flow_get_restore_info;
>  	rte_flow_tunnel_action_decap_release;
>  	rte_flow_tunnel_item_release;
> +
> +        # added in 21.02
> +        rte_apistats_init;
> +        rte_apistats_uninit;
> +        rte_apicounts;
>  };
> 
>  INTERNAL {
> --
> 2.18.0

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Hideyuki Yamashita]

Hi, David

Thanks for your comments.
Please see my comments inline tagged with [HY].

> 
> On 4/12/2020 7:51 AM, Hideyuki Yamashita wrote:
> > In general, DPDK application consumes CPU usage because it polls
> > incoming packets using rx_burst API in infinite loop.
> > This makes difficult to estimate how much CPU usage is really
> > used to send/receive packets by the DPDK application.
> >
> > For example, even if no incoming packets arriving, CPU usage
> > looks nearly 100% when observed by top command.
> >
> > It is beneficial if developers can observe real CPU usage of the
> > DPDK application.
> > Such information can be exported to monitoring application like
> > prometheus/graphana and shows CPU usage graphically.
> >
> > To achieve above, this patch set provides apistats functionality.
> > apistats provides the followiing two counters for each lcore.
> > - rx_burst_counts[RTE_MAX_LCORE]
> > - tx_burst_counts[RTE_MAX_LCORE]
> > Those accumulates rx_burst/tx_burst counts since the application starts.
> >
> > By using those values, developers can roughly estimate CPU usage.
> > Let us assume a DPDK application is simply forwarding packets.
> > It calls tx_burst only if it receive packets.
> > If rx_burst_counts=1000 and tx_burst_count=1000 during certain
> > period of time, one can assume CPU usage is 100%.
> > If rx_burst_counts=1000 and tx_burst_count=100 during certain
> > period of time, one can assume CPU usage is 10%.
> > Here we assumes that tx_burst_count equals counts which rx_burst function
> > really receives incoming packets.
> >
> >
> > This patch set provides the following.
> > - basic API counting functionality(apistats) into librte_ethdev
> > - add code to testpmd to accumulate counter information
> > - add code to proc-info to retrieve above mentioned counter information
> > - add description in proc-info document about --apistats parameter
> > - modify MAINTAINERS file for apistats.c and apistats.h
> >
> > Hideyuki Yamashita (5):
> >    maintainers: update maintainers file for apistats
> >    app/proc-info: add to use apistats
> >    app/test-pmd: add to use apistats
> >    docs: add description of apistats parameter into proc-info
> >    librte_ethdev: add to use apistats
> >
> >   MAINTAINERS                      |  3 ++
> >   app/proc-info/main.c             | 46 +++++++++++++++++++++++
> >   app/test-pmd/testpmd.c           |  4 ++
> >   doc/guides/tools/proc_info.rst   | 10 ++++-
> >   lib/librte_ethdev/meson.build    |  6 ++-
> >   lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/rte_ethdev.h   |  7 ++++
> >   lib/librte_ethdev/version.map    |  5 +++
> >   9 files changed, 205 insertions(+), 4 deletions(-)
> >   create mode 100644 lib/librte_ethdev/rte_apistats.c
> >   create mode 100644 lib/librte_ethdev/rte_apistats.h
> 
> 
> Hi Hideyuki,
> 
> I have a few questions on the patch set.
> 
> How does this compare to the mechanism added to l3fwd-power which counts the number of empty, partial and full polls, and uses them to calculate busyness? We saw pretty good tracking of busyness using those metrics. I would be concerned that just looking at the numebr of rx_bursts and tx_bursts may be limited to only a few use-cases. The l3fwd-power example uses branchless increments to capture empty, non-empty, full, and non-full polls.
[HY]
Thanks for your commetns.
You are correct. As you well know, l3fwd-power measures "how cpu cores
are busy".
And in that sense, the goal of my proposal is the same with yours .
Moreover l3fwd-power is more precise than my proposal.

Point of my proposal is 
- more easy to use
- less code impact on application code

I think that if application developer wants to need to measure "how cpu
cores are busy" he/she will needs to implement
- logic similar with l3fwd-power
or
- use jobstats API

But it is rather heavy for existing applications.
By using my proposal, it is "much easier" to implement.
(But it is "rough" measurement. I think it is trade-off)

> Why not use the existing telemetry library to store the stats? It would be good if whatever metrics were counted were made available in a standard way, so that external entities such as collectd could pick them up, rather than having to parse the new struct. The l3fwd-power example registers the necessary new metrics, and exposes them through the telemetry library.

[HY]
OK.
Currently, no reason not using telemetry.

I think telemetry is useful for applications which does NOT call DPDK
API(C lang API) directly.
My patchset provide only C API to retrieve apistats.
But if assuming not all applications call C API, then I think it is
reasonable to add telemetry in addition to C API for exposing stats.

> And a comment on the patch set in general: The order of the patch set seems reversed. The earlier patch do not compile, because they depend on rte_apistats.h, which is introduced in the final patch. Each patch as it is applied needs to build successfully.
[HY]
Thanks for your information.
OK. Now I understand your point that the order of the patch is very
important. Thanks.

> Also, I would suggest a different name. rte_apistats seems very generic and could apply to anything. How about something like rte_ethdev_stats.h or rte_burst_stats.h?
[HY]
Thanks. I agree.
"txrx_apicall_stats" maybe?

Thanks!
BR,
Hideyuki Yamashita
NTT TechnoCross
 
> Rgds,
> Dave.
> 
> 
> 
> 
> 
> 

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Hideyuki Yamashita]

Hello,

Thanks for your comments.
Please see my comments inline tagged with [HY].

> 04/12/2020 08:51, Hideyuki Yamashita:
> > In general, DPDK application consumes CPU usage because it polls
> > incoming packets using rx_burst API in infinite loop.
> > This makes difficult to estimate how much CPU usage is really
> > used to send/receive packets by the DPDK application.
> > 
> > For example, even if no incoming packets arriving, CPU usage
> > looks nearly 100% when observed by top command.
> > 
> > It is beneficial if developers can observe real CPU usage of the
> > DPDK application.
> > Such information can be exported to monitoring application like
> > prometheus/graphana and shows CPU usage graphically.
> > 
> > To achieve above, this patch set provides apistats functionality.
> > apistats provides the followiing two counters for each lcore.
> > - rx_burst_counts[RTE_MAX_LCORE]
> > - tx_burst_counts[RTE_MAX_LCORE]
> > Those accumulates rx_burst/tx_burst counts since the application starts.
> 
> Please could you compare with what rte_jobstats offers?
> http://code.dpdk.org/dpdk/latest/source/lib/librte_jobstats/rte_jobstats.h
> 
> I feel all of this shares the same goals as librte_power work.

[HY]
Thanks for your commetns.
You are correct. As you well know, l3fwd-power measures "how cpu cores
are busy".
And in that sense, the goal of my proposal is the same with yours .
Moreover l3fwd-power is more precise than my proposal.

Point of my proposal is 
- more easy to use
- less code impact on application code

I think that if application developer wants to need to measure "how cpu
cores are busy" he/she will needs to implement
- logic similar with l3fwd-power
or
- use jobstats API

But it is rather heavy for existing applications.
By using my proposal, it is "much easier" to implement.
(But it is "rough" measurement. I think it is trade-off)
 
How do you think about the idea?

> [...]
> > - basic API counting functionality(apistats) into librte_ethdev
> 
> Could it be it be accessible via rte_telemetry?
> http://code.dpdk.org/dpdk/latest/source/lib/librte_telemetry/rte_telemetry.h
> 
[HY]
OK.
Currently, no reason not using telemetry.

I think telemetry is useful for applications which does NOT call DPDK
API(C lang API) directly.
My patchset provide only C API to retrieve apistats.
But if assuming not all applications call C API, then I think it is
reasonable to add telemetry in addition to C API for exposing stats.

Do you think "exposure via C API" is not needed?

Thanks!
BR,
Hideyuki Yamashita
NTT TechnoCross

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Hideyuki Yamashita]

Hello,

Thanks for your comments.
Please see my comments inline tagged with [HY].

> Hi,
> 
> > 
> > This patch modifies to use apistats by librte_ethdev.
> > 
> > Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> > ---
> >  lib/librte_ethdev/meson.build    |  6 ++-
> >  lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev.h   |  7 ++++
> >  lib/librte_ethdev/version.map    |  5 +++
> >  5 files changed, 144 insertions(+), 2 deletions(-)
> >  create mode 100644 lib/librte_ethdev/rte_apistats.c
> >  create mode 100644 lib/librte_ethdev/rte_apistats.h
> > 
> > diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
> > index e4b6102..d03e784 100644
> > --- a/lib/librte_ethdev/meson.build
> > +++ b/lib/librte_ethdev/meson.build
> > @@ -8,7 +8,8 @@ sources = files('ethdev_private.c',
> >  	'rte_ethdev.c',
> >  	'rte_flow.c',
> >  	'rte_mtr.c',
> > -	'rte_tm.c')
> > +	'rte_tm.c' ,
> > +        'rte_apistats.c')
> > 
> >  headers = files('rte_ethdev.h',
> >  	'rte_ethdev_driver.h',
> > @@ -24,6 +25,7 @@ headers = files('rte_ethdev.h',
> >  	'rte_mtr.h',
> >  	'rte_mtr_driver.h',
> >  	'rte_tm.h',
> > -	'rte_tm_driver.h')
> > +	'rte_tm_driver.h',
> > +        'rte_apistats.h')
> > 
> >  deps += ['net', 'kvargs', 'meter', 'telemetry']
> > diff --git a/lib/librte_ethdev/rte_apistats.c b/lib/librte_ethdev/rte_apistats.c
> > new file mode 100644
> > index 0000000..c4bde34
> > --- /dev/null
> > +++ b/lib/librte_ethdev/rte_apistats.c
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2019 NTT TechnoCross Corporation
> > + */
> > +
> > +
> > +#include <unistd.h>
> > +#include <sys/types.h>
> > +#include <string.h>
> > +#include <rte_log.h>
> > +#include <rte_memzone.h>
> > +#include <rte_lcore.h>
> > +
> > +#include "rte_apistats.h"
> > +
> > +/* Macros for printing using RTE_LOG */
> > +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
> > +
> > +#define MZ_APISTATS "rte_apistats"
> > +
> > +struct rte_apistats *rte_apicounts;
> > +
> > +int rte_apistats_init(void)
> > +{
> > +	int i;
> > +	const struct rte_memzone *mz = NULL;
> > +	const unsigned int flags = 0;
> > +
> > +	/** Allocate stats in shared memory fo multi process support */
> > +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > +		mz = rte_memzone_lookup(MZ_APISTATS);
> > +		if (mz == NULL) {
> > +			RTE_LOG(ERR, APISTATS, "Cannot get info structure\n");
> > +			return -1;
> > +		}
> > +		rte_apicounts = mz->addr;
> > +	} else {
> > +		/* RTE_PROC_PRIMARY */
> > +		mz = rte_memzone_reserve(MZ_APISTATS, sizeof(*rte_apicounts),
> > +			rte_socket_id(), flags);
> > +		if (mz == NULL) {
> > +			RTE_LOG(ERR, APISTATS, "Cannot reserve memory zone\n");
> > +			return -ENOMEM;
> > +		}
> > +		rte_apicounts = mz->addr;
> > +		memset(rte_apicounts, 0, sizeof(*rte_apicounts));
> > +	}
> > +
> > +	/* set up array for data */
> > +	RTE_LCORE_FOREACH(i) {
> > +		rte_apicounts->lcoreid_list[i] = 1;
> > +		RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n", i);
> > +	}
> > +	return 0;
> > +}
> > +
> > +int rte_apistats_uninit(void)
> > +{
> > +	const struct rte_memzone *mz = NULL;
> > +	/* free up the memzone */
> > +	mz = rte_memzone_lookup(MZ_APISTATS);
> > +	if (mz)
> > +		rte_memzone_free(mz);
> > +	return 0;
> > +}
> > diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h
> > new file mode 100644
> > index 0000000..afea50e
> > --- /dev/null
> > +++ b/lib/librte_ethdev/rte_apistats.h
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2019 NTT TechnoCross Corporation
> > + */
> > +
> > +#ifndef _RTE_APISTATS_H_
> > +#define _RTE_APISTATS_H_
> > +
> > +/**
> > + * @file
> > + * RTE apistats
> > + *
> > + * library to provide rte_rx_burst/tx_burst api stats.
> > + */
> > +
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <rte_compat.h>
> > +#include <rte_lcore.h>
> > +
> > +/**
> > + * A structure for rte_rx_burst/tx_burst api statistics.
> > + */
> > +struct rte_apistats {
> > +	int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
> > +	/**< Total rte_rx_burst call counts */
> > +	uint64_t rx_burst_counts[RTE_MAX_LCORE];
> > +
> > +	/**< Total rte_tx_burst call counts */
> > +	uint64_t tx_burst_counts[RTE_MAX_LCORE];
> > +};
> > +
> > +extern struct rte_apistats *rte_apicounts;
> > +
> > +/**
> > + *  Initialize rte_rx_burst/tx_burst call count area.
> > + *  @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + *  @return
> > + *   -1     : On error
> > + *   -ENOMEM: On error
> > + *    0     : On success
> > + */
> > +__rte_experimental
> > +int rte_apistats_init(void);
> > +
> > +/**
> > + *  Clean up and free memory.
> > + *  @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + *  @return
> > + *   -1: On error
> > + *    0: On success
> > + */
> > +__rte_experimental
> > +int rte_apistats_uninit(void);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_APISTATS_H_ */
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index f5f8919..bef9bc6 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -160,6 +160,7 @@ extern "C" {
> > 
> >  #include "rte_ethdev_trace_fp.h"
> >  #include "rte_dev_info.h"
> > +#include <rte_apistats.h>
> > 
> >  extern int rte_eth_dev_logtype;
> > 
> > @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> >  				     rx_pkts, nb_pkts);
> > 
> > +	int lcore_id = rte_lcore_id();
> > +	rte_apicounts->rx_burst_counts[lcore_id]++;
> 
> There are few problems with current implementation:
> 1.  rx_burst() can be called by non-EAL thread (rte_lcore_id() == -1)
>     In that case it would cause a crash.
[HY]
Thanks for your info.
I think by adding code like following.

     if(rte_lcore_id() = -1){
		return
      }


> 2. Because of the layout of struct rte_apistats it would cause significant
>     performance degradation (false cache-lines sharing).
[HY]
I think you are correct.
This affects change in core 1 to all other cores
even thogh change in core 1 does NOT affect other cores.

Root cause is using array like [RTE_MAX_LCORE], correct?
I will change it in revised patcheset.

+struct rte_apistats {
+       int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
+       /**< Total rte_rx_burst call counts */
+       uint64_t rx_burst_counts[RTE_MAX_LCORE];
+
+       /**< Total rte_tx_burst call counts */
+       uint64_t tx_burst_counts[RTE_MAX_LCORE];
+};


> As a generic one: such sort of statistics can be easily collected by the app itself.
> Either by just incrementing counters before rx/tx_burst function call directly or
> via rx/tx callbacks.
> So I don't see any point to push it inside ethdev layer.  
> Konstantin
[HY]
You are correct.
Application can do it.
But if applications want to do it, then every applicaiton needs 
to do it.
The reason why I propose my patchset is to provide such
common function (count invocation of rx_burst/tx_burst)
so that application only needs to "retrieve the information".

I think it is benefitical than all application  do similar thing.
Your feedback is highly appreciated.

Thanks!
BR,
Hideyuki Yamashita
NTT TechnoCross

> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> >  	struct rte_eth_rxtx_callback *cb;
> > 
> > @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
> >  	}
> >  #endif
> > 
> > +	int lcore_id = rte_lcore_id();
> > +	rte_apicounts->tx_burst_counts[lcore_id]++;
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> >  	struct rte_eth_rxtx_callback *cb;
> > 
> > diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> > index d3f5410..adea432 100644
> > --- a/lib/librte_ethdev/version.map
> > +++ b/lib/librte_ethdev/version.map
> > @@ -240,6 +240,11 @@ EXPERIMENTAL {
> >  	rte_flow_get_restore_info;
> >  	rte_flow_tunnel_action_decap_release;
> >  	rte_flow_tunnel_item_release;
> > +
> > +        # added in 21.02
> > +        rte_apistats_init;
> > +        rte_apistats_uninit;
> > +        rte_apicounts;
> >  };
> > 
> >  INTERNAL {
> > --
> > 2.18.0

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Hideyuki Yamashita]

Hello,

Thanks for your comments.
Please see my comments inline tagged with [HY].

> Hi,
> 
> > 
> > This patch modifies to use apistats by librte_ethdev.
> > 
> > Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> > ---
> >  lib/librte_ethdev/meson.build    |  6 ++-
> >  lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev.h   |  7 ++++
> >  lib/librte_ethdev/version.map    |  5 +++
> >  5 files changed, 144 insertions(+), 2 deletions(-)
> >  create mode 100644 lib/librte_ethdev/rte_apistats.c
> >  create mode 100644 lib/librte_ethdev/rte_apistats.h
> > 
> > diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
> > index e4b6102..d03e784 100644
> > --- a/lib/librte_ethdev/meson.build
> > +++ b/lib/librte_ethdev/meson.build
> > @@ -8,7 +8,8 @@ sources = files('ethdev_private.c',
> >  	'rte_ethdev.c',
> >  	'rte_flow.c',
> >  	'rte_mtr.c',
> > -	'rte_tm.c')
> > +	'rte_tm.c' ,
> > +        'rte_apistats.c')
> > 
> >  headers = files('rte_ethdev.h',
> >  	'rte_ethdev_driver.h',
> > @@ -24,6 +25,7 @@ headers = files('rte_ethdev.h',
> >  	'rte_mtr.h',
> >  	'rte_mtr_driver.h',
> >  	'rte_tm.h',
> > -	'rte_tm_driver.h')
> > +	'rte_tm_driver.h',
> > +        'rte_apistats.h')
> > 
> >  deps += ['net', 'kvargs', 'meter', 'telemetry']
> > diff --git a/lib/librte_ethdev/rte_apistats.c b/lib/librte_ethdev/rte_apistats.c
> > new file mode 100644
> > index 0000000..c4bde34
> > --- /dev/null
> > +++ b/lib/librte_ethdev/rte_apistats.c
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2019 NTT TechnoCross Corporation
> > + */
> > +
> > +
> > +#include <unistd.h>
> > +#include <sys/types.h>
> > +#include <string.h>
> > +#include <rte_log.h>
> > +#include <rte_memzone.h>
> > +#include <rte_lcore.h>
> > +
> > +#include "rte_apistats.h"
> > +
> > +/* Macros for printing using RTE_LOG */
> > +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
> > +
> > +#define MZ_APISTATS "rte_apistats"
> > +
> > +struct rte_apistats *rte_apicounts;
> > +
> > +int rte_apistats_init(void)
> > +{
> > +	int i;
> > +	const struct rte_memzone *mz = NULL;
> > +	const unsigned int flags = 0;
> > +
> > +	/** Allocate stats in shared memory fo multi process support */
> > +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > +		mz = rte_memzone_lookup(MZ_APISTATS);
> > +		if (mz == NULL) {
> > +			RTE_LOG(ERR, APISTATS, "Cannot get info structure\n");
> > +			return -1;
> > +		}
> > +		rte_apicounts = mz->addr;
> > +	} else {
> > +		/* RTE_PROC_PRIMARY */
> > +		mz = rte_memzone_reserve(MZ_APISTATS, sizeof(*rte_apicounts),
> > +			rte_socket_id(), flags);
> > +		if (mz == NULL) {
> > +			RTE_LOG(ERR, APISTATS, "Cannot reserve memory zone\n");
> > +			return -ENOMEM;
> > +		}
> > +		rte_apicounts = mz->addr;
> > +		memset(rte_apicounts, 0, sizeof(*rte_apicounts));
> > +	}
> > +
> > +	/* set up array for data */
> > +	RTE_LCORE_FOREACH(i) {
> > +		rte_apicounts->lcoreid_list[i] = 1;
> > +		RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n", i);
> > +	}
> > +	return 0;
> > +}
> > +
> > +int rte_apistats_uninit(void)
> > +{
> > +	const struct rte_memzone *mz = NULL;
> > +	/* free up the memzone */
> > +	mz = rte_memzone_lookup(MZ_APISTATS);
> > +	if (mz)
> > +		rte_memzone_free(mz);
> > +	return 0;
> > +}
> > diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h
> > new file mode 100644
> > index 0000000..afea50e
> > --- /dev/null
> > +++ b/lib/librte_ethdev/rte_apistats.h
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2019 NTT TechnoCross Corporation
> > + */
> > +
> > +#ifndef _RTE_APISTATS_H_
> > +#define _RTE_APISTATS_H_
> > +
> > +/**
> > + * @file
> > + * RTE apistats
> > + *
> > + * library to provide rte_rx_burst/tx_burst api stats.
> > + */
> > +
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <rte_compat.h>
> > +#include <rte_lcore.h>
> > +
> > +/**
> > + * A structure for rte_rx_burst/tx_burst api statistics.
> > + */
> > +struct rte_apistats {
> > +	int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
> > +	/**< Total rte_rx_burst call counts */
> > +	uint64_t rx_burst_counts[RTE_MAX_LCORE];
> > +
> > +	/**< Total rte_tx_burst call counts */
> > +	uint64_t tx_burst_counts[RTE_MAX_LCORE];
> > +};
> > +
> > +extern struct rte_apistats *rte_apicounts;
> > +
> > +/**
> > + *  Initialize rte_rx_burst/tx_burst call count area.
> > + *  @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + *  @return
> > + *   -1     : On error
> > + *   -ENOMEM: On error
> > + *    0     : On success
> > + */
> > +__rte_experimental
> > +int rte_apistats_init(void);
> > +
> > +/**
> > + *  Clean up and free memory.
> > + *  @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + *  @return
> > + *   -1: On error
> > + *    0: On success
> > + */
> > +__rte_experimental
> > +int rte_apistats_uninit(void);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_APISTATS_H_ */
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index f5f8919..bef9bc6 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -160,6 +160,7 @@ extern "C" {
> > 
> >  #include "rte_ethdev_trace_fp.h"
> >  #include "rte_dev_info.h"
> > +#include <rte_apistats.h>
> > 
> >  extern int rte_eth_dev_logtype;
> > 
> > @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> >  				     rx_pkts, nb_pkts);
> > 
> > +	int lcore_id = rte_lcore_id();
> > +	rte_apicounts->rx_burst_counts[lcore_id]++;
> 
> There are few problems with current implementation:
> 1.  rx_burst() can be called by non-EAL thread (rte_lcore_id() == -1)
>     In that case it would cause a crash.
[HY]
Thanks for your info.
I think by adding code like following.

     if(rte_lcore_id() = -1){
		return
      }


> 2. Because of the layout of struct rte_apistats it would cause significant
>     performance degradation (false cache-lines sharing).
[HY]
I think you are correct.
This affects change in core 1 to all other cores
even thogh change in core 1 does NOT affect other cores.

Root cause is using array like [RTE_MAX_LCORE], correct?
I will change it in revised patcheset.

+struct rte_apistats {
+       int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
+       /**< Total rte_rx_burst call counts */
+       uint64_t rx_burst_counts[RTE_MAX_LCORE];
+
+       /**< Total rte_tx_burst call counts */
+       uint64_t tx_burst_counts[RTE_MAX_LCORE];
+};


> As a generic one: such sort of statistics can be easily collected by the app itself.
> Either by just incrementing counters before rx/tx_burst function call directly or
> via rx/tx callbacks.
> So I don't see any point to push it inside ethdev layer.  
> Konstantin
[HY]
You are correct.
Application can do it.
But if applications want to do it, then every applicaiton needs 
to do it.
The reason why I propose my patchset is to provide such
common function (count invocation of rx_burst/tx_burst)
so that application only needs to "retrieve the information".

I think it is benefitical than all application  do similar thing.
Your feedback is highly appreciated.

Thanks!
BR,
Hideyuki Yamashita
NTT TechnoCross

> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> >  	struct rte_eth_rxtx_callback *cb;
> > 
> > @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
> >  	}
> >  #endif
> > 
> > +	int lcore_id = rte_lcore_id();
> > +	rte_apicounts->tx_burst_counts[lcore_id]++;
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> >  	struct rte_eth_rxtx_callback *cb;
> > 
> > diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> > index d3f5410..adea432 100644
> > --- a/lib/librte_ethdev/version.map
> > +++ b/lib/librte_ethdev/version.map
> > @@ -240,6 +240,11 @@ EXPERIMENTAL {
> >  	rte_flow_get_restore_info;
> >  	rte_flow_tunnel_action_decap_release;
> >  	rte_flow_tunnel_item_release;
> > +
> > +        # added in 21.02
> > +        rte_apistats_init;
> > +        rte_apistats_uninit;
> > +        rte_apicounts;
> >  };
> > 
> >  INTERNAL {
> > --
> > 2.18.0

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hideyuki Yamashita
> Sent: Tuesday, December 22, 2020 3:50 AM
> 
> Hello,
> 
> Thanks for your comments.
> Please see my comments inline tagged with [HY].
> 
> > > diff --git a/lib/librte_ethdev/rte_ethdev.h
> b/lib/librte_ethdev/rte_ethdev.h
> > > index f5f8919..bef9bc6 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -160,6 +160,7 @@ extern "C" {
> > >
> > >  #include "rte_ethdev_trace_fp.h"
> > >  #include "rte_dev_info.h"
> > > +#include <rte_apistats.h>
> > >
> > >  extern int rte_eth_dev_logtype;
> > >
> > > @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> queue_id,
> > >  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> > >  				     rx_pkts, nb_pkts);
> > >
> > > +	int lcore_id = rte_lcore_id();
> > > +	rte_apicounts->rx_burst_counts[lcore_id]++;

[...]

> > As a generic one: such sort of statistics can be easily collected by the
> app itself.
> > Either by just incrementing counters before rx/tx_burst function call
> directly or
> > via rx/tx callbacks.
> > So I don't see any point to push it inside ethdev layer.
> > Konstantin
> [HY]
> You are correct.
> Application can do it.
> But if applications want to do it, then every applicaiton needs
> to do it.
> The reason why I propose my patchset is to provide such
> common function (count invocation of rx_burst/tx_burst)
> so that application only needs to "retrieve the information".
> 
> I think it is benefitical than all application  do similar thing.
> Your feedback is highly appreciated.

For performance reasons, I am strongly opposed to adding anything into the ethdev rx/tx functions, unless it is absolutely critical for a large user base.

I get your argument about avoiding additional application code by doing it directly in the ethdev rx/tx functions - this is the benefit that this library adds to DPDK. So as a compromise, I suggest surrounding the added code in these functions by #ifdef/#endif, so there is no performance degradation if the library is not used.

Personally, I would prefer a much more advanced library for measuring CPU load and RX/TX usage. However, I can also imagine that simple DPDK applications would benefit from this library, because is easy to understand and requires nearly no effort to use.

> 
> Thanks!
> BR,
> Hideyuki Yamashita
> NTT TechnoCross
> 
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > >  	struct rte_eth_rxtx_callback *cb;
> > >
> > > @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> queue_id,
> > >  	}
> > >  #endif
> > >
> > > +	int lcore_id = rte_lcore_id();
> > > +	rte_apicounts->tx_burst_counts[lcore_id]++;
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > >  	struct rte_eth_rxtx_callback *cb;
> > >

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Ananyev, Konstantin]

> Hello,
> 
> Thanks for your comments.
> Please see my comments inline tagged with [HY].
> 
> > Hi,
> >
> > >
> > > This patch modifies to use apistats by librte_ethdev.
> > >
> > > Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> > > ---
> > >  lib/librte_ethdev/meson.build    |  6 ++-
> > >  lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
> > >  lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
> > >  lib/librte_ethdev/rte_ethdev.h   |  7 ++++
> > >  lib/librte_ethdev/version.map    |  5 +++
> > >  5 files changed, 144 insertions(+), 2 deletions(-)
> > >  create mode 100644 lib/librte_ethdev/rte_apistats.c
> > >  create mode 100644 lib/librte_ethdev/rte_apistats.h
> > >
> > > diff --git a/lib/librte_ethdev/meson.build b/lib/librte_ethdev/meson.build
> > > index e4b6102..d03e784 100644
> > > --- a/lib/librte_ethdev/meson.build
> > > +++ b/lib/librte_ethdev/meson.build
> > > @@ -8,7 +8,8 @@ sources = files('ethdev_private.c',
> > >  	'rte_ethdev.c',
> > >  	'rte_flow.c',
> > >  	'rte_mtr.c',
> > > -	'rte_tm.c')
> > > +	'rte_tm.c' ,
> > > +        'rte_apistats.c')
> > >
> > >  headers = files('rte_ethdev.h',
> > >  	'rte_ethdev_driver.h',
> > > @@ -24,6 +25,7 @@ headers = files('rte_ethdev.h',
> > >  	'rte_mtr.h',
> > >  	'rte_mtr_driver.h',
> > >  	'rte_tm.h',
> > > -	'rte_tm_driver.h')
> > > +	'rte_tm_driver.h',
> > > +        'rte_apistats.h')
> > >
> > >  deps += ['net', 'kvargs', 'meter', 'telemetry']
> > > diff --git a/lib/librte_ethdev/rte_apistats.c b/lib/librte_ethdev/rte_apistats.c
> > > new file mode 100644
> > > index 0000000..c4bde34
> > > --- /dev/null
> > > +++ b/lib/librte_ethdev/rte_apistats.c
> > > @@ -0,0 +1,64 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright(c) 2019 NTT TechnoCross Corporation
> > > + */
> > > +
> > > +
> > > +#include <unistd.h>
> > > +#include <sys/types.h>
> > > +#include <string.h>
> > > +#include <rte_log.h>
> > > +#include <rte_memzone.h>
> > > +#include <rte_lcore.h>
> > > +
> > > +#include "rte_apistats.h"
> > > +
> > > +/* Macros for printing using RTE_LOG */
> > > +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
> > > +
> > > +#define MZ_APISTATS "rte_apistats"
> > > +
> > > +struct rte_apistats *rte_apicounts;
> > > +
> > > +int rte_apistats_init(void)
> > > +{
> > > +	int i;
> > > +	const struct rte_memzone *mz = NULL;
> > > +	const unsigned int flags = 0;
> > > +
> > > +	/** Allocate stats in shared memory fo multi process support */
> > > +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > > +		mz = rte_memzone_lookup(MZ_APISTATS);
> > > +		if (mz == NULL) {
> > > +			RTE_LOG(ERR, APISTATS, "Cannot get info structure\n");
> > > +			return -1;
> > > +		}
> > > +		rte_apicounts = mz->addr;
> > > +	} else {
> > > +		/* RTE_PROC_PRIMARY */
> > > +		mz = rte_memzone_reserve(MZ_APISTATS, sizeof(*rte_apicounts),
> > > +			rte_socket_id(), flags);
> > > +		if (mz == NULL) {
> > > +			RTE_LOG(ERR, APISTATS, "Cannot reserve memory zone\n");
> > > +			return -ENOMEM;
> > > +		}
> > > +		rte_apicounts = mz->addr;
> > > +		memset(rte_apicounts, 0, sizeof(*rte_apicounts));
> > > +	}
> > > +
> > > +	/* set up array for data */
> > > +	RTE_LCORE_FOREACH(i) {
> > > +		rte_apicounts->lcoreid_list[i] = 1;
> > > +		RTE_LOG(INFO, APISTATS, "Enable core usage for lcore %u\n", i);
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +int rte_apistats_uninit(void)
> > > +{
> > > +	const struct rte_memzone *mz = NULL;
> > > +	/* free up the memzone */
> > > +	mz = rte_memzone_lookup(MZ_APISTATS);
> > > +	if (mz)
> > > +		rte_memzone_free(mz);
> > > +	return 0;
> > > +}
> > > diff --git a/lib/librte_ethdev/rte_apistats.h b/lib/librte_ethdev/rte_apistats.h
> > > new file mode 100644
> > > index 0000000..afea50e
> > > --- /dev/null
> > > +++ b/lib/librte_ethdev/rte_apistats.h
> > > @@ -0,0 +1,64 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright(c) 2019 NTT TechnoCross Corporation
> > > + */
> > > +
> > > +#ifndef _RTE_APISTATS_H_
> > > +#define _RTE_APISTATS_H_
> > > +
> > > +/**
> > > + * @file
> > > + * RTE apistats
> > > + *
> > > + * library to provide rte_rx_burst/tx_burst api stats.
> > > + */
> > > +
> > > +
> > > +#ifdef __cplusplus
> > > +extern "C" {
> > > +#endif
> > > +
> > > +#include <rte_compat.h>
> > > +#include <rte_lcore.h>
> > > +
> > > +/**
> > > + * A structure for rte_rx_burst/tx_burst api statistics.
> > > + */
> > > +struct rte_apistats {
> > > +	int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
> > > +	/**< Total rte_rx_burst call counts */
> > > +	uint64_t rx_burst_counts[RTE_MAX_LCORE];
> > > +
> > > +	/**< Total rte_tx_burst call counts */
> > > +	uint64_t tx_burst_counts[RTE_MAX_LCORE];
> > > +};
> > > +
> > > +extern struct rte_apistats *rte_apicounts;
> > > +
> > > +/**
> > > + *  Initialize rte_rx_burst/tx_burst call count area.
> > > + *  @b EXPERIMENTAL: this API may change without prior notice.
> > > + *
> > > + *  @return
> > > + *   -1     : On error
> > > + *   -ENOMEM: On error
> > > + *    0     : On success
> > > + */
> > > +__rte_experimental
> > > +int rte_apistats_init(void);
> > > +
> > > +/**
> > > + *  Clean up and free memory.
> > > + *  @b EXPERIMENTAL: this API may change without prior notice.
> > > + *
> > > + *  @return
> > > + *   -1: On error
> > > + *    0: On success
> > > + */
> > > +__rte_experimental
> > > +int rte_apistats_uninit(void);
> > > +
> > > +#ifdef __cplusplus
> > > +}
> > > +#endif
> > > +
> > > +#endif /* _RTE_APISTATS_H_ */
> > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > > index f5f8919..bef9bc6 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -160,6 +160,7 @@ extern "C" {
> > >
> > >  #include "rte_ethdev_trace_fp.h"
> > >  #include "rte_dev_info.h"
> > > +#include <rte_apistats.h>
> > >
> > >  extern int rte_eth_dev_logtype;
> > >
> > > @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> > >  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> > >  				     rx_pkts, nb_pkts);
> > >
> > > +	int lcore_id = rte_lcore_id();
> > > +	rte_apicounts->rx_burst_counts[lcore_id]++;
> >
> > There are few problems with current implementation:
> > 1.  rx_burst() can be called by non-EAL thread (rte_lcore_id() == -1)
> >     In that case it would cause a crash.
> [HY]
> Thanks for your info.
> I think by adding code like following.
> 
>      if(rte_lcore_id() = -1){
> 		return
>       }


That's not a good idea, as it would break existing functionality.
 
> 
> > 2. Because of the layout of struct rte_apistats it would cause significant
> >     performance degradation (false cache-lines sharing).
> [HY]
> I think you are correct.
> This affects change in core 1 to all other cores
> even thogh change in core 1 does NOT affect other cores.
> 
> Root cause is using array like [RTE_MAX_LCORE], correct?

Yes, you have several arrays in which each elem used by different thread,
but these elems share the same cache-line.

> I will change it in revised patcheset.
> 
> +struct rte_apistats {
> +       int lcoreid_list[RTE_MAX_LCORE];        /**< In use lcoreid list */
> +       /**< Total rte_rx_burst call counts */
> +       uint64_t rx_burst_counts[RTE_MAX_LCORE];
> +
> +       /**< Total rte_tx_burst call counts */
> +       uint64_t tx_burst_counts[RTE_MAX_LCORE];
> +};
> 
> 
> > As a generic one: such sort of statistics can be easily collected by the app itself.
> > Either by just incrementing counters before rx/tx_burst function call directly or
> > via rx/tx callbacks.
> > So I don't see any point to push it inside ethdev layer.
> > Konstantin
> [HY]
> You are correct.
> Application can do it.
> But if applications want to do it, then every applicaiton needs
> to do it.

Not *every* application needs that sort of stats.
Some can happily exist without it, while others might want to collect
something more sophisticated. Let say not only for per lcore_id,
but also per lcore+port_id, or lcore+port_id+queue_id, or ...
It is not possible to predict all combinations here.
If we'll allow people to add into rx_burst() every stats their apps need,
very soon rx_burst() will become slow and umnaintenable. 

> The reason why I propose my patchset is to provide such
> common function (count invocation of rx_burst/tx_burst)
> so that application only needs to "retrieve the information".
>
> I think it is benefitical than all application  do similar thing.

If you have several apps that do need that sort of app, then
there is at least two easy ways to achieve that:
1. have a common wrapper function around rx_burst(),
that would do rx_burst() plus stats collection.
Then use this wrapper function inside your apps.
2. If you want it to be totally 'transparent' to the app:
create/install  an rx callback function that would do such stats collection.  

> Your feedback is highly appreciated.
> 
> Thanks!
> BR,
> Hideyuki Yamashita
> NTT TechnoCross
> 
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > >  	struct rte_eth_rxtx_callback *cb;
> > >
> > > @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
> > >  	}
> > >  #endif
> > >
> > > +	int lcore_id = rte_lcore_id();
> > > +	rte_apicounts->tx_burst_counts[lcore_id]++;
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > >  	struct rte_eth_rxtx_callback *cb;
> > >
> > > diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> > > index d3f5410..adea432 100644
> > > --- a/lib/librte_ethdev/version.map
> > > +++ b/lib/librte_ethdev/version.map
> > > @@ -240,6 +240,11 @@ EXPERIMENTAL {
> > >  	rte_flow_get_restore_info;
> > >  	rte_flow_tunnel_action_decap_release;
> > >  	rte_flow_tunnel_item_release;
> > > +
> > > +        # added in 21.02
> > > +        rte_apistats_init;
> > > +        rte_apistats_uninit;
> > > +        rte_apicounts;
> > >  };
> > >
> > >  INTERNAL {
> > > --
> > > 2.18.0
> 

Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

[Hideyuki Yamashita]

Hello,

Thanks for your feedback.
> On Fri, 04 Dec 2020 16:51:09 +0900
> Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp> wrote:
> 
> > +
> > +/* Macros for printing using RTE_LOG */
> > +#define RTE_LOGTYPE_APISTATS RTE_LOGTYPE_USER1
> > +
> 
> Please don't use static logtypes.
> Better to allocate a dynamic logtype value and use that.

I understand you are saying to use the following.
I will incorporate your comments in upcoming revised patch set.

#define RTE_LOG_REGISTER (   type,  
    name,  
    level  
 ) 

https://doc.dpdk.org/api/rte__log_8h.html

Thanks!
BR,
Hideyuki Yamahista
NTT TechnoCross

Re: [dpdk-dev] [PATCH 1/5] maintainers: update maintainers file for apistats

[Hideyuki Yamashita]

Hello,

Sorry about the following.
I typo in email address in "signed-off-by".

I will revise it in  the upcoming revised patchset.
Sorry!

BR,
Hideyuki Yamashita
NTT TechnoCross

> Look like I am facing `mail delivery` issues to `ntt-tx.co.jp`.
> 
> ```
> The following message to <yamashtia.hideyuki@ntt-tx.co.jp> was undeliverable.
> The reason for the problem:
> 5.1.0 - Unknown address error 550-'5.1.1 <yamashtia.hideyuki@ntt-tx.co.jp>: Recipient address rejected: User unknown in relay recipient table'
> ```
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Hideyuki Yamashita
> > Sent: Friday, December 4, 2020 1:21 PM
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: dev@dpdk.org; Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>;
> > Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> > Subject: [dpdk-dev] [PATCH 1/5] maintainers: update maintainers file for
> > apistats
> > 
> > This patch adds maintainer of rte_apistats.c and rte_apistats.h.
> > 
> > Signed-off-by: Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
> > ---
> >  MAINTAINERS | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index eafe9f8..dba2acf 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -489,6 +489,9 @@ F: drivers/raw/skeleton/
> >  F: app/test/test_rawdev.c
> >  F: doc/guides/prog_guide/rawdev.rst
> > 
> > +API stats API -EXPERIMENTAL
> > +M: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>
> > +F: lib/librte_ethdev/rte_apistats.*
> > 
> >  Memory Pool Drivers
> >  -------------------
> > --
> > 2.18.0

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Hideyuki Yamashita]

Hello 

Thanks for your comments.
I know you kindly provided many valuable comments
though I reply the following first because I think it is 
important that my idea/proposal is acceptable or not
first.

> Sharing an alternate approach, if RX-TX callbacks are enabled in DPDK (which is by default). One can register a callback handler to update counters with the following information as `port-queue pair, lcoreid, total rx burst request, total empty rx burst, 1-8 pks, 9-16 pkts, 16-32 pkts`. Callback handlers can be selectively enabled or disabled too.
> 
> Can you please help me understand how `rte_apistats` would be different or pros of having it as library (that needs to be added to linking and running in case of using DPDK applications)?
You are correct.
Application can do that by using callback of rx_burst/tx_burst.
But needs to modify/add the code for RX/TX process logic.

Point of my patchset is couting is done by library 
so that APP only needs to "retrieve/read" those data
if needed (especially for existing applications).

I think it makes some developers happy becaseu it is relatively easy to
measure "how cpu core is bysy" easily.
(I admit relatively rough measurement though. I think it is trade-off)

What do you think?

Thanks!
BR,
Hideyuki Yamashita
NTT TechnoCross

> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of David Hunt
> > Sent: Friday, December 4, 2020 3:51 PM
> > To: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/5] add apistats function
> > 
> > 
> > On 4/12/2020 7:51 AM, Hideyuki Yamashita wrote:
> > > In general, DPDK application consumes CPU usage because it polls
> > > incoming packets using rx_burst API in infinite loop.
> > > This makes difficult to estimate how much CPU usage is really used to
> > > send/receive packets by the DPDK application.
> > >
> > > For example, even if no incoming packets arriving, CPU usage looks
> > > nearly 100% when observed by top command.
> > >
> > > It is beneficial if developers can observe real CPU usage of the DPDK
> > > application.
> > > Such information can be exported to monitoring application like
> > > prometheus/graphana and shows CPU usage graphically.
> > >
> > > To achieve above, this patch set provides apistats functionality.
> > > apistats provides the followiing two counters for each lcore.
> > > - rx_burst_counts[RTE_MAX_LCORE]
> > > - tx_burst_counts[RTE_MAX_LCORE]
> > > Those accumulates rx_burst/tx_burst counts since the application starts.
> > >
> > > By using those values, developers can roughly estimate CPU usage.
> > > Let us assume a DPDK application is simply forwarding packets.
> > > It calls tx_burst only if it receive packets.
> > > If rx_burst_counts=1000 and tx_burst_count=1000 during certain period
> > > of time, one can assume CPU usage is 100%.
> > > If rx_burst_counts=1000 and tx_burst_count=100 during certain period
> > > of time, one can assume CPU usage is 10%.
> > > Here we assumes that tx_burst_count equals counts which rx_burst
> > > function really receives incoming packets.
> > >
> > >
> > > This patch set provides the following.
> > > - basic API counting functionality(apistats) into librte_ethdev
> > > - add code to testpmd to accumulate counter information
> > > - add code to proc-info to retrieve above mentioned counter
> > > information
> > > - add description in proc-info document about --apistats parameter
> > > - modify MAINTAINERS file for apistats.c and apistats.h
> > >
> > > Hideyuki Yamashita (5):
> > >    maintainers: update maintainers file for apistats
> > >    app/proc-info: add to use apistats
> > >    app/test-pmd: add to use apistats
> > >    docs: add description of apistats parameter into proc-info
> > >    librte_ethdev: add to use apistats
> > >
> > >   MAINTAINERS                      |  3 ++
> > >   app/proc-info/main.c             | 46 +++++++++++++++++++++++
> > >   app/test-pmd/testpmd.c           |  4 ++
> > >   doc/guides/tools/proc_info.rst   | 10 ++++-
> > >   lib/librte_ethdev/meson.build    |  6 ++-
> > >   lib/librte_ethdev/rte_apistats.c | 64 ++++++++++++++++++++++++++++++++
> > >   lib/librte_ethdev/rte_apistats.h | 64 ++++++++++++++++++++++++++++++++
> > >   lib/librte_ethdev/rte_ethdev.h   |  7 ++++
> > >   lib/librte_ethdev/version.map    |  5 +++
> > >   9 files changed, 205 insertions(+), 4 deletions(-)
> > >   create mode 100644 lib/librte_ethdev/rte_apistats.c
> > >   create mode 100644 lib/librte_ethdev/rte_apistats.h
> > 
> > 
> > Hi Hideyuki,
> > 
> > I have a few questions on the patch set.
> > 
> > How does this compare to the mechanism added to l3fwd-power which counts
> > the number of empty, partial and full polls, and uses them to calculate
> > busyness? We saw pretty good tracking of busyness using those metrics. I
> > would be concerned that just looking at the numebr of rx_bursts and tx_bursts
> > may be limited to only a few use-cases. The l3fwd-power example uses
> > branchless increments to capture empty, non-empty, full, and non-full polls.
> > 
> > Why not use the existing telemetry library to store the stats? It would be good
> > if whatever metrics were counted were made available in a standard way, so
> > that external entities such as collectd could pick them up, rather than having to
> > parse the new struct. The l3fwd-power example registers the necessary new
> > metrics, and exposes them through the telemetry library.
> > 
> > And a comment on the patch set in general: The order of the patch set seems
> > reversed. The earlier patch do not compile, because they depend on
> > rte_apistats.h, which is introduced in the final patch. Each patch as it is applied
> > needs to build successfully.
> > 
> > Also, I would suggest a different name. rte_apistats seems very generic and
> > could apply to anything. How about something like rte_ethdev_stats.h or
> > rte_burst_stats.h?
> > 
> > Rgds,
> > Dave.
> > 
> > 
> > 
> > 
> > 
> > 
> 

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Varghese, Vipin]

snipped
> 
> Thanks for your comments.
> I know you kindly provided many valuable comments though I reply the
> following first because I think it is important that my idea/proposal is
> acceptable or not first.
> 
> > Sharing an alternate approach, if RX-TX callbacks are enabled in DPDK (which
> is by default). One can register a callback handler to update counters with the
> following information as `port-queue pair, lcoreid, total rx burst request, total
> empty rx burst, 1-8 pks, 9-16 pkts, 16-32 pkts`. Callback handlers can be
> selectively enabled or disabled too.
> >
> > Can you please help me understand how `rte_apistats` would be different or
> pros of having it as library (that needs to be added to linking and running in
> case of using DPDK applications)?
> You are correct.
> Application can do that by using callback of rx_burst/tx_burst.
> But needs to modify/add the code for RX/TX process logic.

No the RX or TX application logic is not modified as the call back registration is done once right after port_init.

> 
> Point of my patchset is couting is done by library so that APP only needs to
> "retrieve/read" those data if needed (especially for existing applications).

As mentioned in the other patchset the library function is enabled through and not conditionally built. Any application which is built with this patch set will be forced to invoke the calls.

> 
> I think it makes some developers happy becaseu it is relatively easy to measure
> "how cpu core is bysy" easily.

Not sure what do you mean, as there 2 factors which conflicts
1. there are existing uses cases and examples like power, metric, telemetry all uses RX/TX callbacks which does the same.
2. The current logic only helps in RX/TX cores and not other cores. So in case of pipeline model there are only a couple of RX or TX threads.

> (I admit relatively rough measurement though. I think it is trade-off)
> 
> What do you think?

If I consider RX calls there are 3 main metrics
1. How many times RX is invoked.
2. How many times RX returns with `0` packets
3. How many times RX returns with `> 0` packets.

With these values in the current logic you are trying to deduct actual CPU RX usage by `useful work = number of times `> 0` / total RX calls`

As a end user I will always be happy to see telemetry data as `from time t1 to t2, 
1. how many times per second on average RX calls were made.
2. how many times per second on average The calls returned packets in range of 1-4, 5-8, 9-16, 17 and more  `

Current logic is not trying to address this problem. With my current understanding I am not convinced that one needs yet another library when the same can be done either with `RX/TX callback`

> 
snipped

Re: [dpdk-dev] [PATCH 0/5] add apistats function

[Ferruh Yigit]

On 12/22/2020 2:22 AM, Hideyuki Yamashita wrote:
> Hello,
> 
> Thanks for your comments.
> Please see my comments inline tagged with [HY].
> 
>> 04/12/2020 08:51, Hideyuki Yamashita:
>>> In general, DPDK application consumes CPU usage because it polls
>>> incoming packets using rx_burst API in infinite loop.
>>> This makes difficult to estimate how much CPU usage is really
>>> used to send/receive packets by the DPDK application.
>>>
>>> For example, even if no incoming packets arriving, CPU usage
>>> looks nearly 100% when observed by top command.
>>>
>>> It is beneficial if developers can observe real CPU usage of the
>>> DPDK application.
>>> Such information can be exported to monitoring application like
>>> prometheus/graphana and shows CPU usage graphically.
>>>
>>> To achieve above, this patch set provides apistats functionality.
>>> apistats provides the followiing two counters for each lcore.
>>> - rx_burst_counts[RTE_MAX_LCORE]
>>> - tx_burst_counts[RTE_MAX_LCORE]
>>> Those accumulates rx_burst/tx_burst counts since the application starts.
>>
>> Please could you compare with what rte_jobstats offers?
>> http://code.dpdk.org/dpdk/latest/source/lib/librte_jobstats/rte_jobstats.h
>>
>> I feel all of this shares the same goals as librte_power work.
> 
> [HY]
> Thanks for your commetns.
> You are correct. As you well know, l3fwd-power measures "how cpu cores
> are busy".
> And in that sense, the goal of my proposal is the same with yours .
> Moreover l3fwd-power is more precise than my proposal.
> 
> Point of my proposal is
> - more easy to use
> - less code impact on application code
> 
> I think that if application developer wants to need to measure "how cpu
> cores are busy" he/she will needs to implement
> - logic similar with l3fwd-power
> or
> - use jobstats API
> 
> But it is rather heavy for existing applications.
> By using my proposal, it is "much easier" to implement.
> (But it is "rough" measurement. I think it is trade-off)
>   
> How do you think about the idea?
> 
>> [...]
>>> - basic API counting functionality(apistats) into librte_ethdev
>>
>> Could it be it be accessible via rte_telemetry?
>> http://code.dpdk.org/dpdk/latest/source/lib/librte_telemetry/rte_telemetry.h
>>
> [HY]
> OK.
> Currently, no reason not using telemetry.
> 
> I think telemetry is useful for applications which does NOT call DPDK
> API(C lang API) directly.
> My patchset provide only C API to retrieve apistats.
> But if assuming not all applications call C API, then I think it is
> reasonable to add telemetry in addition to C API for exposing stats.
> 
> Do you think "exposure via C API" is not needed?
> 

Hi Hideyuki,

With current implementation the change will cause a performance degradation for 
all users, even if they use the stats or not.

Even if the statics update wrapped with #ifdef, as suggested, the functionality 
is simple and it can be easily implemented by application using Rx/Tx callbacks, 
again as suggested, without introducing this new complexity.

And for more complex usage, the jobstats already tries to provide a generic 
library for it, or application specific needs can be implemented in application 
level as done is l3fwd-power.


Overall I agree the problem this patch is trying to solve is a valid one, but 
not sure about the current implementation, and I am marking current version as 
'rejected', please feel free to send a new version with a new approach.

Thanks,
ferruh