On Wed, 11 Jan 2017 18:41:28 +0000 Ferruh Yigit <ferruh.yi...@intel.com> 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 <konstantin.anan...@intel.com>; Stephen Hemminger > >> <step...@networkplumber.org> > >> Cc: Sergey Vyazmitinov <s.vyazmiti...@brain4net.com>; > >> olivier.m...@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:step...@networkplumber.org] > >>>> Sent: Wednesday, January 11, 2017 5:36 PM > >>>> To: Ananyev, Konstantin <konstantin.anan...@intel.com> > >>>> Cc: Sergey Vyazmitinov <s.vyazmiti...@brain4net.com>; > >>>> olivier.m...@6wind.com; Yigit, Ferruh <ferruh.yi...@intel.com>; > >> 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" <konstantin.anan...@intel.com> wrote: > >>>> > >>>>>> -----Original Message----- > >>>>>> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Stephen Hemminger > >>>>>> Sent: Wednesday, January 11, 2017 4:18 PM > >>>>>> To: Sergey Vyazmitinov <s.vyazmiti...@brain4net.com> > >>>>>> Cc: olivier.m...@6wind.com; Yigit, Ferruh <ferruh.yi...@intel.com>; > >>>>>> 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 <s.vyazmiti...@brain4net.com> 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). 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: This handles multiple pools and multi-segment and indirect mbufs.
