All of lore.kernel.org
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v2] lib/mempool: distinguish debug counters from cache and pool
@ 2021-03-18 11:20 Joyce Kong
  2021-04-07 14:28 ` Olivier Matz
  2021-04-20  0:07 ` [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats Dharmik Thakkar
  0 siblings, 2 replies; 22+ messages in thread
From: Joyce Kong @ 2021-03-18 11:20 UTC (permalink / raw)
  To: olivier.matz, andrew.rybchenko, ruifeng.wang, honnappa.nagarahalli
  Cc: dev, nd

If cache is enabled, objects will be retrieved/put from/to cache,
subsequently from/to the common pool. Now the debug stats calculate
the objects retrieved/put from/to cache and pool together, it is
better to distinguish the data number from local cache and common
pool.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
---
 lib/librte_mempool/rte_mempool.c | 12 ++++++
 lib/librte_mempool/rte_mempool.h | 64 ++++++++++++++++++++++----------
 2 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index afb1239c8..9cb69367a 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1244,8 +1244,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
 		sum.put_objs += mp->stats[lcore_id].put_objs;
+		sum.put_objs_cache += mp->stats[lcore_id].put_objs_cache;
+		sum.put_objs_pool += mp->stats[lcore_id].put_objs_pool;
+		sum.put_objs_flush += mp->stats[lcore_id].put_objs_flush;
 		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
 		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
+		sum.get_success_objs_cache += mp->stats[lcore_id].get_success_objs_cache;
+		sum.get_success_objs_pool += mp->stats[lcore_id].get_success_objs_pool;
+		sum.get_success_objs_refill += mp->stats[lcore_id].get_success_objs_refill;
 		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
 		sum.get_fail_objs += mp->stats[lcore_id].get_fail_objs;
 		sum.get_success_blks += mp->stats[lcore_id].get_success_blks;
@@ -1254,8 +1260,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	fprintf(f, "  stats:\n");
 	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
 	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
+	fprintf(f, "    put_objs_cache=%"PRIu64"\n", sum.put_objs_cache);
+	fprintf(f, "    put_objs_pool=%"PRIu64"\n", sum.put_objs_pool);
+	fprintf(f, "    put_objs_flush=%"PRIu64"\n", sum.put_objs_flush);
 	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
 	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
+	fprintf(f, "    get_success_objs_cache=%"PRIu64"\n", sum.get_success_objs_cache);
+	fprintf(f, "    get_success_objs_pool=%"PRIu64"\n", sum.get_success_objs_pool);
+	fprintf(f, "    get_success_objs_refill=%"PRIu64"\n", sum.get_success_objs_refill);
 	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
 	fprintf(f, "    get_fail_objs=%"PRIu64"\n", sum.get_fail_objs);
 	if (info.contig_block_size > 0) {
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index c551cf733..29d80d97e 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -66,12 +66,18 @@ extern "C" {
  * A structure that stores the mempool statistics (per-lcore).
  */
 struct rte_mempool_debug_stats {
-	uint64_t put_bulk;         /**< Number of puts. */
-	uint64_t put_objs;         /**< Number of objects successfully put. */
-	uint64_t get_success_bulk; /**< Successful allocation number. */
-	uint64_t get_success_objs; /**< Objects successfully allocated. */
-	uint64_t get_fail_bulk;    /**< Failed allocation number. */
-	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
+	uint64_t put_bulk;		  /**< Number of puts. */
+	uint64_t put_objs;		  /**< Number of objects successfully put. */
+	uint64_t put_objs_cache;	  /**< Number of objects successfully put to cache. */
+	uint64_t put_objs_pool;		  /**< Number of objects successfully put to pool. */
+	uint64_t put_objs_flush;	  /**< Number of flushing objects from cache to pool. */
+	uint64_t get_success_bulk;	  /**< Successful allocation number. */
+	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
+	uint64_t get_success_objs_cache;  /**< Objects successfully allocated from cache. */
+	uint64_t get_success_objs_pool;	  /**< Objects successfully allocated from pool. */
+	uint64_t get_success_objs_refill; /**< Number of refilling objects from pool to cache. */
+	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
+	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
 	/** Successful allocation number of contiguous blocks. */
 	uint64_t get_success_blks;
 	/** Failed allocation number of contiguous blocks. */
@@ -270,22 +276,34 @@ struct rte_mempool {
  *   Number to add to the object-oriented statistics.
  */
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
-#define __MEMPOOL_STAT_ADD(mp, name, n) do {                    \
-		unsigned __lcore_id = rte_lcore_id();           \
-		if (__lcore_id < RTE_MAX_LCORE) {               \
+#define __MEMPOOL_STAT_ADD(mp, name, n) do {			\
+		unsigned __lcore_id = rte_lcore_id();		\
+		if (__lcore_id < RTE_MAX_LCORE) {		\
 			mp->stats[__lcore_id].name##_objs += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
-		}                                               \
-	} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {                    \
-		unsigned int __lcore_id = rte_lcore_id();       \
-		if (__lcore_id < RTE_MAX_LCORE) {               \
+			mp->stats[__lcore_id].name##_bulk += 1; \
+		}						\
+	} while (0)
+#define __MEMPOOL_OBJS_STAT_ADD(mp, name1, name2, n) do {	\
+		unsigned __lcore_id = rte_lcore_id();		\
+		if (__lcore_id < RTE_MAX_LCORE)			\
+			mp->stats[__lcore_id].name1##_objs_##name2 += n;	\
+	} while (0)
+#define __MEMPOOL_OBJS_STAT_SUB(mp, name1, name2, n) do {	\
+		unsigned __lcore_id = rte_lcore_id();		\
+		if (__lcore_id < RTE_MAX_LCORE)			\
+			mp->stats[__lcore_id].name1##_objs_##name2 -= n;	\
+	} while (0)
+#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {	\
+		unsigned int __lcore_id = rte_lcore_id();	\
+		if (__lcore_id < RTE_MAX_LCORE) {		\
 			mp->stats[__lcore_id].name##_blks += n;	\
 			mp->stats[__lcore_id].name##_bulk += 1;	\
-		}                                               \
+		}						\
 	} while (0)
 #else
-#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
+#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while (0)
+#define __MEMPOOL_OBJS_STAT_ADD(mp, name1, name2, n) do {} while (0)
+#define __MEMPOOL_OBJS_STAT_SUB(mp, name1, nmae2, n) do {} while (0)
 #define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {} while (0)
 #endif
 
@@ -1305,10 +1323,13 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 
 	/* Add elements back into the cache */
 	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
-
+	__MEMPOOL_OBJS_STAT_ADD(mp, put, cache, n);
 	cache->len += n;
 
 	if (cache->len >= cache->flushthresh) {
+		__MEMPOOL_OBJS_STAT_SUB(mp, put, cache, cache->len - cache->size);
+		__MEMPOOL_OBJS_STAT_ADD(mp, put, pool, cache->len - cache->size);
+		__MEMPOOL_OBJS_STAT_ADD(mp, put, flush, 1);
 		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
 				cache->len - cache->size);
 		cache->len = cache->size;
@@ -1318,6 +1339,7 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 
 ring_enqueue:
 
+	__MEMPOOL_OBJS_STAT_ADD(mp, put, pool, n);
 	/* push remaining objects in ring */
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
 	if (rte_mempool_ops_enqueue_bulk(mp, obj_table, n) < 0)
@@ -1437,6 +1459,7 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 			goto ring_dequeue;
 		}
 
+		__MEMPOOL_OBJS_STAT_ADD(mp, get_success, refill, 1);
 		cache->len += req;
 	}
 
@@ -1447,6 +1470,7 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 	cache->len -= n;
 
 	__MEMPOOL_STAT_ADD(mp, get_success, n);
+	__MEMPOOL_OBJS_STAT_ADD(mp, get_success, cache, n);
 
 	return 0;
 
@@ -1457,8 +1481,10 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 
 	if (ret < 0)
 		__MEMPOOL_STAT_ADD(mp, get_fail, n);
-	else
+	else {
 		__MEMPOOL_STAT_ADD(mp, get_success, n);
+		__MEMPOOL_OBJS_STAT_ADD(mp, get_success, pool, n);
+	}
 
 	return ret;
 }
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] lib/mempool: distinguish debug counters from cache and pool
  2021-03-18 11:20 [dpdk-dev] [PATCH v2] lib/mempool: distinguish debug counters from cache and pool Joyce Kong
