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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 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
>