Re: [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

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