@ 2021-04-07 14:28 ` Olivier Matz
  2021-04-20  0:31   ` Dharmik Thakkar
  2021-04-20  0:07 ` [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats Dharmik Thakkar
  1 sibling, 1 reply; 22+ messages in thread
From: Olivier Matz @ 2021-04-07 14:28 UTC (permalink / raw)
  To: Joyce Kong; +Cc: andrew.rybchenko, ruifeng.wang, honnappa.nagarahalli, dev, nd

Hi Joyce,

On Thu, Mar 18, 2021 at 07:20:22PM +0800, Joyce Kong wrote:
> If cache is enabled, objects will be retrieved/put from/to cache,
> subsequently from/to the common pool. Now the debug stats calculate
> the objects retrieved/put from/to cache and pool together, it is
> better to distinguish the data number from local cache and common
> pool.

This is indeed a very useful information, thanks for proposing this.

Please see some comments below.

> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 12 ++++++
>  lib/librte_mempool/rte_mempool.h | 64 ++++++++++++++++++++++----------
>  2 files changed, 57 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index afb1239c8..9cb69367a 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -1244,8 +1244,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>  		sum.put_bulk += mp->stats[lcore_id].put_bulk;
>  		sum.put_objs += mp->stats[lcore_id].put_objs;
> +		sum.put_objs_cache += mp->stats[lcore_id].put_objs_cache;
> +		sum.put_objs_pool += mp->stats[lcore_id].put_objs_pool;
> +		sum.put_objs_flush += mp->stats[lcore_id].put_objs_flush;
>  		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
>  		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
> +		sum.get_success_objs_cache += mp->stats[lcore_id].get_success_objs_cache;
> +		sum.get_success_objs_pool += mp->stats[lcore_id].get_success_objs_pool;
> +		sum.get_success_objs_refill += mp->stats[lcore_id].get_success_objs_refill;
>  		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
>  		sum.get_fail_objs += mp->stats[lcore_id].get_fail_objs;
>  		sum.get_success_blks += mp->stats[lcore_id].get_success_blks;
> @@ -1254,8 +1260,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	fprintf(f, "  stats:\n");
>  	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
>  	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
> +	fprintf(f, "    put_objs_cache=%"PRIu64"\n", sum.put_objs_cache);
> +	fprintf(f, "    put_objs_pool=%"PRIu64"\n", sum.put_objs_pool);
> +	fprintf(f, "    put_objs_flush=%"PRIu64"\n", sum.put_objs_flush);
>  	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
>  	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
> +	fprintf(f, "    get_success_objs_cache=%"PRIu64"\n", sum.get_success_objs_cache);
> +	fprintf(f, "    get_success_objs_pool=%"PRIu64"\n", sum.get_success_objs_pool);
> +	fprintf(f, "    get_success_objs_refill=%"PRIu64"\n", sum.get_success_objs_refill);
>  	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
>  	fprintf(f, "    get_fail_objs=%"PRIu64"\n", sum.get_fail_objs);
>  	if (info.contig_block_size > 0) {
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index c551cf733..29d80d97e 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -66,12 +66,18 @@ extern "C" {
>   * A structure that stores the mempool statistics (per-lcore).
>   */
>  struct rte_mempool_debug_stats {
> -	uint64_t put_bulk;         /**< Number of puts. */
> -	uint64_t put_objs;         /**< Number of objects successfully put. */
> -	uint64_t get_success_bulk; /**< Successful allocation number. */
> -	uint64_t get_success_objs; /**< Objects successfully allocated. */
> -	uint64_t get_fail_bulk;    /**< Failed allocation number. */
> -	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
> +	uint64_t put_bulk;		  /**< Number of puts. */
> +	uint64_t put_objs;		  /**< Number of objects successfully put. */
> +	uint64_t put_objs_cache;	  /**< Number of objects successfully put to cache. */
> +	uint64_t put_objs_pool;		  /**< Number of objects successfully put to pool. */
> +	uint64_t put_objs_flush;	  /**< Number of flushing objects from cache to pool. */
> +	uint64_t get_success_bulk;	  /**< Successful allocation number. */
> +	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
> +	uint64_t get_success_objs_cache;  /**< Objects successfully allocated from cache. */
> +	uint64_t get_success_objs_pool;	  /**< Objects successfully allocated from pool. */
> +	uint64_t get_success_objs_refill; /**< Number of refilling objects from pool to cache. */
> +	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
> +	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */

What about having instead the following new stats:

- put_common_pool_bulk: number of bulks enqueued in common pool
- put_common_pool_objs: number of objects enqueued in common pool
- get_common_pool_bulk: number of bulks dequeued from common pool
- get_common_pool_objs: number of objects dequeued from common pool

It looks easier to me to understand, compared to
flush/refill. Especially, having 'objs' in the name makes me think that
it counts a number of objects, but it's not the case.

>  	/** Successful allocation number of contiguous blocks. */
>  	uint64_t get_success_blks;
>  	/** Failed allocation number of contiguous blocks. */
> @@ -270,22 +276,34 @@ struct rte_mempool {
>   *   Number to add to the object-oriented statistics.
>   */
>  #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> -#define __MEMPOOL_STAT_ADD(mp, name, n) do {                    \
> -		unsigned __lcore_id = rte_lcore_id();           \
> -		if (__lcore_id < RTE_MAX_LCORE) {               \
> +#define __MEMPOOL_STAT_ADD(mp, name, n) do {			\
> +		unsigned __lcore_id = rte_lcore_id();		\
> +		if (__lcore_id < RTE_MAX_LCORE) {		\
>  			mp->stats[__lcore_id].name##_objs += n;	\
> -			mp->stats[__lcore_id].name##_bulk += 1;	\
> -		}                                               \
> -	} while(0)
> -#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {                    \
> -		unsigned int __lcore_id = rte_lcore_id();       \
> -		if (__lcore_id < RTE_MAX_LCORE) {               \
> +			mp->stats[__lcore_id].name##_bulk += 1; \
> +		}						\
> +	} while (0)
> +#define __MEMPOOL_OBJS_STAT_ADD(mp, name1, name2, n) do {	\
> +		unsigned __lcore_id = rte_lcore_id();		\
> +		if (__lcore_id < RTE_MAX_LCORE)			\
> +			mp->stats[__lcore_id].name1##_objs_##name2 += n;	\
> +	} while (0)
> +#define __MEMPOOL_OBJS_STAT_SUB(mp, name1, name2, n) do {	\
> +		unsigned __lcore_id = rte_lcore_id();		\
> +		if (__lcore_id < RTE_MAX_LCORE)			\
> +			mp->stats[__lcore_id].name1##_objs_##name2 -= n;	\
> +	} while (0)
> +#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {	\
> +		unsigned int __lcore_id = rte_lcore_id();	\
> +		if (__lcore_id < RTE_MAX_LCORE) {		\
>  			mp->stats[__lcore_id].name##_blks += n;	\
>  			mp->stats[__lcore_id].name##_bulk += 1;	\
> -		}                                               \
> +		}						\
>  	} while (0)

There are too many stats macros. I think that the original
__MEMPOOL_STAT_ADD() macro can be reused if using the stats name I'm
suggesting.

Else, if not using names ending with _objs and _bulks, it would be
better to rework the macro to something more generic, like this:

  #define __MEMPOOL_STAT_ADD(mp, name, n) do {	\
  		unsigned __lcore_id = rte_lcore_id();		\
  		if (__lcore_id < RTE_MAX_LCORE)			\
  			mp->stats[__lcore_id].name += n;	\
  	} while (0)

In this case, it could replace both __MEMPOOL_STAT_ADD() and
__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(), and could be in a separate patch,
before this one. It would replace the 6 macro calls by 12, but would
be clearer because one can see the real field name.
Eventually the macro could take the lcore_id as a parameter.

>  #else
> -#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
> +#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while (0)
> +#define __MEMPOOL_OBJS_STAT_ADD(mp, name1, name2, n) do {} while (0)
> +#define __MEMPOOL_OBJS_STAT_SUB(mp, name1, nmae2, n) do {} while (0)
>  #define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {} while (0)
>  #endif
>  
> @@ -1305,10 +1323,13 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
>  
>  	/* Add elements back into the cache */
>  	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
> -
> +	__MEMPOOL_OBJS_STAT_ADD(mp, put, cache, n);
>  	cache->len += n;
>  
>  	if (cache->len >= cache->flushthresh) {
> +		__MEMPOOL_OBJS_STAT_SUB(mp, put, cache, cache->len - cache->size);

I don't think it is a good idea to decrease a statistic counter.
If using put_common_pool_bulk/put_common_pool_objs, I think the code could
go in rte_mempool_ops_enqueue_bulk() (and in rte_mempool_ops_dequeue_bulk() for
'get' stats).


> +		__MEMPOOL_OBJS_STAT_ADD(mp, put, pool, cache->len - cache->size);
> +		__MEMPOOL_OBJS_STAT_ADD(mp, put, flush, 1);
>  		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
>  				cache->len - cache->size);
>  		cache->len = cache->size;
> @@ -1318,6 +1339,7 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
>  
>  ring_enqueue:
>  
> +	__MEMPOOL_OBJS_STAT_ADD(mp, put, pool, n);
>  	/* push remaining objects in ring */
>  #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>  	if (rte_mempool_ops_enqueue_bulk(mp, obj_table, n) < 0)
> @@ -1437,6 +1459,7 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  			goto ring_dequeue;
>  		}
>  
> +		__MEMPOOL_OBJS_STAT_ADD(mp, get_success, refill, 1);
>  		cache->len += req;
>  	}
>  
> @@ -1447,6 +1470,7 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  	cache->len -= n;
>  
>  	__MEMPOOL_STAT_ADD(mp, get_success, n);
> +	__MEMPOOL_OBJS_STAT_ADD(mp, get_success, cache, n);
>  
>  	return 0;
>  
> @@ -1457,8 +1481,10 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  
>  	if (ret < 0)
>  		__MEMPOOL_STAT_ADD(mp, get_fail, n);
> -	else
> +	else {
>  		__MEMPOOL_STAT_ADD(mp, get_success, n);
> +		__MEMPOOL_OBJS_STAT_ADD(mp, get_success, pool, n);
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.30.0
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats
  2021-03-18 11:20 [dpdk-dev] [PATCH v2] lib/mempool: distinguish debug counters from cache and pool Joyce Kong
  2021-04-07 14:28 ` Olivier Matz
