[dpdk-dev] [PATCH v6 0/2] kni: use bulk functions to allocate and free mbufs

[dpdk-dev] [PATCH v6 0/2] kni: use bulk functions to allocate and free mbufs

[Sergey Vyazmitinov]

Optimized kni_allocate_mbufs and kni_free_mbufs by using mbuf bulk functions

Sergey Vyazmitinov (2):
  kni: add bulk function to free mbufs
  kni: Use bulk functions to allocate and free mbufs

 lib/librte_kni/rte_kni.c      | 47 +++++++++++++++++++----------------------
 lib/librte_kni/rte_kni_fifo.h | 18 ++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h    | 49 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 25 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v6 1/2] kni: add bulk function to free mbufs

[Sergey Vyazmitinov]

Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>
---
v3:
* Fixed issue with possible different mempools in buffer list.
* Fixed issue with wrong rte_pktmbuf_alloc_bulk function return value
processing in the kni_allocate_mbufs.
---
 lib/librte_mbuf/rte_mbuf.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 4476d75..69d314f 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,52 @@ 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 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_mbuf **mbufs,
+		unsigned int n)
+{
+	void *tofree[RTE_MBUF_BULK_FREE];
+	struct rte_mempool *mp = NULL;
+	unsigned int 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) != NULL)) {
+				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:
-- 
2.7.4

Re: [dpdk-dev] [PATCH v6 1/2] kni: add bulk function to free mbufs

[Olivier Matz]

Hi,

On Thu, 19 Jan 2017 11:46:58 +0700, Sergey Vyazmitinov
<s.vyazmitinov@brain4net.com> wrote:
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>
> ---
> v3:
> * Fixed issue with possible different mempools in buffer list.
> * Fixed issue with wrong rte_pktmbuf_alloc_bulk function return value
> processing in the kni_allocate_mbufs.
> ---
>  lib/librte_mbuf/rte_mbuf.h | 49
> ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49
> insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 4476d75..69d314f 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,52 @@ 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 mp
> + *   The packets mempool.

This parameter was removed, it could be removed from the API comment.


> + * @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 int n)

I suggest we could use mbuf instead of pktmbuf in the function name.
It's a bit shorter, and the function would also apply on ctrlmbuf.

Also, the struct rte_mbuf **mbufs could probably be replaced by struct
rte_mbuf * const *mbufs.


> +{
> +	void *tofree[RTE_MBUF_BULK_FREE];
> +	struct rte_mempool *mp = NULL;
> +	unsigned int 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) !=
> NULL)) {
> +				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.
>   *

The function looks good to me, thank you. It looks also better than
what I've suggested in [1], since it properly manage mbuf chains. On
the other hand, I think my proposal could also help in drivers, where
segments are already unchained. I'll submit it in a RFC.

[1] http://dpdk.org/ml/archives/dev/2017-January/054538.html


One more thing, maybe it's worth adding a basic test in
app/test/test_mbuf.c.

Thanks,
Olivier

Re: [dpdk-dev] [PATCH v6 1/2] kni: add bulk function to free mbufs

[Olivier Matz]

Hi,

[resent and updated, previous mail was sent from the wrong address]


On Mon, 23 Jan 2017 13:59:47 +0100, Olivier Matz
<olivier.matz@6wind.com> wrote:
> > + * @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 int n)  
> 
> I suggest we could use mbuf instead of pktmbuf in the function name.
> It's a bit shorter, and the function would also apply on ctrlmbuf.

Sorry, please ignore this one. It's more consistent to have a
rte_pktmbuf_free and rte_pktmbuf_free_bulk having the same prefix.



Also a small comment on the title: it should be "mbuf:", not "kni:".

In the other patch, the uppercase in "Use" could also be changed to
lowercase (seen with ./devtools/check-git-log.sh).

Thanks,
Olivier

[dpdk-dev] [PATCH v6 2/2] kni: Use bulk functions to allocate and free mbufs

[Sergey Vyazmitinov]

Optimized kni_allocate_mbufs and kni_free_mbufs by using mbuf
bulk functions. This can improve performance more than two times.

