Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 11 Jan 2017 18:41:28 +0000
Ferruh Yigit <ferruh.yigit@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.ananyev@intel.com>; Stephen Hemminger <stephen@networkplumber.org>
> >> Cc: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>; 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 <konstantin.ananyev@intel.com>
> >>>> Cc: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>; olivier.matz@6wind.com; Yigit, Ferruh <ferruh.yigit@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.ananyev@intel.com> 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 <s.vyazmitinov@brain4net.com>
> >>>>>> Cc: olivier.matz@6wind.com; Yigit, Ferruh <ferruh.yigit@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.vyazmitinov@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.