@ 2021-04-20  0:07 ` Dharmik Thakkar
  2021-04-20  0:07   ` [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
                     ` (2 more replies)
  1 sibling, 3 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-20  0:07 UTC (permalink / raw)
  Cc: dev, nd, joyce.kong, Dharmik Thakkar

- Add debug counters for objects put/get to/from the cache and
the common pool.
- Make __MEMPOOL_STAT_ADD() more generic

---
v3:
 - Add a patch to make stat add macro generic
 - Remove other stat add/subtract macros
 - Rename counters for better understanding
 - Add put/get cache bulk counters

v2:
 - Fix typo in the commit message
---

Dharmik Thakkar (1):
  lib/mempool: make stats macro generic

Joyce Kong (1):
  lib/mempool: distinguish debug counters from cache and pool

 lib/librte_mempool/rte_mempool.c | 24 +++++++++++
 lib/librte_mempool/rte_mempool.h | 71 ++++++++++++++++++++------------
 2 files changed, 68 insertions(+), 27 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic
  2021-04-20  0:07 ` [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats Dharmik Thakkar
@ 2021-04-20  0:07   ` Dharmik Thakkar
  2021-04-21 16:09     ` Olivier Matz
  2021-04-20  0:08   ` [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
  2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
  2 siblings, 1 reply; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-20  0:07 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko; +Cc: dev, nd, joyce.kong, Dharmik Thakkar

Make __MEMPOOL_STAT_ADD macro more generic and delete
__MEMPOOL_CONTIG_BLOCKS_STAT_ADD macro

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/librte_mempool/rte_mempool.h | 34 +++++++++++++++-----------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index c551cf733acf..848a19226149 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -273,20 +273,11 @@ struct rte_mempool {
 #define __MEMPOOL_STAT_ADD(mp, name, n) do {                    \
 		unsigned __lcore_id = rte_lcore_id();           \
 		if (__lcore_id < RTE_MAX_LCORE) {               \
-			mp->stats[__lcore_id].name##_objs += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
+			mp->stats[__lcore_id].name += n;	\
 		}                                               \
 	} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {                    \
-		unsigned int __lcore_id = rte_lcore_id();       \
-		if (__lcore_id < RTE_MAX_LCORE) {               \
-			mp->stats[__lcore_id].name##_blks += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
-		}                                               \
-	} while (0)
 #else
 #define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {} while (0)
 #endif
 
 /**
@@ -1288,7 +1279,8 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 	void **cache_objs;
 
 	/* increment stat now, adding in mempool always success */
-	__MEMPOOL_STAT_ADD(mp, put, n);
+	__MEMPOOL_STAT_ADD(mp, put_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, put_objs, n);
 
 	/* No cache provided or if put would overflow mem allocated for cache */
 	if (unlikely(cache == NULL || n > RTE_MEMPOOL_CACHE_MAX_SIZE))
@@ -1446,7 +1438,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 
 	cache->len -= n;
 
-	__MEMPOOL_STAT_ADD(mp, get_success, n);
+	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
 
 	return 0;
 
@@ -1455,10 +1448,13 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 	/* get remaining objects from ring */
 	ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n);
 
-	if (ret < 0)
-		__MEMPOOL_STAT_ADD(mp, get_fail, n);
-	else
-		__MEMPOOL_STAT_ADD(mp, get_success, n);
+	if (ret < 0) {
+		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
+	} else {
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
+	}
 
 	return ret;
 }
@@ -1581,11 +1577,13 @@ rte_mempool_get_contig_blocks(struct rte_mempool *mp,
 
 	ret = rte_mempool_ops_dequeue_contig_blocks(mp, first_obj_table, n);
 	if (ret == 0) {
-		__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, get_success, n);
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_blks, n);
 		__mempool_contig_blocks_check_cookies(mp, first_obj_table, n,
 						      1);
 	} else {
-		__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, get_fail, n);
+		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_fail_blks, n);
 	}
 
 	rte_mempool_trace_get_contig_blocks(mp, first_obj_table, n);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-20  0:07 ` [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats Dharmik Thakkar
  2021-04-20  0:07   ` [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
@ 2021-04-20  0:08   ` Dharmik Thakkar
  2021-04-21 16:29     ` Olivier Matz
  2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
  2 siblings, 1 reply; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-20  0:08 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko; +Cc: dev, nd, joyce.kong, Dharmik Thakkar

From: Joyce Kong <joyce.kong@arm.com>

If cache is enabled, objects will be retrieved/put from/to cache,
subsequently from/to the common pool. Now the debug stats calculate
the objects retrieved/put from/to cache and pool together, it is
better to distinguish them.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/librte_mempool/rte_mempool.c | 24 ++++++++++++++++
 lib/librte_mempool/rte_mempool.h | 47 ++++++++++++++++++++++----------
 2 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index afb1239c8d48..339f14455624 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1244,6 +1244,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
 		sum.put_objs += mp->stats[lcore_id].put_objs;
+		sum.put_common_pool_bulk +=
+			mp->stats[lcore_id].put_common_pool_bulk;
+		sum.put_common_pool_objs +=
+			mp->stats[lcore_id].put_common_pool_objs;
+		sum.put_cache_bulk += mp->stats[lcore_id].put_cache_bulk;
+		sum.put_cache_objs += mp->stats[lcore_id].put_cache_objs;
+		sum.get_common_pool_bulk +=
+			mp->stats[lcore_id].get_common_pool_bulk;
+		sum.get_common_pool_objs +=
+			mp->stats[lcore_id].get_common_pool_objs;
+		sum.get_cache_bulk += mp->stats[lcore_id].get_cache_bulk;
+		sum.get_cache_objs += mp->stats[lcore_id].get_cache_objs;
 		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
 		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
 		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
@@ -1254,6 +1266,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	fprintf(f, "  stats:\n");
 	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
 	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
+	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
+						sum.put_common_pool_bulk);
+	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
+						sum.put_common_pool_objs);
+	fprintf(f, "    put_cache_bulk=%"PRIu64"\n", sum.put_cache_bulk);
+	fprintf(f, "    put_cache_objs=%"PRIu64"\n", sum.put_cache_objs);
+	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
+						sum.get_common_pool_bulk);
+	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
+						sum.get_common_pool_objs);
+	fprintf(f, "    get_cache_bulk=%"PRIu64"\n", sum.get_cache_bulk);
+	fprintf(f, "    get_cache_objs=%"PRIu64"\n", sum.get_cache_objs);
 	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
 	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
 	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 848a19226149..0959f8a3f367 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -66,12 +66,20 @@ extern "C" {
  * A structure that stores the mempool statistics (per-lcore).
  */
 struct rte_mempool_debug_stats {
-	uint64_t put_bulk;         /**< Number of puts. */
-	uint64_t put_objs;         /**< Number of objects successfully put. */
-	uint64_t get_success_bulk; /**< Successful allocation number. */
-	uint64_t get_success_objs; /**< Objects successfully allocated. */
-	uint64_t get_fail_bulk;    /**< Failed allocation number. */
-	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
+	uint64_t put_bulk;		  /**< Number of puts. */
+	uint64_t put_objs;		  /**< Number of objects successfully put. */
+	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
+	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
+	uint64_t put_cache_bulk;	  /**< Number of bulks enqueued in cache. */
+	uint64_t put_cache_objs;	  /**< Number of objects enqueued in cache. */
+	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
+	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
+	uint64_t get_cache_bulk;	  /**< Number of bulks dequeued from cache. */
+	uint64_t get_cache_objs;	  /**< Number of objects dequeued from cache. */
+	uint64_t get_success_bulk;	  /**< Successful allocation number. */
+	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
+	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
+	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
 	/** Successful allocation number of contiguous blocks. */
 	uint64_t get_success_blks;
 	/** Failed allocation number of contiguous blocks. */
@@ -699,10 +707,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
 		void **obj_table, unsigned n)
 {
 	struct rte_mempool_ops *ops;
+	int ret;
 
 	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
 	ops = rte_mempool_get_ops(mp->ops_index);
-	return ops->dequeue(mp, obj_table, n);
+	ret = ops->dequeue(mp, obj_table, n);
+	if (ret == 0) {
+		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
+	}
+	return ret;
 }
 
 /**
@@ -749,6 +765,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
 {
 	struct rte_mempool_ops *ops;
 
+	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
 	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
 	ops = rte_mempool_get_ops(mp->ops_index);
 	return ops->enqueue(mp, obj_table, n);
@@ -1297,14 +1315,18 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 
 	/* Add elements back into the cache */
 	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
-
 	cache->len += n;
 
+	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
+
 	if (cache->len >= cache->flushthresh) {
+		__MEMPOOL_STAT_ADD(mp, put_cache_objs,
+				   n - (cache->len - cache->size));
 		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
 				cache->len - cache->size);
 		cache->len = cache->size;
-	}
+	} else
+		__MEMPOOL_STAT_ADD(mp, put_cache_objs, n);
 
 	return;
 
@@ -1438,8 +1460,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 
 	cache->len -= n;
 
-	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
-	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
+	__MEMPOOL_STAT_ADD(mp, get_cache_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, get_cache_objs, n);
 
 	return 0;
 
@@ -1451,9 +1473,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 	if (ret < 0) {
 		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
 		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
-	} else {
-		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
-		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
 	}
 
 	return ret;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-07 14:28 ` Olivier Matz
@ 2021-04-20  0:31   ` Dharmik Thakkar
  0 siblings, 0 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-20  0:31 UTC (permalink / raw)
  To: Olivier Matz
  Cc: Joyce Kong, andrew.rybchenko, Ruifeng Wang, Honnappa Nagarahalli,
	dev, nd

Hi Olivier,

Apologies for the delayed response!

I’m stepping in since Joyce is on vacation.
I have updated the patch with the required changes.
Thank you!

> On Apr 7, 2021, at 9:28 AM, Olivier Matz <olivier.matz@6wind.com> wrote:

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic
  2021-04-20  0:07   ` [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
@ 2021-04-21 16:09     ` Olivier Matz
  0 siblings, 0 replies; 22+ messages in thread
From: Olivier Matz @ 2021-04-21 16:09 UTC (permalink / raw)
  To: Dharmik Thakkar; +Cc: Andrew Rybchenko, dev, nd, joyce.kong

On Mon, Apr 19, 2021 at 07:07:59PM -0500, Dharmik Thakkar wrote:
> Make __MEMPOOL_STAT_ADD macro more generic and delete
> __MEMPOOL_CONTIG_BLOCKS_STAT_ADD macro
> 
> Suggested-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-20  0:08   ` [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
@ 2021-04-21 16:29     ` Olivier Matz
  2021-04-22 21:27       ` Dharmik Thakkar
  2021-04-23 10:41       ` Kinsella, Ray
  0 siblings, 2 replies; 22+ messages in thread
From: Olivier Matz @ 2021-04-21 16:29 UTC (permalink / raw)
  To: Dharmik Thakkar; +Cc: Andrew Rybchenko, dev, nd, joyce.kong, Kinsella, Ray

Hi Dharmik,

Please see some comments below.

On Mon, Apr 19, 2021 at 07:08:00PM -0500, Dharmik Thakkar wrote:
> From: Joyce Kong <joyce.kong@arm.com>
> 
> If cache is enabled, objects will be retrieved/put from/to cache,
> subsequently from/to the common pool. Now the debug stats calculate
> the objects retrieved/put from/to cache and pool together, it is
> better to distinguish them.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  lib/librte_mempool/rte_mempool.c | 24 ++++++++++++++++
>  lib/librte_mempool/rte_mempool.h | 47 ++++++++++++++++++++++----------
>  2 files changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index afb1239c8d48..339f14455624 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -1244,6 +1244,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>  		sum.put_bulk += mp->stats[lcore_id].put_bulk;
>  		sum.put_objs += mp->stats[lcore_id].put_objs;
> +		sum.put_common_pool_bulk +=
> +			mp->stats[lcore_id].put_common_pool_bulk;
> +		sum.put_common_pool_objs +=
> +			mp->stats[lcore_id].put_common_pool_objs;
> +		sum.put_cache_bulk += mp->stats[lcore_id].put_cache_bulk;
> +		sum.put_cache_objs += mp->stats[lcore_id].put_cache_objs;
> +		sum.get_common_pool_bulk +=
> +			mp->stats[lcore_id].get_common_pool_bulk;
> +		sum.get_common_pool_objs +=
> +			mp->stats[lcore_id].get_common_pool_objs;
> +		sum.get_cache_bulk += mp->stats[lcore_id].get_cache_bulk;
> +		sum.get_cache_objs += mp->stats[lcore_id].get_cache_objs;
>  		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
>  		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
>  		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
> @@ -1254,6 +1266,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	fprintf(f, "  stats:\n");
>  	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
>  	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
> +	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
> +						sum.put_common_pool_bulk);
> +	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
> +						sum.put_common_pool_objs);
> +	fprintf(f, "    put_cache_bulk=%"PRIu64"\n", sum.put_cache_bulk);
> +	fprintf(f, "    put_cache_objs=%"PRIu64"\n", sum.put_cache_objs);
> +	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
> +						sum.get_common_pool_bulk);
> +	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
> +						sum.get_common_pool_objs);
> +	fprintf(f, "    get_cache_bulk=%"PRIu64"\n", sum.get_cache_bulk);
> +	fprintf(f, "    get_cache_objs=%"PRIu64"\n", sum.get_cache_objs);
>  	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
>  	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
>  	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 848a19226149..0959f8a3f367 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -66,12 +66,20 @@ extern "C" {
>   * A structure that stores the mempool statistics (per-lcore).
>   */
>  struct rte_mempool_debug_stats {
> -	uint64_t put_bulk;         /**< Number of puts. */
> -	uint64_t put_objs;         /**< Number of objects successfully put. */
> -	uint64_t get_success_bulk; /**< Successful allocation number. */
> -	uint64_t get_success_objs; /**< Objects successfully allocated. */
> -	uint64_t get_fail_bulk;    /**< Failed allocation number. */
> -	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
> +	uint64_t put_bulk;		  /**< Number of puts. */
> +	uint64_t put_objs;		  /**< Number of objects successfully put. */
> +	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
> +	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
> +	uint64_t put_cache_bulk;	  /**< Number of bulks enqueued in cache. */
> +	uint64_t put_cache_objs;	  /**< Number of objects enqueued in cache. */
> +	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
> +	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
> +	uint64_t get_cache_bulk;	  /**< Number of bulks dequeued from cache. */
> +	uint64_t get_cache_objs;	  /**< Number of objects dequeued from cache. */
> +	uint64_t get_success_bulk;	  /**< Successful allocation number. */
> +	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
> +	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
> +	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */

I missed it the first time, but this changes the size of the
rte_mempool_debug_stats structure. I think we don't care about this ABI
breakage because this structure is only defined if
RTE_LIBRTE_MEMPOOL_DEBUG is set. But just in case, adding Ray as Cc.

About the field themselves, I'm not certain that there is an added value
to have stats for cache gets and puts. My feeling is that the important
stat to monitor is the access to common pool, because it is the one that
highlights a possible performance impact (contention). The cache stats
are more or less equal to "success + fail - common". Moreover, it will
simplify the patch and avoid risks of mistakes.

What do you think?

>  	/** Successful allocation number of contiguous blocks. */
>  	uint64_t get_success_blks;
>  	/** Failed allocation number of contiguous blocks. */
> @@ -699,10 +707,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
>  		void **obj_table, unsigned n)
>  {
>  	struct rte_mempool_ops *ops;
> +	int ret;
>  
>  	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
>  	ops = rte_mempool_get_ops(mp->ops_index);
> -	return ops->dequeue(mp, obj_table, n);
> +	ret = ops->dequeue(mp, obj_table, n);
> +	if (ret == 0) {
> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> +	}
> +	return ret;
>  }
>  
>  /**
> @@ -749,6 +765,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
>  {
>  	struct rte_mempool_ops *ops;
>  
> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
>  	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
>  	ops = rte_mempool_get_ops(mp->ops_index);
>  	return ops->enqueue(mp, obj_table, n);
> @@ -1297,14 +1315,18 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
>  
>  	/* Add elements back into the cache */
>  	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
> -
>  	cache->len += n;
>  
> +	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
> +
>  	if (cache->len >= cache->flushthresh) {
> +		__MEMPOOL_STAT_ADD(mp, put_cache_objs,
> +				   n - (cache->len - cache->size));
>  		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
>  				cache->len - cache->size);
>  		cache->len = cache->size;
> -	}
> +	} else
> +		__MEMPOOL_STAT_ADD(mp, put_cache_objs, n);
>  

In case we keep cache stats, I'd add {} after the else to be consistent
with the if().

>  	return;
>  
> @@ -1438,8 +1460,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  
>  	cache->len -= n;
>  
> -	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> -	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> +	__MEMPOOL_STAT_ADD(mp, get_cache_bulk, 1);
> +	__MEMPOOL_STAT_ADD(mp, get_cache_objs, n);

In case we keep cache stats, I don't think we should remove get_success
stats increment. Else, the success stats will never be incremented when
retrieving objects from the cache.


>  
>  	return 0;
>  
> @@ -1451,9 +1473,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  	if (ret < 0) {
>  		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
>  		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
> -	} else {
> -		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> -		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>  	}
>  
>  	return ret;
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-21 16:29     ` Olivier Matz
@ 2021-04-22 21:27       ` Dharmik Thakkar
  2021-04-22 21:47         ` Honnappa Nagarahalli
  2021-04-23 10:41       ` Kinsella, Ray
  1 sibling, 1 reply; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-22 21:27 UTC (permalink / raw)
  To: Olivier Matz
  Cc: Andrew Rybchenko, dev, nd, Joyce Kong, Kinsella, Ray,
	Honnappa Nagarahalli

Hi Olivier,

Thank you for your comments!

> On Apr 21, 2021, at 11:29 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> Hi Dharmik,
> 
> Please see some comments below.
> 
> On Mon, Apr 19, 2021 at 07:08:00PM -0500, Dharmik Thakkar wrote:
>> From: Joyce Kong <joyce.kong@arm.com>
>> 
>> If cache is enabled, objects will be retrieved/put from/to cache,
>> subsequently from/to the common pool. Now the debug stats calculate
>> the objects retrieved/put from/to cache and pool together, it is
>> better to distinguish them.
>> 
>> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> ---
>> lib/librte_mempool/rte_mempool.c | 24 ++++++++++++++++
>> lib/librte_mempool/rte_mempool.h | 47 ++++++++++++++++++++++----------
>> 2 files changed, 57 insertions(+), 14 deletions(-)
>> 
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index afb1239c8d48..339f14455624 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -1244,6 +1244,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>> 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>> 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
>> 		sum.put_objs += mp->stats[lcore_id].put_objs;
>> +		sum.put_common_pool_bulk +=
>> +			mp->stats[lcore_id].put_common_pool_bulk;
>> +		sum.put_common_pool_objs +=
>> +			mp->stats[lcore_id].put_common_pool_objs;
>> +		sum.put_cache_bulk += mp->stats[lcore_id].put_cache_bulk;
>> +		sum.put_cache_objs += mp->stats[lcore_id].put_cache_objs;
>> +		sum.get_common_pool_bulk +=
>> +			mp->stats[lcore_id].get_common_pool_bulk;
>> +		sum.get_common_pool_objs +=
>> +			mp->stats[lcore_id].get_common_pool_objs;
>> +		sum.get_cache_bulk += mp->stats[lcore_id].get_cache_bulk;
>> +		sum.get_cache_objs += mp->stats[lcore_id].get_cache_objs;
>> 		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
>> 		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
>> 		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
>> @@ -1254,6 +1266,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>> 	fprintf(f, "  stats:\n");
>> 	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
>> 	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
>> +	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
>> +						sum.put_common_pool_bulk);
>> +	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
>> +						sum.put_common_pool_objs);
>> +	fprintf(f, "    put_cache_bulk=%"PRIu64"\n", sum.put_cache_bulk);
>> +	fprintf(f, "    put_cache_objs=%"PRIu64"\n", sum.put_cache_objs);
>> +	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
>> +						sum.get_common_pool_bulk);
>> +	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
>> +						sum.get_common_pool_objs);
>> +	fprintf(f, "    get_cache_bulk=%"PRIu64"\n", sum.get_cache_bulk);
>> +	fprintf(f, "    get_cache_objs=%"PRIu64"\n", sum.get_cache_objs);
>> 	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
>> 	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
>> 	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index 848a19226149..0959f8a3f367 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -66,12 +66,20 @@ extern "C" {
>>  * A structure that stores the mempool statistics (per-lcore).
>>  */
>> struct rte_mempool_debug_stats {
>> -	uint64_t put_bulk;         /**< Number of puts. */
>> -	uint64_t put_objs;         /**< Number of objects successfully put. */
>> -	uint64_t get_success_bulk; /**< Successful allocation number. */
>> -	uint64_t get_success_objs; /**< Objects successfully allocated. */
>> -	uint64_t get_fail_bulk;    /**< Failed allocation number. */
>> -	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
>> +	uint64_t put_bulk;		  /**< Number of puts. */
>> +	uint64_t put_objs;		  /**< Number of objects successfully put. */
>> +	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
>> +	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
>> +	uint64_t put_cache_bulk;	  /**< Number of bulks enqueued in cache. */
>> +	uint64_t put_cache_objs;	  /**< Number of objects enqueued in cache. */
>> +	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
>> +	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
>> +	uint64_t get_cache_bulk;	  /**< Number of bulks dequeued from cache. */
>> +	uint64_t get_cache_objs;	  /**< Number of objects dequeued from cache. */
>> +	uint64_t get_success_bulk;	  /**< Successful allocation number. */
>> +	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
>> +	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
>> +	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
> 
> I missed it the first time, but this changes the size of the
> rte_mempool_debug_stats structure. I think we don't care about this ABI
> breakage because this structure is only defined if
> RTE_LIBRTE_MEMPOOL_DEBUG is set. But just in case, adding Ray as Cc.

Agreed, thank you!

> 
> About the field themselves, I'm not certain that there is an added value
> to have stats for cache gets and puts. My feeling is that the important
> stat to monitor is the access to common pool, because it is the one that
> highlights a possible performance impact (contention). The cache stats
> are more or less equal to "success + fail - common". Moreover, it will
> simplify the patch and avoid risks of mistakes.
> 
> What do you think?

Yes, I think the cache stats can be removed.
Also, please correct me if I’m wrong; but, in my understanding,
the cache stats are equal to “success - common”. Is adding “fail” required?

> 
>> 	/** Successful allocation number of contiguous blocks. */
>> 	uint64_t get_success_blks;
>> 	/** Failed allocation number of contiguous blocks. */
>> @@ -699,10 +707,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
>> 		void **obj_table, unsigned n)
>> {
>> 	struct rte_mempool_ops *ops;
>> +	int ret;
>> 
>> 	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
>> 	ops = rte_mempool_get_ops(mp->ops_index);
>> -	return ops->dequeue(mp, obj_table, n);
>> +	ret = ops->dequeue(mp, obj_table, n);
>> +	if (ret == 0) {
>> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
>> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
>> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
>> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>> +	}
>> +	return ret;
>> }
>> 
>> /**
>> @@ -749,6 +765,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
>> {
>> 	struct rte_mempool_ops *ops;
>> 
>> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
>> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
>> 	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
>> 	ops = rte_mempool_get_ops(mp->ops_index);
>> 	return ops->enqueue(mp, obj_table, n);
>> @@ -1297,14 +1315,18 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
>> 
>> 	/* Add elements back into the cache */
>> 	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
>> -
>> 	cache->len += n;
>> 
>> +	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
>> +
>> 	if (cache->len >= cache->flushthresh) {
>> +		__MEMPOOL_STAT_ADD(mp, put_cache_objs,
>> +				   n - (cache->len - cache->size));
>> 		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
>> 				cache->len - cache->size);
>> 		cache->len = cache->size;
>> -	}
>> +	} else
>> +		__MEMPOOL_STAT_ADD(mp, put_cache_objs, n);
>> 
> 
> In case we keep cache stats, I'd add {} after the else to be consistent
> with the if().

Ack.

> 
>> 	return;
>> 
>> @@ -1438,8 +1460,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>> 
>> 	cache->len -= n;
>> 
>> -	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
>> -	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>> +	__MEMPOOL_STAT_ADD(mp, get_cache_bulk, 1);
>> +	__MEMPOOL_STAT_ADD(mp, get_cache_objs, n);
> 
> In case we keep cache stats, I don't think we should remove get_success
> stats increment. Else, the success stats will never be incremented when
> retrieving objects from the cache.
> 

Good catch. Thanks!

> 
>> 
>> 	return 0;
>> 
>> @@ -1451,9 +1473,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>> 	if (ret < 0) {
>> 		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
>> 		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
>> -	} else {
>> -		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
>> -		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>> 	}
>> 
>> 	return ret;
>> -- 
>> 2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-22 21:27       ` Dharmik Thakkar
@ 2021-04-22 21:47         ` Honnappa Nagarahalli
  0 siblings, 0 replies; 22+ messages in thread
From: Honnappa Nagarahalli @ 2021-04-22 21:47 UTC (permalink / raw)
  To: Dharmik Thakkar, Olivier Matz
  Cc: Andrew Rybchenko, dev, nd, Joyce Kong, Kinsella, Ray,
	Honnappa Nagarahalli, nd

<snip>

> >> diff --git a/lib/librte_mempool/rte_mempool.h
> >> b/lib/librte_mempool/rte_mempool.h
> >> index 848a19226149..0959f8a3f367 100644
> >> --- a/lib/librte_mempool/rte_mempool.h
> >> +++ b/lib/librte_mempool/rte_mempool.h
> >> @@ -66,12 +66,20 @@ extern "C" {
> >>  * A structure that stores the mempool statistics (per-lcore).
> >>  */
> >> struct rte_mempool_debug_stats {
> >> -uint64_t put_bulk;         /**< Number of puts. */
> >> -uint64_t put_objs;         /**< Number of objects successfully put. */
> >> -uint64_t get_success_bulk; /**< Successful allocation number. */
> >> -uint64_t get_success_objs; /**< Objects successfully allocated. */
> >> -uint64_t get_fail_bulk;    /**< Failed allocation number. */
> >> -uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
> >> +uint64_t put_bulk;  /**< Number of puts. */ uint64_t put_objs;  /**<
> >> +Number of objects successfully put. */ uint64_t
> >> +put_common_pool_bulk;  /**< Number of bulks enqueued in common
> pool.
> >> +*/ uint64_t put_common_pool_objs;  /**< Number of objects enqueued
> >> +in common pool. */ uint64_t put_cache_bulk;  /**< Number of bulks
> >> +enqueued in cache. */ uint64_t put_cache_objs;  /**< Number of objects
> enqueued in cache. */
> >> +uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from
> common pool. */
> >> +uint64_t get_common_pool_objs;  /**< Number of objects dequeued from
> >> +common pool. */ uint64_t get_cache_bulk;  /**< Number of bulks
> >> +dequeued from cache. */ uint64_t get_cache_objs;  /**< Number of
> >> +objects dequeued from cache. */ uint64_t get_success_bulk;  /**<
> >> +Successful allocation number. */ uint64_t get_success_objs;  /**<
> >> +Objects successfully allocated. */ uint64_t get_fail_bulk;  /**<
> >> +Failed allocation number. */ uint64_t get_fail_objs;  /**< Objects
> >> +that failed to be allocated. */
> >
> > I missed it the first time, but this changes the size of the
> > rte_mempool_debug_stats structure. I think we don't care about this
> > ABI breakage because this structure is only defined if
> > RTE_LIBRTE_MEMPOOL_DEBUG is set. But just in case, adding Ray as Cc.
> 
> Agreed, thank you!
> 
> >
> > About the field themselves, I'm not certain that there is an added
> > value to have stats for cache gets and puts. My feeling is that the
> > important stat to monitor is the access to common pool, because it is
> > the one that highlights a possible performance impact (contention).
> > The cache stats are more or less equal to "success + fail - common".
> > Moreover, it will simplify the patch and avoid risks of mistakes.
> >
> > What do you think?
Agree as well. Can you please add a comment making a note of this in the stats structure?

> 
> Yes, I think the cache stats can be removed.
> Also, please correct me if I’m wrong; but, in my understanding, the cache stats
> are equal to “success - common”. Is adding “fail” required?
> 
> >
<snip>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats
  2021-04-20  0:07 ` [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats Dharmik Thakkar
  2021-04-20  0:07   ` [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
  2021-04-20  0:08   ` [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
@ 2021-04-23  1:29   ` Dharmik Thakkar
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
                       ` (3 more replies)
  2 siblings, 4 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-23  1:29 UTC (permalink / raw)
  Cc: dev, nd, joyce.kong, Dharmik Thakkar

- Add debug counters for objects put/get to/from the common pool.
- Make __MEMPOOL_STAT_ADD() more generic

---
v4:
 - Remove cache stats

v3:
 - Add a patch to make stat add macro generic
 - Remove other stat add/subtract macros
 - Rename counters for better understanding
 - Add put/get cache bulk counters

v2:
 - Fix typo in the commit message
---

Dharmik Thakkar (1):
  lib/mempool: make stats macro generic

Joyce Kong (1):
  lib/mempool: distinguish debug counters from cache and pool

 lib/mempool/rte_mempool.c | 16 ++++++++++
 lib/mempool/rte_mempool.h | 67 +++++++++++++++++++++++----------------
 2 files changed, 56 insertions(+), 27 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v4 1/2] lib/mempool: make stats macro generic
  2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
@ 2021-04-23  1:29     ` Dharmik Thakkar
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-23  1:29 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko; +Cc: dev, nd, joyce.kong, Dharmik Thakkar

Make __MEMPOOL_STAT_ADD macro more generic and delete
__MEMPOOL_CONTIG_BLOCKS_STAT_ADD macro

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/mempool/rte_mempool.h | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index c551cf733acf..848a19226149 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -273,20 +273,11 @@ struct rte_mempool {
 #define __MEMPOOL_STAT_ADD(mp, name, n) do {                    \
 		unsigned __lcore_id = rte_lcore_id();           \
 		if (__lcore_id < RTE_MAX_LCORE) {               \
-			mp->stats[__lcore_id].name##_objs += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
+			mp->stats[__lcore_id].name += n;	\
 		}                                               \
 	} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {                    \
-		unsigned int __lcore_id = rte_lcore_id();       \
-		if (__lcore_id < RTE_MAX_LCORE) {               \
-			mp->stats[__lcore_id].name##_blks += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
-		}                                               \
-	} while (0)
 #else
 #define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {} while (0)
 #endif
 
 /**
@@ -1288,7 +1279,8 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 	void **cache_objs;
 
 	/* increment stat now, adding in mempool always success */
-	__MEMPOOL_STAT_ADD(mp, put, n);
+	__MEMPOOL_STAT_ADD(mp, put_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, put_objs, n);
 
 	/* No cache provided or if put would overflow mem allocated for cache */
 	if (unlikely(cache == NULL || n > RTE_MEMPOOL_CACHE_MAX_SIZE))
@@ -1446,7 +1438,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 
 	cache->len -= n;
 
-	__MEMPOOL_STAT_ADD(mp, get_success, n);
+	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
 
 	return 0;
 
@@ -1455,10 +1448,13 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 	/* get remaining objects from ring */
 	ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n);
 
-	if (ret < 0)
-		__MEMPOOL_STAT_ADD(mp, get_fail, n);
-	else
-		__MEMPOOL_STAT_ADD(mp, get_success, n);
+	if (ret < 0) {
+		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
+	} else {
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
+	}
 
 	return ret;
 }
@@ -1581,11 +1577,13 @@ rte_mempool_get_contig_blocks(struct rte_mempool *mp,
 
 	ret = rte_mempool_ops_dequeue_contig_blocks(mp, first_obj_table, n);
 	if (ret == 0) {
-		__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, get_success, n);
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_blks, n);
 		__mempool_contig_blocks_check_cookies(mp, first_obj_table, n,
 						      1);
 	} else {
-		__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, get_fail, n);
+		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_fail_blks, n);
 	}
 
 	rte_mempool_trace_get_contig_blocks(mp, first_obj_table, n);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
@ 2021-04-23  1:29     ` Dharmik Thakkar
  2021-04-23 20:29       ` Dharmik Thakkar
  2021-04-27 12:18       ` Olivier Matz
  2021-04-27 12:28     ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Olivier Matz
  2021-04-27 16:01     ` [dpdk-dev] [PATCH v5 0/2] mempool: " Dharmik Thakkar
  3 siblings, 2 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-23  1:29 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko; +Cc: dev, nd, joyce.kong, Dharmik Thakkar

From: Joyce Kong <joyce.kong@arm.com>

If cache is enabled, objects will be retrieved/put from/to cache,
subsequently from/to the common pool. Now the debug stats calculate
the objects retrieved/put from/to cache and pool together, it is
better to distinguish them.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/mempool/rte_mempool.c | 16 +++++++++++++++
 lib/mempool/rte_mempool.h | 43 ++++++++++++++++++++++++++-------------
 2 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index afb1239c8d48..e9343c2a7f6b 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -1244,6 +1244,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
 		sum.put_objs += mp->stats[lcore_id].put_objs;
+		sum.put_common_pool_bulk +=
+			mp->stats[lcore_id].put_common_pool_bulk;
+		sum.put_common_pool_objs +=
+			mp->stats[lcore_id].put_common_pool_objs;
+		sum.get_common_pool_bulk +=
+			mp->stats[lcore_id].get_common_pool_bulk;
+		sum.get_common_pool_objs +=
+			mp->stats[lcore_id].get_common_pool_objs;
 		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
 		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
 		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
@@ -1254,6 +1262,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	fprintf(f, "  stats:\n");
 	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
 	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
+	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
+						sum.put_common_pool_bulk);
+	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
+						sum.put_common_pool_objs);
+	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
+						sum.get_common_pool_bulk);
+	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
+						sum.get_common_pool_objs);
 	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
 	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
 	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 848a19226149..4343b287dc4e 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -64,14 +64,21 @@ extern "C" {
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
 /**
  * A structure that stores the mempool statistics (per-lcore).
+ * Note: Cache stats (put_cache_bulk/objs, get_cache_bulk/objs) are not
+ * captured since they can be calculated from other stats.
+ * For example: put_cache_objs = put_objs - put_common_pool_objs.
  */
 struct rte_mempool_debug_stats {
-	uint64_t put_bulk;         /**< Number of puts. */
-	uint64_t put_objs;         /**< Number of objects successfully put. */
-	uint64_t get_success_bulk; /**< Successful allocation number. */
-	uint64_t get_success_objs; /**< Objects successfully allocated. */
-	uint64_t get_fail_bulk;    /**< Failed allocation number. */
-	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
+	uint64_t put_bulk;		  /**< Number of puts. */
+	uint64_t put_objs;		  /**< Number of objects successfully put. */
+	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
+	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
+	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
+	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
+	uint64_t get_success_bulk;	  /**< Successful allocation number. */
+	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
+	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
+	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
 	/** Successful allocation number of contiguous blocks. */
 	uint64_t get_success_blks;
 	/** Failed allocation number of contiguous blocks. */
@@ -699,10 +706,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
 		void **obj_table, unsigned n)
 {
 	struct rte_mempool_ops *ops;
+	int ret;
 
 	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
 	ops = rte_mempool_get_ops(mp->ops_index);
-	return ops->dequeue(mp, obj_table, n);
+	ret = ops->dequeue(mp, obj_table, n);
+	if (ret == 0) {
+		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
+	}
+	return ret;
 }
 
 /**
@@ -749,6 +764,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
 {
 	struct rte_mempool_ops *ops;
 
+	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
 	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
 	ops = rte_mempool_get_ops(mp->ops_index);
 	return ops->enqueue(mp, obj_table, n);
@@ -1297,9 +1314,10 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 
 	/* Add elements back into the cache */
 	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
-
 	cache->len += n;
 
+	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
+
 	if (cache->len >= cache->flushthresh) {
 		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
 				cache->len - cache->size);
@@ -1430,6 +1448,9 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 		}
 
 		cache->len += req;
+	} else {
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
 	}
 
 	/* Now fill in the response ... */
@@ -1438,9 +1459,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 
 	cache->len -= n;
 
-	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
-	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
-
 	return 0;
 
 ring_dequeue:
@@ -1451,9 +1469,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 	if (ret < 0) {
 		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
 		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
-	} else {
-		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
-		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
 	}
 
 	return ret;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-21 16:29     ` Olivier Matz
  2021-04-22 21:27       ` Dharmik Thakkar
@ 2021-04-23 10:41       ` Kinsella, Ray
  1 sibling, 0 replies; 22+ messages in thread
From: Kinsella, Ray @ 2021-04-23 10:41 UTC (permalink / raw)
  To: Olivier Matz, Dharmik Thakkar; +Cc: Andrew Rybchenko, dev, nd, joyce.kong



On 21/04/2021 17:29, Olivier Matz wrote:
> Hi Dharmik,
> 
> Please see some comments below.
> 
> On Mon, Apr 19, 2021 at 07:08:00PM -0500, Dharmik Thakkar wrote:
>> From: Joyce Kong <joyce.kong@arm.com>
>>
>> If cache is enabled, objects will be retrieved/put from/to cache,
>> subsequently from/to the common pool. Now the debug stats calculate
>> the objects retrieved/put from/to cache and pool together, it is
>> better to distinguish them.
>>
>> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> ---
>>  lib/librte_mempool/rte_mempool.c | 24 ++++++++++++++++
>>  lib/librte_mempool/rte_mempool.h | 47 ++++++++++++++++++++++----------
>>  2 files changed, 57 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index afb1239c8d48..339f14455624 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -1244,6 +1244,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>>  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>>  		sum.put_bulk += mp->stats[lcore_id].put_bulk;
>>  		sum.put_objs += mp->stats[lcore_id].put_objs;
>> +		sum.put_common_pool_bulk +=
>> +			mp->stats[lcore_id].put_common_pool_bulk;
>> +		sum.put_common_pool_objs +=
>> +			mp->stats[lcore_id].put_common_pool_objs;
>> +		sum.put_cache_bulk += mp->stats[lcore_id].put_cache_bulk;
>> +		sum.put_cache_objs += mp->stats[lcore_id].put_cache_objs;
>> +		sum.get_common_pool_bulk +=
>> +			mp->stats[lcore_id].get_common_pool_bulk;
>> +		sum.get_common_pool_objs +=
>> +			mp->stats[lcore_id].get_common_pool_objs;
>> +		sum.get_cache_bulk += mp->stats[lcore_id].get_cache_bulk;
>> +		sum.get_cache_objs += mp->stats[lcore_id].get_cache_objs;
>>  		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
>>  		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
>>  		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
>> @@ -1254,6 +1266,18 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>>  	fprintf(f, "  stats:\n");
>>  	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
>>  	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
>> +	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
>> +						sum.put_common_pool_bulk);
>> +	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
>> +						sum.put_common_pool_objs);
>> +	fprintf(f, "    put_cache_bulk=%"PRIu64"\n", sum.put_cache_bulk);
>> +	fprintf(f, "    put_cache_objs=%"PRIu64"\n", sum.put_cache_objs);
>> +	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
>> +						sum.get_common_pool_bulk);
>> +	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
>> +						sum.get_common_pool_objs);
>> +	fprintf(f, "    get_cache_bulk=%"PRIu64"\n", sum.get_cache_bulk);
>> +	fprintf(f, "    get_cache_objs=%"PRIu64"\n", sum.get_cache_objs);
>>  	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
>>  	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
>>  	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index 848a19226149..0959f8a3f367 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -66,12 +66,20 @@ extern "C" {
>>   * A structure that stores the mempool statistics (per-lcore).
>>   */
>>  struct rte_mempool_debug_stats {
>> -	uint64_t put_bulk;         /**< Number of puts. */
>> -	uint64_t put_objs;         /**< Number of objects successfully put. */
>> -	uint64_t get_success_bulk; /**< Successful allocation number. */
>> -	uint64_t get_success_objs; /**< Objects successfully allocated. */
>> -	uint64_t get_fail_bulk;    /**< Failed allocation number. */
>> -	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
>> +	uint64_t put_bulk;		  /**< Number of puts. */
>> +	uint64_t put_objs;		  /**< Number of objects successfully put. */
>> +	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
>> +	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
>> +	uint64_t put_cache_bulk;	  /**< Number of bulks enqueued in cache. */
>> +	uint64_t put_cache_objs;	  /**< Number of objects enqueued in cache. */
>> +	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
>> +	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
>> +	uint64_t get_cache_bulk;	  /**< Number of bulks dequeued from cache. */
>> +	uint64_t get_cache_objs;	  /**< Number of objects dequeued from cache. */
>> +	uint64_t get_success_bulk;	  /**< Successful allocation number. */
>> +	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
>> +	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
>> +	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
> 
> I missed it the first time, but this changes the size of the
> rte_mempool_debug_stats structure. I think we don't care about this ABI
> breakage because this structure is only defined if
> RTE_LIBRTE_MEMPOOL_DEBUG is set. But just in case, adding Ray as Cc.

Agreed, if it is just a debugging non-default feature. 

> About the field themselves, I'm not certain that there is an added value
> to have stats for cache gets and puts. My feeling is that the important
> stat to monitor is the access to common pool, because it is the one that
> highlights a possible performance impact (contention). The cache stats
> are more or less equal to "success + fail - common". Moreover, it will
> simplify the patch and avoid risks of mistakes.
> 
> What do you think?
> 
>>  	/** Successful allocation number of contiguous blocks. */
>>  	uint64_t get_success_blks;
>>  	/** Failed allocation number of contiguous blocks. */
>> @@ -699,10 +707,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
>>  		void **obj_table, unsigned n)
>>  {
>>  	struct rte_mempool_ops *ops;
>> +	int ret;
>>  
>>  	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
>>  	ops = rte_mempool_get_ops(mp->ops_index);
>> -	return ops->dequeue(mp, obj_table, n);
>> +	ret = ops->dequeue(mp, obj_table, n);
>> +	if (ret == 0) {
>> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
>> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
>> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
>> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>> +	}
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -749,6 +765,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
>>  {
>>  	struct rte_mempool_ops *ops;
>>  
>> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
>> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
>>  	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
>>  	ops = rte_mempool_get_ops(mp->ops_index);
>>  	return ops->enqueue(mp, obj_table, n);
>> @@ -1297,14 +1315,18 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
>>  
>>  	/* Add elements back into the cache */
>>  	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
>> -
>>  	cache->len += n;
>>  
>> +	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
>> +
>>  	if (cache->len >= cache->flushthresh) {
>> +		__MEMPOOL_STAT_ADD(mp, put_cache_objs,
>> +				   n - (cache->len - cache->size));
>>  		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
>>  				cache->len - cache->size);
>>  		cache->len = cache->size;
>> -	}
>> +	} else
>> +		__MEMPOOL_STAT_ADD(mp, put_cache_objs, n);
>>  
> 
> In case we keep cache stats, I'd add {} after the else to be consistent
> with the if().
> 
>>  	return;
>>  
>> @@ -1438,8 +1460,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>>  
>>  	cache->len -= n;
>>  
>> -	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
>> -	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>> +	__MEMPOOL_STAT_ADD(mp, get_cache_bulk, 1);
>> +	__MEMPOOL_STAT_ADD(mp, get_cache_objs, n);
> 
> In case we keep cache stats, I don't think we should remove get_success
> stats increment. Else, the success stats will never be incremented when
> retrieving objects from the cache.
> 
> 
>>  
>>  	return 0;
>>  
>> @@ -1451,9 +1473,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>>  	if (ret < 0) {
>>  		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
>>  		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
>> -	} else {
>> -		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
>> -		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>>  	}
>>  
>>  	return ret;
>> -- 
>> 2.17.1
>>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
@ 2021-04-23 20:29       ` Dharmik Thakkar
  2021-04-27 12:18       ` Olivier Matz
  1 sibling, 0 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-23 20:29 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko; +Cc: dev, nd, Joyce Kong, Honnappa Nagarahalli

+Honnappa

> On Apr 22, 2021, at 8:29 PM, Dharmik Thakkar <Dharmik.Thakkar@arm.com> wrote:
> 
> From: Joyce Kong <joyce.kong@arm.com>
> 
> If cache is enabled, objects will be retrieved/put from/to cache,
> subsequently from/to the common pool. Now the debug stats calculate
> the objects retrieved/put from/to cache and pool together, it is
> better to distinguish them.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> lib/mempool/rte_mempool.c | 16 +++++++++++++++
> lib/mempool/rte_mempool.h | 43 ++++++++++++++++++++++++++-------------
> 2 files changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index afb1239c8d48..e9343c2a7f6b 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -1244,6 +1244,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
> 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
> 		sum.put_objs += mp->stats[lcore_id].put_objs;
> +		sum.put_common_pool_bulk +=
> +			mp->stats[lcore_id].put_common_pool_bulk;
> +		sum.put_common_pool_objs +=
> +			mp->stats[lcore_id].put_common_pool_objs;
> +		sum.get_common_pool_bulk +=
> +			mp->stats[lcore_id].get_common_pool_bulk;
> +		sum.get_common_pool_objs +=
> +			mp->stats[lcore_id].get_common_pool_objs;
> 		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
> 		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
> 		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
> @@ -1254,6 +1262,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
> 	fprintf(f, "  stats:\n");
> 	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
> 	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
> +	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
> +						sum.put_common_pool_bulk);
> +	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
> +						sum.put_common_pool_objs);
> +	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
> +						sum.get_common_pool_bulk);
> +	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
> +						sum.get_common_pool_objs);
> 	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
> 	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
> 	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 848a19226149..4343b287dc4e 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -64,14 +64,21 @@ extern "C" {
> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> /**
>  * A structure that stores the mempool statistics (per-lcore).
> + * Note: Cache stats (put_cache_bulk/objs, get_cache_bulk/objs) are not
> + * captured since they can be calculated from other stats.
> + * For example: put_cache_objs = put_objs - put_common_pool_objs.
>  */
> struct rte_mempool_debug_stats {
> -	uint64_t put_bulk;         /**< Number of puts. */
> -	uint64_t put_objs;         /**< Number of objects successfully put. */
> -	uint64_t get_success_bulk; /**< Successful allocation number. */
> -	uint64_t get_success_objs; /**< Objects successfully allocated. */
> -	uint64_t get_fail_bulk;    /**< Failed allocation number. */
> -	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
> +	uint64_t put_bulk;		  /**< Number of puts. */
> +	uint64_t put_objs;		  /**< Number of objects successfully put. */
> +	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
> +	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
> +	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
> +	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
> +	uint64_t get_success_bulk;	  /**< Successful allocation number. */
> +	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
> +	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
> +	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
> 	/** Successful allocation number of contiguous blocks. */
> 	uint64_t get_success_blks;
> 	/** Failed allocation number of contiguous blocks. */
> @@ -699,10 +706,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
> 		void **obj_table, unsigned n)
> {
> 	struct rte_mempool_ops *ops;
> +	int ret;
> 
> 	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
> 	ops = rte_mempool_get_ops(mp->ops_index);
> -	return ops->dequeue(mp, obj_table, n);
> +	ret = ops->dequeue(mp, obj_table, n);
> +	if (ret == 0) {
> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> +	}
> +	return ret;
> }
> 
> /**
> @@ -749,6 +764,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
> {
> 	struct rte_mempool_ops *ops;
> 
> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
> 	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
> 	ops = rte_mempool_get_ops(mp->ops_index);
> 	return ops->enqueue(mp, obj_table, n);
> @@ -1297,9 +1314,10 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
> 
> 	/* Add elements back into the cache */
> 	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
> -
> 	cache->len += n;
> 
> +	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
> +
> 	if (cache->len >= cache->flushthresh) {
> 		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
> 				cache->len - cache->size);
> @@ -1430,6 +1448,9 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
> 		}
> 
> 		cache->len += req;
> +	} else {
> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> 	}
> 
> 	/* Now fill in the response ... */
> @@ -1438,9 +1459,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
> 
> 	cache->len -= n;
> 
> -	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> -	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> -
> 	return 0;
> 
> ring_dequeue:
> @@ -1451,9 +1469,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
> 	if (ret < 0) {
> 		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
> 		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
> -	} else {
> -		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> -		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> 	}
> 
> 	return ret;
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
  2021-04-23 20:29       ` Dharmik Thakkar
@ 2021-04-27 12:18       ` Olivier Matz
  1 sibling, 0 replies; 22+ messages in thread
