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. >
