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

Varghese, Vipin vipin.varghese at intel.com
Sat Dec 5 14:01:50 CET 2020


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



More information about the dev mailing list