From: Olivier Matz @ 2021-04-27 12:18 UTC (permalink / raw)
  To: Dharmik Thakkar; +Cc: Andrew Rybchenko, dev, nd, joyce.kong

Hi Dharmik,

Few comments below.

On Thu, Apr 22, 2021 at 08:29:38PM -0500, Dharmik Thakkar wrote:
> From: Joyce Kong <joyce.kong@arm.com>
> 
> If cache is enabled, objects will be retrieved/put from/to cache,
> subsequently from/to the common pool. Now the debug stats calculate
> the objects retrieved/put from/to cache and pool together, it is
> better to distinguish them.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  lib/mempool/rte_mempool.c | 16 +++++++++++++++
>  lib/mempool/rte_mempool.h | 43 ++++++++++++++++++++++++++-------------
>  2 files changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index afb1239c8d48..e9343c2a7f6b 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -1244,6 +1244,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>  		sum.put_bulk += mp->stats[lcore_id].put_bulk;
>  		sum.put_objs += mp->stats[lcore_id].put_objs;
> +		sum.put_common_pool_bulk +=
> +			mp->stats[lcore_id].put_common_pool_bulk;
> +		sum.put_common_pool_objs +=
> +			mp->stats[lcore_id].put_common_pool_objs;
> +		sum.get_common_pool_bulk +=
> +			mp->stats[lcore_id].get_common_pool_bulk;
> +		sum.get_common_pool_objs +=
> +			mp->stats[lcore_id].get_common_pool_objs;
>  		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
>  		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
>  		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
> @@ -1254,6 +1262,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	fprintf(f, "  stats:\n");
>  	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
>  	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
> +	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
> +						sum.put_common_pool_bulk);
> +	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
> +						sum.put_common_pool_objs);
> +	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
> +						sum.get_common_pool_bulk);
> +	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
> +						sum.get_common_pool_objs);
>  	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
>  	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
>  	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 848a19226149..4343b287dc4e 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -64,14 +64,21 @@ extern "C" {
>  #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>  /**
>   * A structure that stores the mempool statistics (per-lcore).
> + * Note: Cache stats (put_cache_bulk/objs, get_cache_bulk/objs) are not
> + * captured since they can be calculated from other stats.
> + * For example: put_cache_objs = put_objs - put_common_pool_objs.
>   */
>  struct rte_mempool_debug_stats {
> -	uint64_t put_bulk;         /**< Number of puts. */
> -	uint64_t put_objs;         /**< Number of objects successfully put. */
> -	uint64_t get_success_bulk; /**< Successful allocation number. */
> -	uint64_t get_success_objs; /**< Objects successfully allocated. */
> -	uint64_t get_fail_bulk;    /**< Failed allocation number. */
> -	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
> +	uint64_t put_bulk;		  /**< Number of puts. */
> +	uint64_t put_objs;		  /**< Number of objects successfully put. */
> +	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
> +	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
> +	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
> +	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
> +	uint64_t get_success_bulk;	  /**< Successful allocation number. */
> +	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
> +	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
> +	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
>  	/** Successful allocation number of contiguous blocks. */
>  	uint64_t get_success_blks;
>  	/** Failed allocation number of contiguous blocks. */
> @@ -699,10 +706,18 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
>  		void **obj_table, unsigned n)
>  {
>  	struct rte_mempool_ops *ops;
> +	int ret;
>  
>  	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
>  	ops = rte_mempool_get_ops(mp->ops_index);
> -	return ops->dequeue(mp, obj_table, n);
> +	ret = ops->dequeue(mp, obj_table, n);
> +	if (ret == 0) {
> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> +	}
> +	return ret;
>  }

I think we should only have the common_pool stats here, for 2 reasons:

- more consistent with put()
- in case we are called by __mempool_generic_get() for a "backfill"
  operation, the number of successes will not be incremented by the
  correct value (the "req" variable is != n)

>  
>  /**
> @@ -749,6 +764,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
>  {
>  	struct rte_mempool_ops *ops;
>  
> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
> +	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
>  	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
>  	ops = rte_mempool_get_ops(mp->ops_index);
>  	return ops->enqueue(mp, obj_table, n);
> @@ -1297,9 +1314,10 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
>  
>  	/* Add elements back into the cache */
>  	rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n);
> -
>  	cache->len += n;
>  
> +	__MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
> +

