[dpdk-dev] [PATCH] net/liquidio: do not touch mbuf initialized fields

[dpdk-dev] [PATCH] net/liquidio: do not touch mbuf initialized fields

[Shijith Thotton]

Avoid re-initializing of mbuf fields which are set while in pool.
Replaced lio_recv_buffer_alloc with rte_pktmbuf_alloc.

See commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool").

Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
---
 drivers/net/liquidio/lio_rxtx.c   | 37 ++++---------------------------------
 drivers/net/liquidio/lio_struct.h |  2 +-
 2 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/drivers/net/liquidio/lio_rxtx.c b/drivers/net/liquidio/lio_rxtx.c
index 9533015..2bbb893 100644
--- a/drivers/net/liquidio/lio_rxtx.c
+++ b/drivers/net/liquidio/lio_rxtx.c
@@ -81,28 +81,6 @@
 	lio_droq_reset_indices(droq);
 }
 
-static void *
-lio_recv_buffer_alloc(struct lio_device *lio_dev, int q_no)
-{
-	struct lio_droq *droq = lio_dev->droq[q_no];
-	struct rte_mempool *mpool = droq->mpool;
-	struct rte_mbuf *m;
-
-	m = rte_pktmbuf_alloc(mpool);
-	if (m == NULL) {
-		lio_dev_err(lio_dev, "Cannot allocate\n");
-		return NULL;
-	}
-
-	rte_mbuf_refcnt_set(m, 1);
-	m->next = NULL;
-	m->data_off = RTE_PKTMBUF_HEADROOM;
-	m->nb_segs = 1;
-	m->pool = mpool;
-
-	return m;
-}
-
 static int
 lio_droq_setup_ring_buffers(struct lio_device *lio_dev,
 			    struct lio_droq *droq)
