DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: Hideyuki Yamashita <yamashita.hideyuki@ntt-tx.co.jp>,
	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>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Hideyuki Yamashita <yamashtia.hideyuki@ntt-tx.co.jp>
Subject: Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats
Date: Sat, 5 Dec 2020 13:01:50 +0000
Message-ID: <MWHPR11MB15810152CDC9C0A52488291E90F00@MWHPR11MB1581.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201204075109.14694-6-yamashita.hideyuki@ntt-tx.co.jp>

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


  reply	other threads:[~2020-12-05 13:02 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 [this message]
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
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=MWHPR11MB15810152CDC9C0A52488291E90F00@MWHPR11MB1581.namprd11.prod.outlook.com \
    --to=vipin.varghese@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 \
    --cc=yamashtia.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