This one was forgotten, there is a compilation error:

In file included from ../lib/mempool/rte_mempool_ops_default.c:7:
../lib/mempool/rte_mempool.h: In function ‘__mempool_generic_put’:
../lib/mempool/rte_mempool.h:1319:25: error: ‘struct rte_mempool_debug_stats’ has no member named ‘put_cache_bulk’; did you mean ‘put_bulk’?
  __MEMPOOL_STAT_ADD(mp, put_cache_bulk, 1);
                         ^~~~~~~~~~~~~~
../lib/mempool/rte_mempool.h:283:26: note: in definition of macro ‘__MEMPOOL_STAT_ADD’
    mp->stats[__lcore_id].name += n; \
                          ^~~~



>  	if (cache->len >= cache->flushthresh) {
>  		rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size],
>  				cache->len - cache->size);
> @@ -1430,6 +1448,9 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  		}
>  
>  		cache->len += req;
> +	} else {
> +		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> +		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>  	}
>  
>  	/* Now fill in the response ... */
> @@ -1438,9 +1459,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  
>  	cache->len -= n;
>  
> -	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> -	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
> -
>  	return 0;
>  
>  ring_dequeue:
> @@ -1451,9 +1469,6 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
>  	if (ret < 0) {
>  		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
>  		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
> -	} else {
> -		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
> -		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
>  	}
>  
>  	return ret;
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats
  2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
  2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