Signed-off-by: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>
---
v5:
* use rte_pktmbuf_free_bulk for removing packets which was not
put in alloc queue.
v6:
* fix c99 compilation error
---
 lib/librte_kni/rte_kni.c      | 47 ++++++++++++++++++++-----------------------
 lib/librte_kni/rte_kni_fifo.h | 18 +++++++++++++++++
 2 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index a80cefd..6591f4f 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -590,22 +590,21 @@ rte_kni_rx_burst(struct rte_kni *kni, struct rte_mbuf **mbufs, unsigned num)
 static void
 kni_free_mbufs(struct rte_kni *kni)
 {
-	int i, ret;
+	unsigned int freeing;
 	struct rte_mbuf *pkts[MAX_MBUF_BURST_NUM];
 
-	ret = kni_fifo_get(kni->free_q, (void **)pkts, MAX_MBUF_BURST_NUM);
-	if (likely(ret > 0)) {
-		for (i = 0; i < ret; i++)
-			rte_pktmbuf_free(pkts[i]);
+	freeing = kni_fifo_get(kni->free_q, (void **)pkts, MAX_MBUF_BURST_NUM);
+	if (likely(freeing > 0)) {
+		rte_pktmbuf_free_bulk(pkts, freeing);
 	}
 }
 
 static void
 kni_allocate_mbufs(struct rte_kni *kni)
 {
-	int i, ret;
-	struct rte_mbuf *pkts[MAX_MBUF_BURST_NUM];
-	void *phys[MAX_MBUF_BURST_NUM];
+	unsigned int count, put, i;
+	struct rte_mbuf *pkts[KNI_FIFO_COUNT_MAX];
+	void *phys[KNI_FIFO_COUNT_MAX];
 
 	RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pool) !=
 			 offsetof(struct rte_kni_mbuf, pool));
@@ -628,28 +627,26 @@ kni_allocate_mbufs(struct rte_kni *kni)
 		return;
 	}
 
-	for (i = 0; i < MAX_MBUF_BURST_NUM; i++) {
-		pkts[i] = rte_pktmbuf_alloc(kni->pktmbuf_pool);
-		if (unlikely(pkts[i] == NULL)) {
-			/* Out of memory */
-			RTE_LOG(ERR, KNI, "Out of memory\n");
-			break;
-		}
-		phys[i] = va2pa(pkts[i]);
-	}
+	/* Calculate alloc queue free space */
+	count = kni_fifo_free_count(kni->alloc_q);
 
-	/* No pkt mbuf alocated */
-	if (i <= 0)
+	/* Get buffers from mempool */
+	if (rte_pktmbuf_alloc_bulk(kni->pktmbuf_pool, pkts, count) != 0) {
+		RTE_LOG(ERR, KNI, "Can`t allocate %d mbufs\n", count);
 		return;
+	}
+
+	for (i = 0; i < count; i++)
+		phys[i] = va2pa(pkts[i]);
 
-	ret = kni_fifo_put(kni->alloc_q, phys, i);
+	/* Put buffers into alloc queue */
+	put = kni_fifo_put(kni->alloc_q, (void **)phys, count);
 
 	/* Check if any mbufs not put into alloc_q, and then free them */
-	if (ret >= 0 && ret < i && ret < MAX_MBUF_BURST_NUM) {
-		int j;
-
-		for (j = ret; j < i; j++)
-			rte_pktmbuf_free(pkts[j]);
+	if (unlikely(put < count)) {
+		RTE_LOG(ERR, KNI, "Free %u of %u allocated buffers\n",
+			count - put, count);
+		rte_pktmbuf_free_bulk(pkts + put, count - put);
 	}
 }
 
diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
index 8cb8587..361ddb0 100644
--- a/lib/librte_kni/rte_kni_fifo.h
+++ b/lib/librte_kni/rte_kni_fifo.h
@@ -91,3 +91,21 @@ kni_fifo_get(struct rte_kni_fifo *fifo, void **data, unsigned num)
 	fifo->read = new_read;
 	return i;
 }
