DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
	Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>,
	"Burakov, Anatoly" <anatoly.burakov@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats
Date: Tue, 22 Dec 2020 13:05:02 +0000
Message-ID: <BYAPR11MB330151563F12C5D972A800B19ADF0@BYAPR11MB3301.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201222115006.CFA6.17218CA3@ntt-tx.co.jp>

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


  parent reply	other threads:[~2020-12-22 13:05 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-04  7:51 [dpdk-dev] [PATCH 0/5] add apistats function Hideyuki Yamashita
2020-12-04  7:51 ` [dpdk-dev] [PATCH 1/5] maintainers: update maintainers file for apistats Hideyuki Yamashita
2020-12-05 13:27   ` Varghese, Vipin
2020-12-24  6:36     ` Hideyuki Yamashita
2020-12-04  7:51 ` [dpdk-dev] [PATCH 2/5] app/proc-info: add to use apistats Hideyuki Yamashita
2020-12-05 13:15   ` Varghese, Vipin
2020-12-04  7:51 ` [dpdk-dev] [PATCH 3/5] app/test-pmd: " Hideyuki Yamashita
2020-12-05 13:04   ` Varghese, Vipin
2020-12-04  7:51 ` [dpdk-dev] [PATCH 4/5] docs: add description of apistats parameter into proc-info Hideyuki Yamashita
2020-12-05 13:02   ` Varghese, Vipin
2020-12-07  9:48     ` Pattan, Reshma
2020-12-04  7:51 ` [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats Hideyuki Yamashita
2020-12-05 13:01   ` Varghese, Vipin
2020-12-06 17:47   ` Stephen Hemminger
2020-12-24  6:33     ` Hideyuki Yamashita
2020-12-07 12:38   ` Ananyev, Konstantin
2020-12-22  2:48     ` Hideyuki Yamashita
2020-12-22  2:50     ` Hideyuki Yamashita
2020-12-22  9:04       ` Morten Brørup
2020-12-22 13:05       ` Ananyev, Konstantin [this message]
2020-12-04  9:09 ` [dpdk-dev] [PATCH 0/5] add apistats function Ferruh Yigit
2020-12-04 10:20 ` David Hunt
2020-12-05 13:23   ` Varghese, Vipin
2020-12-24  6:43     ` Hideyuki Yamashita
2020-12-24 12:35       ` Varghese, Vipin
2020-12-22  2:16   ` Hideyuki Yamashita
2020-12-07  9:46 ` Thomas Monjalon
2020-12-22  2:22   ` Hideyuki Yamashita
2021-02-22 15:10     ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR11MB330151563F12C5D972A800B19ADF0@BYAPR11MB3301.namprd11.prod.outlook.com \
    --to=konstantin.ananyev@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=mdr@ashroe.eu \
    --cc=nhorman@tuxdriver.com \
    --cc=thomas@monjalon.net \
    --cc=yamashita.hideyuki@ntt-tx.co.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git