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 632BEA0547; Wed, 21 Apr 2021 18:29:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 275B2410F9; Wed, 21 Apr 2021 18:29:42 +0200 (CEST) Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com [209.85.221.54]) by mails.dpdk.org (Postfix) with ESMTP id 0ADCE4068A for ; Wed, 21 Apr 2021 18:29:40 +0200 (CEST) Received: by mail-wr1-f54.google.com with SMTP id s7so41913393wru.6 for ; Wed, 21 Apr 2021 09:29:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=TWiYDu/xWdgP1EJCqjtEhChiTmHWjJHLELkVg5k/AcA=; b=gzm56Kk2E5ycfaPe2lugg2vpJOerE39FjZUp2QbaYZZ2YQ4tj3whObJhyik9Ph9OHV 17L7dSA140IlW5GxfRPKPXMKSHtnKMvINJQmSHNYN2dZkuY7RDUvRS3uU42FmYTRdLR5 baaHyi+8LITLGLRXHlAwWnH8Z4Io9sYBM/g9mnGGvR/l5yXpW6nQDYTpHF+qx0yMHO0/ kEQZHNGImNiYRdjQHZ1yEczNSHQL3ytuyuOkVAFYDM75pLWfyvYwZr1orwQxGQN8SBgE QLMwYKLC9OHvQgoguI42+Qbp6GeLuPYur/m78hLXsk86a5w9ZOYa05R2Cwf9a1afkMh5 pTxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=TWiYDu/xWdgP1EJCqjtEhChiTmHWjJHLELkVg5k/AcA=; b=MrRGdTBU7/aQlHImqPlzOGHOGGen63LGSODFJ0/p7c1pKWTFPKxI89kzvn/7QV5Zrx wYMorRuLxKcXOuU/9aiQn0a5FbF/vZe5UNMLVvCC3FpGziWQy2jTMnQJ2Fr5rspbUGLa 4FSpTuZ/VbMbhOk2gpjd8rodzbvAS1wtEyf5UnDUqLOW+CS0nl8d7vl4f1jzS6097II4 RvewvXxh+NvYeEJLbG9bqGtJGYkhNYFJo+LL0RxEJApnPRDKfoNjuY9hFeYXbk9+5JQ1 CQQjkvkYeBHVGAZbe7bdIw77ek0fjnH8FuiplpNW4MV/0mV/hUOKxi7uRpUK2grCtTGv LWmg== X-Gm-Message-State: AOAM5303Q+sxd75PVYPcGG8hqsLcQjRMSxMkrLnLxfm3J/jpwddQeW3c 1D39siChprXzVO1pxP6S6/aQdmeQ36sWuA== X-Google-Smtp-Source: ABdhPJx57nmTSeZ4RUNCYpNfA1OvUDSv56UsevMbYOzBp/TnQG/yRkJVZpdlE2yoheZ5EQIYodC8lQ== X-Received: by 2002:a5d:654c:: with SMTP id z12mr28151122wrv.12.1619022579692; Wed, 21 Apr 2021 09:29:39 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id o17sm3602004wrv.16.2021.04.21.09.29.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Apr 2021 09:29:39 -0700 (PDT) Date: Wed, 21 Apr 2021 18:29:38 +0200 From: Olivier Matz To: Dharmik Thakkar Cc: Andrew Rybchenko , dev@dpdk.org, nd@arm.com, joyce.kong@arm.com, "Kinsella, Ray" Message-ID: <20210421162938.GG1726@platinum> References: <20210318112022.10510-1-joyce.kong@arm.com> <20210420000800.1504-1-dharmik.thakkar@arm.com> <20210420000800.1504-3-dharmik.thakkar@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210420000800.1504-3-dharmik.thakkar@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) 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" 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. 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 >