+
+/**
+ * Get the num of elements in the fifo
+ */
+static inline unsigned
+kni_fifo_count(struct rte_kni_fifo *fifo)
+{
+	return (fifo->len + fifo->write - fifo->read) & (fifo->len - 1);
+}
+
+/**
+ * Get the num of available elements in the fifo
+ */
+static inline unsigned
+kni_fifo_free_count(struct rte_kni_fifo *fifo)
+{
+	return (fifo->read - fifo->write - 1) & (fifo->len - 1);
+}
-- 
2.7.4

Re: [dpdk-dev] [PATCH v6 2/2] kni: Use bulk functions to allocate and free mbufs

[Ferruh Yigit]

On 1/19/2017 4:46 AM, Sergey Vyazmitinov wrote:
> Optimized kni_allocate_mbufs and kni_free_mbufs by using mbuf
> bulk functions. This can improve performance more than two times.
> 
> Signed-off-by: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>
> ---
> v5:
> * use rte_pktmbuf_free_bulk for removing packets which was not
> put in alloc queue.
> v6:
> * fix c99 compilation error
> ---
>  lib/librte_kni/rte_kni.c      | 47 ++++++++++++++++++++-----------------------
>  lib/librte_kni/rte_kni_fifo.h | 18 +++++++++++++++++
>  2 files changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index a80cefd..6591f4f 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -590,22 +590,21 @@ rte_kni_rx_burst(struct rte_kni *kni, struct rte_mbuf **mbufs, unsigned num)
>  static void
>  kni_free_mbufs(struct rte_kni *kni)
>  {
> -	int i, ret;
> +	unsigned int freeing;
>  	struct rte_mbuf *pkts[MAX_MBUF_BURST_NUM];
>  
> -	ret = kni_fifo_get(kni->free_q, (void **)pkts, MAX_MBUF_BURST_NUM);
> -	if (likely(ret > 0)) {
> -		for (i = 0; i < ret; i++)
> -			rte_pktmbuf_free(pkts[i]);
> +	freeing = kni_fifo_get(kni->free_q, (void **)pkts, MAX_MBUF_BURST_NUM);
> +	if (likely(freeing > 0)) {
> +		rte_pktmbuf_free_bulk(pkts, freeing);
>  	}
>  }
>  
>  static void
>  kni_allocate_mbufs(struct rte_kni *kni)
>  {
> -	int i, ret;
> -	struct rte_mbuf *pkts[MAX_MBUF_BURST_NUM];
> -	void *phys[MAX_MBUF_BURST_NUM];
> +	unsigned int count, put, i;
> +	struct rte_mbuf *pkts[KNI_FIFO_COUNT_MAX];
> +	void *phys[KNI_FIFO_COUNT_MAX];
>  
>  	RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pool) !=
>  			 offsetof(struct rte_kni_mbuf, pool));
> @@ -628,28 +627,26 @@ kni_allocate_mbufs(struct rte_kni *kni)
>  		return;
>  	}
>  
> -	for (i = 0; i < MAX_MBUF_BURST_NUM; i++) {
> -		pkts[i] = rte_pktmbuf_alloc(kni->pktmbuf_pool);
> -		if (unlikely(pkts[i] == NULL)) {
> -			/* Out of memory */
> -			RTE_LOG(ERR, KNI, "Out of memory\n");
> -			break;
> -		}
> -		phys[i] = va2pa(pkts[i]);
> -	}
> +	/* Calculate alloc queue free space */
> +	count = kni_fifo_free_count(kni->alloc_q);

This seems reason of the performance loss mentioned in cover letter.
Updating this line as following recovers the performance:

count = MAX_MBUF_BURST_NUM;

kni->alloc_q is fifo shared between kernel thread, which runs on its own
core, and the rx poll core.
Getting free count and allocating that much mbuf causing worse
performance, comparing to the original behavior.



Btw, I have tried something else, which is giving conflicting results
(it was better when I first tried, now getting worse result):

kni_allocate_mbufs() called every time received some packets from
kernel, because this means kernel thread consumed some mbufs from
alloc_q, and userspace needs to fill it back again.