@ 2021-04-27 12:28     ` Olivier Matz
  2021-04-27 16:01     ` [dpdk-dev] [PATCH v5 0/2] mempool: " Dharmik Thakkar
  3 siblings, 0 replies; 22+ messages in thread
From: Olivier Matz @ 2021-04-27 12:28 UTC (permalink / raw)
  To: Dharmik Thakkar; +Cc: dev, nd, joyce.kong

On Thu, Apr 22, 2021 at 08:29:36PM -0500, Dharmik Thakkar wrote:
> Subject: [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats

I missed that one: please use "mempool:" instead of "lib/mempool:"
prefix in the titles.

Thanks,
Olivier


>
> - Add debug counters for objects put/get to/from the common pool.
> - Make __MEMPOOL_STAT_ADD() more generic
> 
> ---
> v4:
>  - Remove cache stats
> 
> v3:
>  - Add a patch to make stat add macro generic
>  - Remove other stat add/subtract macros
>  - Rename counters for better understanding
>  - Add put/get cache bulk counters
> 
> v2:
>  - Fix typo in the commit message
> ---
> 
> Dharmik Thakkar (1):
>   lib/mempool: make stats macro generic
> 
> Joyce Kong (1):
>   lib/mempool: distinguish debug counters from cache and pool
> 
>  lib/mempool/rte_mempool.c | 16 ++++++++++
>  lib/mempool/rte_mempool.h | 67 +++++++++++++++++++++++----------------
>  2 files changed, 56 insertions(+), 27 deletions(-)
> 
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v5 0/2] mempool: add debug stats
  2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
                       ` (2 preceding siblings ...)
  2021-04-27 12:28     ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Olivier Matz
@ 2021-04-27 16:01     ` Dharmik Thakkar
  2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 1/2] mempool: make stats macro generic Dharmik Thakkar
                         ` (2 more replies)
  3 siblings, 3 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-27 16:01 UTC (permalink / raw)
  Cc: dev, nd, joyce.kong, honnappa.nagarahalli, Dharmik Thakkar

- Add debug counters for objects put/get to/from the common pool.
- Make __MEMPOOL_STAT_ADD() more generic

---
v5:
 - Fix compilation issue
 - Move 'increment get_success_bulk/objs' inside __mempool_generic_get

v4:
 - Remove cache stats

v3:
 - Add a patch to make stat add macro generic
 - Remove other stat add/subtract macros
 - Rename counters for better understanding
 - Add put/get cache bulk counters

v2:
 - Fix typo in the commit message
---

Dharmik Thakkar (1):
  mempool: make stats macro generic

Joyce Kong (1):
  mempool: distinguish debug counters from cache and pool

 lib/mempool/rte_mempool.c | 16 ++++++++++
 lib/mempool/rte_mempool.h | 63 +++++++++++++++++++++++----------------
 2 files changed, 54 insertions(+), 25 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v5 1/2] mempool: make stats macro generic
  2021-04-27 16:01     ` [dpdk-dev] [PATCH v5 0/2] mempool: " Dharmik Thakkar
