From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 037E8A0548; Fri, 23 Apr 2021 12:41:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6C44F410D8; Fri, 23 Apr 2021 12:41:50 +0200 (CEST) Received: from mail-108-mta238.mxroute.com (mail-108-mta238.mxroute.com [136.175.108.238]) by mails.dpdk.org (Postfix) with ESMTP id 34E3C4014F for ; Fri, 23 Apr 2021 12:41:49 +0200 (CEST) Received: from filter004.mxroute.com ([149.28.56.236] filter004.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta238.mxroute.com (ZoneMTA) with ESMTPSA id 178fe5233a00004964.005 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Fri, 23 Apr 2021 10:41:43 +0000 X-Zone-Loop: 14213cba4fcd149c800f023c52f7e3f32f2275eef76e X-Originating-IP: [149.28.56.236] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=ashroe.eu; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date: Message-ID:From:References:Cc:To:Subject:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=s5MWjD9i+IDa39jC/ToEmMRXAvyUlaxrYnQ4mS2tx/U=; b=LuWVHl+6KC1Gh2Kj41ixrhvRu8 APP3m+K1/BBUi852qdBuyFZBihSLM0Tmpwhc9dAdrEQSdaDXZ7uEpWhXG0Ro63Cf2IIhJbAGqouLV HS/LGQQTrzkvwWn6i+BvllV/qeBtpvxtSboaFUKvr60qXVn7qlPGdvZzQNtoOog2tIi+3jC+x8CU5 bLnL1Q0ljeXcgoMIOu8a21/u4thvtLY+33Ltau5F2AaIVW5DzMj1rVdKUN92mrVgazasdlEW2nEm4 mUq8pOKRYLZqf5cDCR0b/nk7a2SHRoMwlQ6qBIHRzhzXtdDesUdn1oQXF5TnBRMEUKW5uiuPSQkXU GnI6OWLQ==; To: Olivier Matz , Dharmik Thakkar Cc: Andrew Rybchenko , dev@dpdk.org, nd@arm.com, joyce.kong@arm.com References: <20210318112022.10510-1-joyce.kong@arm.com> <20210420000800.1504-1-dharmik.thakkar@arm.com> <20210420000800.1504-3-dharmik.thakkar@arm.com> <20210421162938.GG1726@platinum> From: "Kinsella, Ray" Message-ID: Date: Fri, 23 Apr 2021 11:41:39 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <20210421162938.GG1726@platinum> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-AuthUser: mdr@ashroe.eu Subject: Re: [dpdk-dev] [PATCH v3 2/2] lib/mempool: distinguish debug counters from cache and pool X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> >> 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 >> Signed-off-by: Dharmik Thakkar >> Reviewed-by: Ruifeng Wang >> Reviewed-by: Honnappa Nagarahalli >> --- >> 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 >>