From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B999CA2F6B for ; Wed, 9 Oct 2019 08:47:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 50FD41C10A; Wed, 9 Oct 2019 08:47:08 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 7A4A11C0C0 for ; Wed, 9 Oct 2019 08:47:06 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us2.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id C72C030005C; Wed, 9 Oct 2019 06:47:00 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 9 Oct 2019 07:46:53 +0100 To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , CC: , , , , , References: <20191007101427.16191-1-mb@smartsharesystems.com> <20191007101427.16191-2-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35C60B62@smartserver.smartshare.dk> From: Andrew Rybchenko Message-ID: <51ee6f96-8d29-3625-d1de-3b1fbb1dde48@solarflare.com> Date: Wed, 9 Oct 2019 09:46:50 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C60B62@smartserver.smartshare.dk> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24964.003 X-TM-AS-Result: No-13.458200-8.000000-10 X-TMASE-MatchedRID: Ync95tbzDRnmLzc6AOD8DfHkpkyUphL9Ud7Bjfo+5jTVEYfep7Szpq6+ UxOBi85N7nhDtapR+JTO/WH+Jlz/QP1opoHDJD6EAZ0lncqeHqHbODDlFP8Gtizt4/lw8JZDLlN xKgX1XBKn92DdnY0wJa23OopKfqlCRpHf6sgiT6EPe5gzF3TVt9I6F3PaxADRHCP6WbPscS+ujy kEmQpw6s6/rzj1EIUjQpBj3QW86Bnn88dFYev1lj8Ckw9b/GFeVBDQSDMig9Hfc2Xd6VJ+yvm8t yRwJQtU5HzvdTqjiYu9PhyG9FknIM+D6Af5OJeT1uDM+OtX7BxJaD67iKvY03M6Tvz5qtoQlMC1 svsubLSKXj1JPRZV8MAE15P+XiwapqDX27S3uVtuyDGIvvwW0jsY2/UEG7fklYCBiTJAwbiogo2 jmAgWs2+Zvj88gdTZUmgtxU7rzVgSGqiY07lJdoWC+79yLa56ZwLLfALfnN3JyYU2J054PZKPru if6Wh8+q4gdl1Thtjw0i5tJ0iCwA0qmJSh4JL/9aPMjkYccfnkRE2h9ncEXD2cdopIjrPd+HmBP yReSgxQAl8+fubV5fSrrq2eCJ04L2OlfPfOnFWASxsXx/YbJEw1FmECWDxdGUs9b7xvtJpdQJCl pD/4p42UXV5916fTijdXILMQwyXP+175rCkLSkI4eS9mV4/s57e8iWh64He/mUuzA32VMcOLWoD 10UccCsRPQ32r0MpExYaawGWfuxJIcQw9AtzMN19PjPJahlJB9uUinegF0Zsoi2XrUn/Jsuf7RW bvUtx1r8FBbF0I5gtuKBGekqUpI/NGWt0UYPAbuFWeXSIrtNbvtEYMo0ewnISpx0ZaVuGjPxUo5 KqQWtT2H03zzU1J X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--13.458200-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24964.003 X-MDID: 1570603622-s_qKfwJT7BTy Subject: Re: [dpdk-dev] [PATCH v4 1/1] mbuf: add bulk free function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 10/8/19 11:12 PM, Morten Brørup wrote: >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Rybchenko >> Sent: Tuesday, October 8, 2019 7:26 PM >> >> On 10/7/19 1:14 PM, Morten Brørup wrote: >>> Add function for freeing a bulk of mbufs. >>> >>> v4: >>> * Marked as experimental by adding __rte_experimental. >>> * Add function to experimental section of map file. >>> * Fix source code formatting regarding pointer to pointer. >>> * Squashed multiple modifications into one. >>> v3: >>> * Bugfix: Handle pakets with multiple segments. >>> * Added inline helper function, mainly for readability. >>> * Fix source code formatting regarding indentation. >>> v2: >>> * Function is not inline. >>> * Optimized to free multible mbufs belonging to the same mempool in >>> bulk. Inspired by ixgbe_tx_free_bufs(), but allowing NULL pointers >>> in the array, just like rte_pktmbuf_free() can take a NULL pointer. >>> * Use unsigned int instead of unsigned. Passes checkpatch, but >>> mismatches the original coding style of the modified files. >>> * Fixed a typo in the description headline: mempools is plural. >>> >>> Signed-off-by: Morten Brørup >>> --- >>> lib/librte_mbuf/rte_mbuf.c | 50 ++++++++++++++++++++++++++++ >>> lib/librte_mbuf/rte_mbuf.h | 12 +++++++ >>> lib/librte_mbuf/rte_mbuf_version.map | 1 + >>> 3 files changed, 63 insertions(+) >>> >>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c >>> index 37718d49c..0088117a3 100644 >>> --- a/lib/librte_mbuf/rte_mbuf.c >>> +++ b/lib/librte_mbuf/rte_mbuf.c >>> @@ -245,6 +245,56 @@ int rte_mbuf_check(const struct rte_mbuf *m, int >> is_header, >>> return 0; >>> } >>> >>> +/** >>> + * Size of the array holding mbufs from the same membool to be freed in >> bulk. >>> + */ >>> +#define RTE_PKTMBUF_FREE_BULK_SZ 64 >> How is the constant chosen? When we benchmarked similar thing in >> net/sfc, we finally stick to 32 as the best option. >> > I took it from the ixgbe driver that was the inspiration: > http://code.dpdk.org/dpdk/latest/source/drivers/net/ixgbe/ixgbe_rxtx.c#L111 > http://code.dpdk.org/dpdk/latest/source/drivers/net/ixgbe/ixgbe_rxtx.h#L32 > > If only DPDK provided a common #define for recommended burst array sizes. However, I vaguely recall a discussion recommending it to be dependent on the application and various drivers due to requirements of 100 Gbps drivers. I guess it probably also depends on the CPU being used. I have no qualified opinion about this constant, so I can change it to anything you recommend. No, no, it is OK for me as is. Just curiosity. >>> + >>> +/** >>> + * @internal helper function for freeing a bulk of mbufs via an array >> holding >>> + * mbufs from the same mempool. >>> + */ >>> +static __rte_always_inline void >>> +rte_pktmbuf_free_seg_via_array(struct rte_mbuf *m, >> Other internal functions in the file have __ (two underscore) prefix. >> > The other internal function is public, i.e. not private (static). However, this might also go public one day, so I'll update it in the next version. > >>> + struct rte_mbuf ** const free, unsigned int * const nb_free) >> free is bad variable name because of libc free() function. >> It is a bit confusing. Same below. >> > I agree. I simply took it from the ixgbe driver without thinking further about it. How about "pending" or "bulk", or do you have a better suggestion? Both "pending" and "bulk" are OK for me. Up to you. >>> +{ >>> + m = rte_pktmbuf_prefree_seg(m); >>> + if (likely(m != NULL)) { >> I'm not sure about likely() here, but hope that compiler is wise >> enough to inline rte_pktmbuf_prefree_seg() here and avoid the >> branch completely. >> > I somewhat agree with you about this likely() being questionable, and also hope for the compiler to be wise enough anyway. > > This line of code in my function stems from unfolding rte_pktmbuf_free_seg(), which has the same likely(), so I kept it. > > Apparently, the DPDK mbuf library has a strong preference for single-reference mbufs, with lots of likely()/unlikely() assuming that the m.refcnt is never more than one. rte_pktmbuf_prefree_seg() has the same preference. So I think that we should follow the library's convention, respecting that this function is at the very core of the data plane. Makes sense. > The DPDK rationale behind these likely()/unlikely() hints must be that multicasting, flooding and mirroring packets are rare exceptions. We have previously discussed this internally in our organization, and we tend to agree under normal circumstances. It is just an unfortunate side effect that a common network debugging feature - mirroring - introduces an additional performance penalty of having likely()/unlikely() go in the wrong direction, which might again affect what is being debugged. Thanks, Andrew.