So, instead of allocating fixed number of mbuf, or allocating free
number of fifo mbuf, what about allocating amount of mbufs that received
by kernel.
Since kernel consumed that much, fill back that much.

make this function:
kni_allocate_mbufs(struct rte_kni *kni, unsigned int num);

rte_kni_rx_burst() calls:
kni_allocate_mbufs(kni, ret);

What do you think?
If possible can you please try this?


Thanks,
ferruh

>  
> -	/* No pkt mbuf alocated */
> -	if (i <= 0)
> +	/* Get buffers from mempool */
> +	if (rte_pktmbuf_alloc_bulk(kni->pktmbuf_pool, pkts, count) != 0) {
> +		RTE_LOG(ERR, KNI, "Can`t allocate %d mbufs\n", count);
>  		return;
> +	}
> +
> +	for (i = 0; i < count; i++)
> +		phys[i] = va2pa(pkts[i]);
>  
> -	ret = kni_fifo_put(kni->alloc_q, phys, i);
> +	/* Put buffers into alloc queue */
> +	put = kni_fifo_put(kni->alloc_q, (void **)phys, count);
>  
>  	/* Check if any mbufs not put into alloc_q, and then free them */
> -	if (ret >= 0 && ret < i && ret < MAX_MBUF_BURST_NUM) {
> -		int j;
> -
> -		for (j = ret; j < i; j++)
> -			rte_pktmbuf_free(pkts[j]);
> +	if (unlikely(put < count)) {
> +		RTE_LOG(ERR, KNI, "Free %u of %u allocated buffers\n",
> +			count - put, count);
> +		rte_pktmbuf_free_bulk(pkts + put, count - put);
>  	}
>  }
>  
> diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
> index 8cb8587..361ddb0 100644
> --- a/lib/librte_kni/rte_kni_fifo.h
> +++ b/lib/librte_kni/rte_kni_fifo.h
> @@ -91,3 +91,21 @@ kni_fifo_get(struct rte_kni_fifo *fifo, void **data, unsigned num)
>  	fifo->read = new_read;
>  	return i;
>  }
> +
> +/**
> + * Get the num of elements in the fifo
> + */
> +static inline unsigned
> +kni_fifo_count(struct rte_kni_fifo *fifo)
> +{
> +	return (fifo->len + fifo->write - fifo->read) & (fifo->len - 1);
> +}
> +
> +/**
> + * Get the num of available elements in the fifo
> + */
> +static inline unsigned
> +kni_fifo_free_count(struct rte_kni_fifo *fifo)
> +{
> +	return (fifo->read - fifo->write - 1) & (fifo->len - 1);
> +}
> 

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

[Ferruh Yigit]

On 1/19/2017 4:46 AM, Sergey Vyazmitinov wrote:
> Optimized kni_allocate_mbufs and kni_free_mbufs by using mbuf bulk functions

Hi Sergey,

Although I found this patch and its idea useful, performance test result
is showing a performance lose after this patch.

In my test setup [1] the performance drops from ~1.88Mpps to ~1.6Mpps.
What are your test results, are you observing a performance gain?


To analyze the test result, I split the kni patch into two, -it may be
good idea to split these in patchset too-, bulk free and bulk alloc.

It looks like bulk free does not make any difference. This may be
because bottleneck is somewhere else.

On the other hand, bulk alloc is causing the performance drop, but not
mainly because of using bulk mbuf alloc, instead because of alloc logic
modification.

It seems cost of checking free space of the fifo is more than allocating
a fix number of mbuf, fill fifo until it gets full and free remaining mbufs.
I will add more comment into patch itself, and I have another idea,
which looks like giving better result.

Thanks,
ferruh

[1]
- lo_mode_fifo_skb
- single kthread, binded to a core
- single port, connected to traffic generator


> 
> Sergey Vyazmitinov (2):
>   kni: add bulk function to free mbufs
>   kni: Use bulk functions to allocate and free mbufs
> 
>  lib/librte_kni/rte_kni.c      | 47 +++++++++++++++++++----------------------
>  lib/librte_kni/rte_kni_fifo.h | 18 ++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h    | 49 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 89 insertions(+), 25 deletions(-)
>