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 8244846091; Wed, 15 Jan 2025 13:14:37 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 709F34025F; Wed, 15 Jan 2025 13:14:37 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 72848400D7 for ; Wed, 15 Jan 2025 13:14:35 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.174]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4YY4bc6BtSzbk0w; Wed, 15 Jan 2025 20:11:24 +0800 (CST) Received: from kwepemo500011.china.huawei.com (unknown [7.202.195.194]) by mail.maildlp.com (Postfix) with ESMTPS id 37D2F1400E3; Wed, 15 Jan 2025 20:14:33 +0800 (CST) Received: from [10.67.121.193] (10.67.121.193) by kwepemo500011.china.huawei.com (7.202.195.194) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Wed, 15 Jan 2025 20:14:32 +0800 Message-ID: Date: Wed, 15 Jan 2025 20:14:31 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] mbuf: add fast free bulk function To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , CC: Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , Slava Ovsiienko , Shahaf Shuler , Olivier Matz References: <20250114162544.125448-1-mb@smartsharesystems.com> <20250114163951.125667-1-mb@smartsharesystems.com> <02835d2e-6acb-45df-8897-4590360be861@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35E9F9B3@smartserver.smartshare.dk> Content-Language: en-US From: huangdengdui In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F9B3@smartserver.smartshare.dk> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.193] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemo500011.china.huawei.com (7.202.195.194) 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 On 2025/1/15 17:38, Morten Brørup wrote: >> From: huangdengdui [mailto:huangdengdui@huawei.com] >> Sent: Wednesday, 15 January 2025 07.52 >> >> On 2025/1/15 0:39, Morten Brørup wrote: >>> mbuf: add fast free bulk function >>> >>> When putting an mbuf back into its mempool, there are certain >> requirements >>> to the mbuf. Specifically, some of its fields must be initialized. >>> >>> These requirements are in fact invariants about free mbufs, held in >>> mempools, and thus also apply when allocating an mbuf from a mempool. >>> With this in mind, the additional assertions in rte_mbuf_raw_free() >> were >>> moved to __rte_mbuf_raw_sanity_check(). >>> Furthermore, the assertion regarding pinned external buffer was >> enhanced; >>> it now also asserts that the referenced pinned external buffer has >>> refcnt == 1. >>> >>> The description of RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE was updated to >>> include the remaining requirements, which were missing here. >>> >>> And finally: >>> A new rte_mbuf_fast_free_bulk() inline function was added for the >> benefit >>> of ethdev drivers supporting fast release of mbufs. >>> It asserts these requirements and that the mbufs belong to the >> specified >>> mempool, and then calls rte_mempool_put_bulk(). >>> >>> Signed-off-by: Morten Brørup >>> --- >>> v2: >>> * Fixed missing inline. >>> --- >>> lib/ethdev/rte_ethdev.h | 6 ++++-- >>> lib/mbuf/rte_mbuf.h | 39 +++++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 41 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >>> index 1f71cad244..e9267fca79 100644 >>> --- a/lib/ethdev/rte_ethdev.h >>> +++ b/lib/ethdev/rte_ethdev.h >>> @@ -1612,8 +1612,10 @@ struct rte_eth_conf { >>> #define RTE_ETH_TX_OFFLOAD_MULTI_SEGS RTE_BIT64(15) >>> /** >>> * Device supports optimization for fast release of mbufs. >>> - * When set application must guarantee that per-queue all mbufs >> comes from >>> - * the same mempool and has refcnt = 1. >>> + * When set application must guarantee that per-queue all mbufs come >> from the same mempool, >>> + * are direct, have refcnt=1, next=NULL and nb_segs=1, as done by >> rte_pktmbuf_prefree_seg(). >>> + * >>> + * @see rte_mbuf_fast_free_bulk() >>> */ >>> #define RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE RTE_BIT64(16) >>> #define RTE_ETH_TX_OFFLOAD_SECURITY RTE_BIT64(17) >>> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h >>> index 0d2e0e64b3..7590d82689 100644 >>> --- a/lib/mbuf/rte_mbuf.h >>> +++ b/lib/mbuf/rte_mbuf.h >>> @@ -568,6 +568,10 @@ __rte_mbuf_raw_sanity_check(__rte_unused const >> struct rte_mbuf *m) >>> RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1); >>> RTE_ASSERT(m->next == NULL); >>> RTE_ASSERT(m->nb_segs == 1); >>> + RTE_ASSERT(!RTE_MBUF_CLONED(m)); >>> + RTE_ASSERT(!RTE_MBUF_HAS_EXTBUF(m) || >>> + (RTE_MBUF_HAS_PINNED_EXTBUF(m) && >>> + rte_mbuf_ext_refcnt_read(m->shinfo) == 1)); >>> __rte_mbuf_sanity_check(m, 0); >>> } >>> >>> @@ -623,12 +627,43 @@ static inline struct rte_mbuf >> *rte_mbuf_raw_alloc(struct rte_mempool *mp) >>> static __rte_always_inline void >>> rte_mbuf_raw_free(struct rte_mbuf *m) >>> { >>> - RTE_ASSERT(!RTE_MBUF_CLONED(m) && >>> - (!RTE_MBUF_HAS_EXTBUF(m) || >> RTE_MBUF_HAS_PINNED_EXTBUF(m))); >>> __rte_mbuf_raw_sanity_check(m); >>> rte_mempool_put(m->pool, m); >>> } >>> >>> +/** >>> + * Put a bulk of mbufs allocated from the same mempool back into the >> mempool. >>> + * >>> + * The caller must ensure that the mbufs come from the specified >> mempool, >>> + * are direct and properly reinitialized (refcnt=1, next=NULL, >> nb_segs=1), as done by >>> + * rte_pktmbuf_prefree_seg(). >>> + * >>> + * This function should be used with care, when optimization is >>> + * required. For standard needs, prefer rte_pktmbuf_free_bulk(). >>> + * >>> + * @see RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE >>> + * >>> + * @param mp >>> + * The mempool to which the mbufs belong. >>> + * @param mbufs >>> + * Array of pointers to packet mbufs. >>> + * The array must not contain NULL pointers. >>> + * @param count >>> + * Array size. >>> + */ >>> +static __rte_always_inline void >>> +rte_mbuf_fast_free_bulk(struct rte_mempool *mp, struct rte_mbuf >> **mbufs, unsigned int count) >>> +{ >>> + for (unsigned int idx = 0; idx < count; idx++) { >>> + const struct rte_mbuf *m = mbufs[idx]; >>> + RTE_ASSERT(m != NULL); >>> + RTE_ASSERT(m->pool == mp); >>> + __rte_mbuf_raw_sanity_check(m); >>> + } >> >> Is there some way to avoid executing a loop in non-debug mode? Like the >> following or other better way >> >> #ifdef RTE_LIBRTE_MBUF_DEBUG >> { >> for (unsigned int idx = 0; idx < count; idx++) { >> const struct rte_mbuf *m = mbufs[idx]; >> RTE_ASSERT(m != NULL); >> RTE_ASSERT(m->pool == mp); >> __rte_mbuf_raw_sanity_check(m); >> } >> } >> #endif > > The loop is already omitted in non-debug mode: > RTE_ASSERT() [1] is omitted unless RTE_ENABLE_ASSERT is set. > __rte_mbuf_raw_sanity_check() [2] consist of some RTE_ASSERTs and __rte_mbuf_sanity_check(). > __rte_mbuf_sanity_check() [3] is omitted unless RTE_LIBRTE_MBUF_DEBUG is set. > > So the compiler will detect that the loop has no effect, and optimize away the loop. Okay, you're right, I compiled the code and this loop code didn't generate any instructions after the compiler optimization was enabled. > > [1]: https://elixir.bootlin.com/dpdk/v24.11.1/source/lib/eal/include/rte_debug.h#L46 > [2]: https://elixir.bootlin.com/dpdk/v24.11.1/source/lib/mbuf/rte_mbuf.h#L566 > [3]: https://elixir.bootlin.com/dpdk/v24.11.1/source/lib/mbuf/rte_mbuf.h#L348 > >> >>> + >>> + rte_mempool_put_bulk(mp, (void **)mbufs, count); >> >> Can the mp be obtained from the mbuf? > > Passing "mp" as a parameter avoids a potential CPU cache miss by not dereferencing the first mbuf, if the driver/application already has the mempool pointer readily available (and hot in the CPU cache) from somewhere else. > If the driver/or application doesn't have the mempool pointer readily available, it can obtain it from the mbuf when calling this function. > > And as a bonus side effect, passing "mp" as a parameter allows calling the function with count=0 without special handling inside the function. > > Obviously, if the driver/application gets "mp" from mbuf[0]->pool, it needs to first check that count>0; but that would be the situation for the driver/application whenever it accesses an mbuf array, regardless what it is doing with that array. > Okay, It's better this way. Acked-by: huangdengdui@huawei.com