@@ -112,7 +90,7 @@
 	void *buf;
 
 	for (i = 0; i < droq->max_count; i++) {
-		buf = lio_recv_buffer_alloc(lio_dev, droq->q_no);
+		buf = rte_pktmbuf_alloc(droq->mpool);
 		if (buf == NULL) {
 			lio_dev_err(lio_dev, "buffer alloc failed\n");
 			droq->stats.rx_alloc_failure++;
@@ -378,7 +356,6 @@
 
 /* lio_droq_refill
  *
- * @param lio_dev	- pointer to the lio device structure
  * @param droq		- droq in which descriptors require new buffers.
  *
  * Description:
@@ -394,7 +371,7 @@
  * This routine is called with droq->lock held.
  */
 static uint32_t
-lio_droq_refill(struct lio_device *lio_dev, struct lio_droq *droq)
+lio_droq_refill(struct lio_droq *droq)
 {
 	struct lio_droq_desc *desc_ring;
 	uint32_t desc_refilled = 0;
@@ -407,7 +384,7 @@
 		 * reuse the buffer, else allocate.
 		 */
 		if (droq->recv_buf_list[droq->refill_idx].buffer == NULL) {
-			buf = lio_recv_buffer_alloc(lio_dev, droq->q_no);
+			buf = rte_pktmbuf_alloc(droq->mpool);
 			/* If a buffer could not be allocated, no point in
 			 * continuing
 			 */
@@ -489,9 +466,6 @@
 			droq->refill_count++;
 
 			if (likely(nicbuf != NULL)) {
-				nicbuf->data_off = RTE_PKTMBUF_HEADROOM;
-				nicbuf->nb_segs = 1;
-				nicbuf->next = NULL;
 				/* We don't have a way to pass flags yet */
 				nicbuf->ol_flags = 0;
 				if (rh->r_dh.has_hash) {
@@ -545,9 +519,6 @@
 					if (!pkt_len)
 						first_buf = nicbuf;
 
-					nicbuf->data_off = RTE_PKTMBUF_HEADROOM;
-					nicbuf->nb_segs = 1;
-					nicbuf->next = NULL;
 					nicbuf->port = lio_dev->port_id;
 					/* We don't have a way to pass
 					 * flags yet
@@ -617,7 +588,7 @@
 	}
 
 	if (droq->refill_count >= droq->refill_threshold) {
-		int desc_refilled = lio_droq_refill(lio_dev, droq);
+		int desc_refilled = lio_droq_refill(droq);
 
 		/* Flush the droq descriptor data to memory to be sure
 		 * that when we update the credits the data in memory is
diff --git a/drivers/net/liquidio/lio_struct.h b/drivers/net/liquidio/lio_struct.h
index 26f803f..d9cbf00 100644
--- a/drivers/net/liquidio/lio_struct.h
+++ b/drivers/net/liquidio/lio_struct.h
@@ -94,7 +94,7 @@ struct lio_droq_stats {
 	/** Num of vxlan packets received; */
 	uint64_t rx_vxlan;
 
-	/** Num of failures of lio_recv_buffer_alloc() */
+	/** Num of failures of rte_pktmbuf_alloc() */
 	uint64_t rx_alloc_failure;
 
 };
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/liquidio: do not touch mbuf initialized fields

[Ferruh Yigit]

On 6/8/2017 12:22 PM, Shijith Thotton wrote:
> Avoid re-initializing of mbuf fields which are set while in pool.
> Replaced lio_recv_buffer_alloc with rte_pktmbuf_alloc.
> 
> See commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool").
> 
> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>

<...>

> @@ -489,9 +466,6 @@
>  			droq->refill_count++;
>  
>  			if (likely(nicbuf != NULL)) {
> -				nicbuf->data_off = RTE_PKTMBUF_HEADROOM;

Hi Shijith, Olivier,

nb_segs and next are initialized properly when mbuf allocated, but is
"data_off = RTE_PKTMBUF_HEADROOM" guaranteed?

> -				nicbuf->nb_segs = 1;
> -				nicbuf->next = NULL;
>  				/* We don't have a way to pass flags yet */
>  				nicbuf->ol_flags = 0;
>  				if (rh->r_dh.has_hash) {

<...>

Re: [dpdk-dev] [PATCH] net/liquidio: do not touch mbuf initialized fields

[Olivier Matz]

Hi Ferruh,

On Fri, 9 Jun 2017 12:24:56 +0100, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 6/8/2017 12:22 PM, Shijith Thotton wrote:
> > Avoid re-initializing of mbuf fields which are set while in pool.
> > Replaced lio_recv_buffer_alloc with rte_pktmbuf_alloc.
> > 
> > See commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool").
> > 
> > Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>  
> 
> <...>
> 
> > @@ -489,9 +466,6 @@
> >  			droq->refill_count++;
> >  
> >  			if (likely(nicbuf != NULL)) {
> > -				nicbuf->data_off = RTE_PKTMBUF_HEADROOM;  
> 
> Hi Shijith, Olivier,
> 
> nb_segs and next are initialized properly when mbuf allocated, but is
> "data_off = RTE_PKTMBUF_HEADROOM" guaranteed?
> 
> > -				nicbuf->nb_segs = 1;
> > -				nicbuf->next = NULL;
> >  				/* We don't have a way to pass flags yet */
> >  				nicbuf->ol_flags = 0;
> >  				if (rh->r_dh.has_hash) {  
> 
> <...>

It's not guaranteed when using a raw allocation, i.e. rte_mbuf_raw_alloc()
or rte_mempool_get(). The PMD usually use these functions in Rx path to
fully initialize the mbuf fields on their own.

In this driver, it looks it uses rte_pktmbuf_alloc(), which calls
rte_pktmbuf_reset(), fully initializing the mbuf struct. So data_off is
properly set.


As a side note, I wonder if lio_recv_buffer_alloc() replaced by
rte_pktmbuf_alloc() everywhere in the code.


Olivier

Re: [dpdk-dev] [PATCH] net/liquidio: do not touch mbuf initialized fields

[Ferruh Yigit]

On 6/8/2017 12:22 PM, Shijith Thotton wrote:
> Avoid re-initializing of mbuf fields which are set while in pool.
> Replaced lio_recv_buffer_alloc with rte_pktmbuf_alloc.
> 
> See commit 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool").
> 
> Signed-off-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>

Applied to dpdk-next-net/master, thanks.