@ 2021-04-27 16:01       ` Dharmik Thakkar
  2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 2/2] mempool: distinguish debug counters from cache and pool Dharmik Thakkar
  2021-05-04  7:02       ` [dpdk-dev] [PATCH v5 0/2] mempool: add debug stats David Marchand
  2 siblings, 0 replies; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-27 16:01 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko
  Cc: dev, nd, joyce.kong, honnappa.nagarahalli, Dharmik Thakkar

Make __MEMPOOL_STAT_ADD macro more generic and delete
__MEMPOOL_CONTIG_BLOCKS_STAT_ADD macro

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/mempool/rte_mempool.h | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index c551cf733acf..848a19226149 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -273,20 +273,11 @@ struct rte_mempool {
 #define __MEMPOOL_STAT_ADD(mp, name, n) do {                    \
 		unsigned __lcore_id = rte_lcore_id();           \
 		if (__lcore_id < RTE_MAX_LCORE) {               \
-			mp->stats[__lcore_id].name##_objs += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
+			mp->stats[__lcore_id].name += n;	\
 		}                                               \
 	} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {                    \
-		unsigned int __lcore_id = rte_lcore_id();       \
-		if (__lcore_id < RTE_MAX_LCORE) {               \
-			mp->stats[__lcore_id].name##_blks += n;	\
-			mp->stats[__lcore_id].name##_bulk += 1;	\
-		}                                               \
-	} while (0)
 #else
 #define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
-#define __MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, name, n) do {} while (0)
 #endif
 
 /**
@@ -1288,7 +1279,8 @@ __mempool_generic_put(struct rte_mempool *mp, void * const *obj_table,
 	void **cache_objs;
 
 	/* increment stat now, adding in mempool always success */
-	__MEMPOOL_STAT_ADD(mp, put, n);
+	__MEMPOOL_STAT_ADD(mp, put_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, put_objs, n);
 
 	/* No cache provided or if put would overflow mem allocated for cache */
 	if (unlikely(cache == NULL || n > RTE_MEMPOOL_CACHE_MAX_SIZE))
