From patchwork Wed Jan 11 18:56:20 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Hemminger X-Patchwork-Id: 19152 Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 46D6BF610; Wed, 11 Jan 2017 19:56:37 +0100 (CET) Received: from mail-pg0-f49.google.com (mail-pg0-f49.google.com [74.125.83.49]) by dpdk.org (Postfix) with ESMTP id BC48AD592 for ; Wed, 11 Jan 2017 19:56:34 +0100 (CET) Received: by mail-pg0-f49.google.com with SMTP id 14so39030150pgg.1 for ; Wed, 11 Jan 2017 10:56:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=cyD/9G9ALbOQ44Xt5bmTiUlhYbfxW01nKyPRlyQdsFs=; b=pi5zpB5jr026T+WF+5YchJW9GsZIDJ1PGbCLWrsEK3SvSIbpEdgHaeqIjf3oJr4Et+ p+T20UxaEHh8gKWfLfTjxKWTT+mhhg5gR9K7e9o0LvL5JU+sRrX/9WNkPAZ/iE74IqKD /5gcvBa+ukfEsUZdNaN2cI0kDlSTXmKEwyVNY5rmt+lulYeyaTctt1wEgv9IEUZEutNL fmkSkeplv15YkzvOE6FhTp0NEXPooYOl7V8gxWQ0sVA4OENm2mmy+hHk1LAOeBMHLLZ/ 4FgguRkQwElfdY0IWbVbFiQMStXDDHBePVVwpkDR/OBgAdwXGahngyPxsaq1V8vxV5N6 4iXw== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=cyD/9G9ALbOQ44Xt5bmTiUlhYbfxW01nKyPRlyQdsFs=; b=XkjNGxlzW9wZJst50Kf9mMq9J9UNNZ4p85WKtolMHFint920jc62MVofiwSJx2yLDH 8psmHzSRXSRvRLXp3xSAIP5BmmtjwJM/l1I/Vs60p7KWhLAmAUOQ2RSXJ8sz22xbDpZ0 puWpdKeC63v/+Sklq7Kvnjn+7vkjCNi0tEeRBOLT/5pqqN3h/akIZU7TaGw/dTNWOufw N+g9eYjz+y7UMhM1J+HxT2OZsPudfHwy2FkIq5758RYarzhJdpz6Wg7ySB/M8nnUybl+ Gw6paRgTR+Y9VN+XvxMUUJ9c/nCkyUGGwlkZP9PGAKUECt9ZfX8UnPfeWK3XalzqYVUB JNXw== X-Gm-Message-State: AIkVDXKng3c6e38xRuS/ig/3eiLTdptXkCcWhsjdQchse7HmtKK/6xJr88t4XeTysV8PXw== X-Received: by 10.99.114.91 with SMTP id c27mr12537595pgn.163.1484160993821; Wed, 11 Jan 2017 10:56:33 -0800 (PST) Received: from xeon-e3 (204-195-18-65.wavecable.com. [204.195.18.65]) by smtp.gmail.com with ESMTPSA id c22sm15705429pgn.12.2017.01.11.10.56.33 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 11 Jan 2017 10:56:33 -0800 (PST) Date: Wed, 11 Jan 2017 10:56:20 -0800 From: Stephen Hemminger To: Ferruh Yigit Cc: "Ananyev, Konstantin" , Sergey Vyazmitinov , "olivier.matz@6wind.com" , "dev@dpdk.org" Message-ID: <20170111105620.00af45a7@xeon-e3> In-Reply-To: References: <1483048216-2936-1-git-send-email-s.vyazmitinov@brain4net.com> <20170111081759.7b1ee146@xeon-e3> <2601191342CEEE43887BDE71AB9772583F103F8F@irsmsx105.ger.corp.intel.com> <20170111093559.753a0fc9@xeon-e3> <2601191342CEEE43887BDE71AB9772583F103FCA@irsmsx105.ger.corp.intel.com> <63819ae1-f056-0ad7-b7dd-041fe1fe08fa@intel.com> <2601191342CEEE43887BDE71AB9772583F104048@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs 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 Wed, 11 Jan 2017 18:41:28 +0000 Ferruh Yigit wrote: > On 1/11/2017 6:25 PM, Ananyev, Konstantin wrote: > > > > > >> -----Original Message----- > >> From: Yigit, Ferruh > >> Sent: Wednesday, January 11, 2017 5:48 PM > >> To: Ananyev, Konstantin ; Stephen Hemminger > >> Cc: Sergey Vyazmitinov ; olivier.matz@6wind.com; dev@dpdk.org > >> Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs > >> > >> On 1/11/2017 5:43 PM, Ananyev, Konstantin wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org] > >>>> Sent: Wednesday, January 11, 2017 5:36 PM > >>>> To: Ananyev, Konstantin > >>>> Cc: Sergey Vyazmitinov ; olivier.matz@6wind.com; Yigit, Ferruh ; > >> dev@dpdk.org > >>>> Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs > >>>> > >>>> On Wed, 11 Jan 2017 17:28:21 +0000 > >>>> "Ananyev, Konstantin" wrote: > >>>> > >>>>>> -----Original Message----- > >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger > >>>>>> Sent: Wednesday, January 11, 2017 4:18 PM > >>>>>> To: Sergey Vyazmitinov > >>>>>> Cc: olivier.matz@6wind.com; Yigit, Ferruh ; dev@dpdk.org > >>>>>> Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs > >>>>>> > >>>>>> On Fri, 30 Dec 2016 04:50:16 +0700 > >>>>>> Sergey Vyazmitinov wrote: > >>>>>> > >>>>>>> /** > >>>>>>> + * Free n packets mbuf back into its original mempool. > >>>>>>> + * > >>>>>>> + * Free each mbuf, and all its segments in case of chained buffers. Each > >>>>>>> + * segment is added back into its original mempool. > >>>>>>> + * > >>>>>>> + * @param mp > >>>>>>> + * The packets mempool. > >>>>>>> + * @param mbufs > >>>>>>> + * The packets mbufs array to be freed. > >>>>>>> + * @param n > >>>>>>> + * Number of packets. > >>>>>>> + */ > >>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mempool *mp, > >>>>>>> + struct rte_mbuf **mbufs, unsigned n) > >>>>>>> +{ > >>>>>>> + struct rte_mbuf *mbuf, *m_next; > >>>>>>> + unsigned i; > >>>>>>> + for (i = 0; i < n; ++i) { > >>>>>>> + mbuf = mbufs[i]; > >>>>>>> + __rte_mbuf_sanity_check(mbuf, 1); > >>>>>>> + > >>>>>>> + mbuf = mbuf->next; > >>>>>>> + while (mbuf != NULL) { > >>>>>>> + m_next = mbuf->next; > >>>>>>> + rte_pktmbuf_free_seg(mbuf); > >>>>>>> + mbuf = m_next; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + rte_mempool_put_bulk(mp, (void * const *)mbufs, n); > >>>>>>> +} > >>>>>> > >>>>>> The mbufs may come from different pools. You need to handle that. > >>>>> > >>>>> I suppose both stituations are possible: > >>>>> 1) user knows off-hand that all mbufs in the group are from the same mempool > >>>>> 2) user can't guarantee that all mbufs in the group are from same mempool. > >>>>> > >>>>> As I understand that patch is for case 1) only. > >>>>> For 2) it could be a separate function and separate patch. > >>>>> > >>>>> Konstantin > >>>>> > >>>>> > >>>> > >>>> Please don't make unnecessary assumptions in pursuit of minor optimizations. > >>> > >>> I don't suggest to make *any* assumptions. > >>> What I am saying we can have 2 functions for two different cases. > >> > >> kni_free_mbufs() is static function. Even user knows if all mbufs are > >> some same pool or not, can't pass this information to the free function. > >> > >> Of course this information can be passed via new API, or as an update to > >> exiting API, but I think it is better to update free function to cover > >> both cases instead of getting this information from user. > > > > I suppose misunderstanding came from the fact that kni_free_mbufs() > > is modified to use rte_pktmbuf_free_bulk(mp, mbufs, n). > > I am not talking about kni part of the patch > > (to be honest I didn't pay much attention to it). > > What I am saying there are many situations when user knows off-hand > > that all mbufs in that group are from the same mempool and such > > function will be useful too. > > > BTW, for my own curiosity, how it could happen with KNI that > > kni_fifo_get() would return mbufs not from kni->pktmbuf_pool > > (I am not really familiar with KNI and its use-cases)? > > It gets packets from free queue: > kni_fifo_get(kni->free_q, ...) > > DPDK app may send a mbuf (from any pool, like another port's mempool) to > kernel, kernel puts buf back to kni->free_q when done with it. > > > Konstantin > > > >> > >>> Obviously we'll have to document it properly. > >>> Konstantin > >>> > >>>> It is trivial to write a correct free bulk that handles pool changing. > >>>> Also the free_seg could be bulked as well. > > > Please write generic code. Something like the following (untested). This handles multiple pools and multi-segment and indirect mbufs. diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index 4476d75379fd..b7a743ec5c87 100644 --- a/lib/librte_mbuf/rte_mbuf.h +++ b/lib/librte_mbuf/rte_mbuf.h @@ -306,6 +306,9 @@ extern "C" { /** Alignment constraint of mbuf private area. */ #define RTE_MBUF_PRIV_ALIGN 8 +/** Maximum number of mbufs freed in bulk. */ +#define RTE_MBUF_BULK_FREE 64 + /** * Get the name of a RX offload flag * @@ -1261,6 +1264,50 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m) } /** + * Free n packets mbuf back into its original mempool. + * + * Free each mbuf, and all its segments in case of chained buffers. Each + * segment is added back into its original mempool. + * + * @param mbufs + * The packets mbufs array to be freed. + * @param n + * Number of packets. + */ +static inline void rte_pktmbuf_free_bulk(struct rte_mbuf **mbufs, + unsigned n) +{ + struct rte_mbuf *tofree[RTE_MBUF_BULK_FREE]; + struct rte_mempool *mp; + unsigned i, count = 0; + + for (i = 0; i < n; i++) { + struct rte_mbuf *m, *m_next; + + for (m = mbufs[i]; m; m = m_next) { + m_next = m->next; + + if (count > 0 && + (unlikely(m->pool != mp || count == RTE_MBUF_BULK_FREE))) { + rte_mempool_put_bulk(mp, tofree, count); + count = 0; + } + + mp = m->pool; + + if (likely(__rte_pktmbuf_prefree_seg(m))) { + m->next = NULL; + tofree[count++] = m; + } + } + } + + if (likely(count > 0)) + rte_mempool_put_bulk(mp, tofree, count); +} + + +/** * Creates a "clone" of the given packet mbuf. * * Walks through all segments of the given packet mbuf, and for each of them: