[5/5] librte_ethdev: add to use apistats
Checks
Commit Message
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
Comments
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
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.
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
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
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
> 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;
> > >
> 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
>
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
@@ -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']
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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_ */
@@ -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;
@@ -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 {