@@ -1446,7 +1438,8 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 
 	cache->len -= n;
 
-	__MEMPOOL_STAT_ADD(mp, get_success, n);
+	__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
 
 	return 0;
 
@@ -1455,10 +1448,13 @@ __mempool_generic_get(struct rte_mempool *mp, void **obj_table,
 	/* get remaining objects from ring */
 	ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n);
 
-	if (ret < 0)
-		__MEMPOOL_STAT_ADD(mp, get_fail, n);
-	else
-		__MEMPOOL_STAT_ADD(mp, get_success, n);
+	if (ret < 0) {
+		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_fail_objs, n);
+	} else {
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_objs, n);
+	}
 
 	return ret;
 }
@@ -1581,11 +1577,13 @@ rte_mempool_get_contig_blocks(struct rte_mempool *mp,
 
 	ret = rte_mempool_ops_dequeue_contig_blocks(mp, first_obj_table, n);
 	if (ret == 0) {
-		__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, get_success, n);
+		__MEMPOOL_STAT_ADD(mp, get_success_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_success_blks, n);
 		__mempool_contig_blocks_check_cookies(mp, first_obj_table, n,
 						      1);
 	} else {
-		__MEMPOOL_CONTIG_BLOCKS_STAT_ADD(mp, get_fail, n);
+		__MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_fail_blks, n);
 	}
 
 	rte_mempool_trace_get_contig_blocks(mp, first_obj_table, n);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [dpdk-dev] [PATCH v5 2/2] mempool: distinguish debug counters from cache and pool
  2021-04-27 16:01     ` [dpdk-dev] [PATCH v5 0/2] mempool: " Dharmik Thakkar
  2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 1/2] mempool: make stats macro generic Dharmik Thakkar
@ 2021-04-27 16:01       ` Dharmik Thakkar
  2021-05-04  6:54         ` Olivier Matz
  2021-05-04  7:02       ` [dpdk-dev] [PATCH v5 0/2] mempool: add debug stats David Marchand
  2 siblings, 1 reply; 22+ messages in thread
From: Dharmik Thakkar @ 2021-04-27 16:01 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko
  Cc: dev, nd, joyce.kong, honnappa.nagarahalli, Dharmik Thakkar

From: Joyce Kong <joyce.kong@arm.com>

If cache is enabled, objects will be retrieved/put from/to cache,
subsequently from/to the common pool. Now the debug stats calculate
the objects retrieved/put from/to cache and pool together, it is
better to distinguish them.

Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 lib/mempool/rte_mempool.c | 16 ++++++++++++++++
 lib/mempool/rte_mempool.h | 29 ++++++++++++++++++++++-------
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index afb1239c8d48..e9343c2a7f6b 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -1244,6 +1244,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
 		sum.put_objs += mp->stats[lcore_id].put_objs;
+		sum.put_common_pool_bulk +=
+			mp->stats[lcore_id].put_common_pool_bulk;
+		sum.put_common_pool_objs +=
+			mp->stats[lcore_id].put_common_pool_objs;
+		sum.get_common_pool_bulk +=
+			mp->stats[lcore_id].get_common_pool_bulk;
+		sum.get_common_pool_objs +=
+			mp->stats[lcore_id].get_common_pool_objs;
 		sum.get_success_bulk += mp->stats[lcore_id].get_success_bulk;
 		sum.get_success_objs += mp->stats[lcore_id].get_success_objs;
 		sum.get_fail_bulk += mp->stats[lcore_id].get_fail_bulk;
@@ -1254,6 +1262,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	fprintf(f, "  stats:\n");
 	fprintf(f, "    put_bulk=%"PRIu64"\n", sum.put_bulk);
 	fprintf(f, "    put_objs=%"PRIu64"\n", sum.put_objs);
+	fprintf(f, "    put_common_pool_bulk=%"PRIu64"\n",
+						sum.put_common_pool_bulk);
+	fprintf(f, "    put_common_pool_objs=%"PRIu64"\n",
+						sum.put_common_pool_objs);
+	fprintf(f, "    get_common_pool_bulk=%"PRIu64"\n",
+						sum.get_common_pool_bulk);
+	fprintf(f, "    get_common_pool_objs=%"PRIu64"\n",
+						sum.get_common_pool_objs);
 	fprintf(f, "    get_success_bulk=%"PRIu64"\n", sum.get_success_bulk);
 	fprintf(f, "    get_success_objs=%"PRIu64"\n", sum.get_success_objs);
 	fprintf(f, "    get_fail_bulk=%"PRIu64"\n", sum.get_fail_bulk);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 848a19226149..7089baab31e3 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -64,14 +64,21 @@ extern "C" {
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
 /**
  * A structure that stores the mempool statistics (per-lcore).
+ * Note: Cache stats (put_cache_bulk/objs, get_cache_bulk/objs) are not
+ * captured since they can be calculated from other stats.
+ * For example: put_cache_objs = put_objs - put_common_pool_objs.
  */
 struct rte_mempool_debug_stats {
-	uint64_t put_bulk;         /**< Number of puts. */
-	uint64_t put_objs;         /**< Number of objects successfully put. */
-	uint64_t get_success_bulk; /**< Successful allocation number. */
-	uint64_t get_success_objs; /**< Objects successfully allocated. */
-	uint64_t get_fail_bulk;    /**< Failed allocation number. */
-	uint64_t get_fail_objs;    /**< Objects that failed to be allocated. */
+	uint64_t put_bulk;		  /**< Number of puts. */
+	uint64_t put_objs;		  /**< Number of objects successfully put. */
+	uint64_t put_common_pool_bulk;	  /**< Number of bulks enqueued in common pool. */
+	uint64_t put_common_pool_objs;	  /**< Number of objects enqueued in common pool. */
+	uint64_t get_common_pool_bulk;    /**< Number of bulks dequeued from common pool. */
+	uint64_t get_common_pool_objs;	  /**< Number of objects dequeued from common pool. */
+	uint64_t get_success_bulk;	  /**< Successful allocation number. */
+	uint64_t get_success_objs;	  /**< Objects successfully allocated. */
+	uint64_t get_fail_bulk;		  /**< Failed allocation number. */
+	uint64_t get_fail_objs;		  /**< Objects that failed to be allocated. */
 	/** Successful allocation number of contiguous blocks. */
 	uint64_t get_success_blks;
 	/** Failed allocation number of contiguous blocks. */
@@ -699,10 +706,16 @@ rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
 		void **obj_table, unsigned n)
 {
 	struct rte_mempool_ops *ops;
+	int ret;
 
 	rte_mempool_trace_ops_dequeue_bulk(mp, obj_table, n);
 	ops = rte_mempool_get_ops(mp->ops_index);
-	return ops->dequeue(mp, obj_table, n);
+	ret = ops->dequeue(mp, obj_table, n);
+	if (ret == 0) {
+		__MEMPOOL_STAT_ADD(mp, get_common_pool_bulk, 1);
+		__MEMPOOL_STAT_ADD(mp, get_common_pool_objs, n);
+	}
+	return ret;
 }
 
 /**
@@ -749,6 +762,8 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
 {
 	struct rte_mempool_ops *ops;
 
+	__MEMPOOL_STAT_ADD(mp, put_common_pool_bulk, 1);
+	__MEMPOOL_STAT_ADD(mp, put_common_pool_objs, n);
 	rte_mempool_trace_ops_enqueue_bulk(mp, obj_table, n);
 	ops = rte_mempool_get_ops(mp->ops_index);
 	return ops->enqueue(mp, obj_table, n);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v5 2/2] mempool: distinguish debug counters from cache and pool
  2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 2/2] mempool: distinguish debug counters from cache and pool Dharmik Thakkar
@ 2021-05-04  6:54         ` Olivier Matz
  0 siblings, 0 replies; 22+ messages in thread
From: Olivier Matz @ 2021-05-04  6:54 UTC (permalink / raw)
  To: Dharmik Thakkar
  Cc: Andrew Rybchenko, dev, nd, joyce.kong, honnappa.nagarahalli

On Tue, Apr 27, 2021 at 11:01:40AM -0500, Dharmik Thakkar wrote:
> From: Joyce Kong <joyce.kong@arm.com>
> 
> If cache is enabled, objects will be retrieved/put from/to cache,
> subsequently from/to the common pool. Now the debug stats calculate
> the objects retrieved/put from/to cache and pool together, it is
> better to distinguish them.
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dpdk-dev] [PATCH v5 0/2] mempool: add debug stats
  2021-04-27 16:01     ` [dpdk-dev] [PATCH v5 0/2] mempool: " Dharmik Thakkar
  2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 1/2] mempool: make stats macro generic Dharmik Thakkar
  2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 2/2] mempool: distinguish debug counters from cache and pool Dharmik Thakkar
@ 2021-05-04  7:02       ` David Marchand
  2 siblings, 0 replies; 22+ messages in thread
From: David Marchand @ 2021-05-04  7:02 UTC (permalink / raw)
  To: Dharmik Thakkar; +Cc: dev, nd, Joyce Kong, Honnappa Nagarahalli, Olivier Matz

On Tue, Apr 27, 2021 at 6:02 PM Dharmik Thakkar <dharmik.thakkar@arm.com> wrote:
>
> - Add debug counters for objects put/get to/from the common pool.
> - Make __MEMPOOL_STAT_ADD() more generic
>

Series applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-05-04  7:02 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 11:20 [dpdk-dev] [PATCH v2] lib/mempool: distinguish debug counters from cache and pool Joyce Kong
2021-04-07 14:28 ` Olivier Matz
2021-04-20  0:31   ` Dharmik Thakkar
2021-04-20  0:07 ` [dpdk-dev] [PATCH v3 0/2] lib/mempool: add debug stats Dharmik Thakkar
2021-04-20  0:07   ` [dpdk-dev] [PATCH v3 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
2021-04-21 16:09     ` Olivier Matz
2021-04-20  0:08   ` [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
2021-04-21 16:29     ` Olivier Matz
2021-04-22 21:27       ` Dharmik Thakkar
2021-04-22 21:47         ` Honnappa Nagarahalli
2021-04-23 10:41       ` Kinsella, Ray
2021-04-23  1:29   ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Dharmik Thakkar
2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 1/2] lib/mempool: make stats macro generic Dharmik Thakkar
2021-04-23  1:29     ` [dpdk-dev] [PATCH v4 2/2] lib/mempool: distinguish debug counters from cache and pool Dharmik Thakkar
2021-04-23 20:29       ` Dharmik Thakkar
2021-04-27 12:18       ` Olivier Matz
2021-04-27 12:28     ` [dpdk-dev] [PATCH v4 0/2] lib/mempool: add debug stats Olivier Matz
2021-04-27 16:01     ` [dpdk-dev] [PATCH v5 0/2] mempool: " Dharmik Thakkar
2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 1/2] mempool: make stats macro generic Dharmik Thakkar
2021-04-27 16:01       ` [dpdk-dev] [PATCH v5 2/2] mempool: distinguish debug counters from cache and pool Dharmik Thakkar
2021-05-04  6:54         ` Olivier Matz
2021-05-04  7:02       ` [dpdk-dev] [PATCH v5 0/2] mempool: add debug stats David Marchand

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.