[dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[chunmei Liu]

From: chunmei <chunmei.liu@intel.com>

ceph async messenger has some run time error with this dpdk library,
1) need set rxm->next= null at end of packet since not init it to null
when allocate a buffer other wise rte_mbuf_sanity_check will report error.
2) when check the size, can't calculate mbuf_data_room_size
because async messenger dpdk will allocate this part later
not at create mempool.

Signed-off-by: chunmei Liu <chunmei.liu@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 3 ++-
 lib/librte_mbuf/rte_mbuf.c     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 1e07895..918946b 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2190,7 +2190,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
 			rxm->next = next_rxe->mbuf;
 			next_sc_entry->fbuf = first_seg;
 			goto next_desc;
-		}
+		} else
+			rxm->next = NULL;
 
 		/* Initialize the first mbuf of the returned packet */
 		ixgbe_fill_cluster_head_buf(first_seg, &rxd, rxq, staterr);
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 0e3e36a..c01da19 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -102,7 +102,6 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	}
 
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
-		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
 
 	mbp_priv = rte_mempool_get_priv(mp);
@@ -233,6 +232,8 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
 	}
 	if (nb_segs != 0)
 		rte_panic("bad nb_segs\n");
+	if (m_seg != NULL)
+		rte_panic("bad m_seg\n");
 }
 
 /* dump a mbuf on console */
-- 
2.7.4

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Thomas Monjalon]

14/12/2017 00:44, chunmei Liu:
> From: chunmei <chunmei.liu@intel.com>
> 
> ceph async messenger has some run time error with this dpdk library,
> 1) need set rxm->next= null at end of packet since not init it to null
> when allocate a buffer other wise rte_mbuf_sanity_check will report error.
> 2) when check the size, can't calculate mbuf_data_room_size
> because async messenger dpdk will allocate this part later
> not at create mempool.
> 
> Signed-off-by: chunmei Liu <chunmei.liu@intel.com>

Hi Chunmei,

Several comments about the formatting:

I've already notified you that you should Cc Olivier for mbuf changes.
You should also Cc Wenzhuo and Konstantin for ixgbe change.
Please add them in your next emails.

The title should show what is fixed, not the use-case.

When sending a new revision of the patch, you should increment
the version number and add a changelog below the ---.

Please use --in-reply-to when sending a new version, in order
to keep track of all versions in the same thread.

Your author name and SoB name should be the same
(your full name with uppercases):
	Chunmei Liu <chunmei.liu@intel.com>

For more informations, you should read this:
	http://dpdk.org/dev#send

Thank you

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Olivier MATZ]

Hi Chunmei,

On Wed, Dec 13, 2017 at 03:44:55PM -0800, chunmei Liu wrote:
> From: chunmei <chunmei.liu@intel.com>
> 
> ceph async messenger has some run time error with this dpdk library,
> 1) need set rxm->next= null at end of packet since not init it to null
> when allocate a buffer other wise rte_mbuf_sanity_check will report error.
> 2) when check the size, can't calculate mbuf_data_room_size
> because async messenger dpdk will allocate this part later
> not at create mempool.
> 
> Signed-off-by: chunmei Liu <chunmei.liu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c | 3 ++-
>  lib/librte_mbuf/rte_mbuf.c     | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 1e07895..918946b 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2190,7 +2190,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>  			rxm->next = next_rxe->mbuf;
>  			next_sc_entry->fbuf = first_seg;
>  			goto next_desc;
> -		}
> +		} else
> +			rxm->next = NULL;
>  
>  		/* Initialize the first mbuf of the returned packet */
>  		ixgbe_fill_cluster_head_buf(first_seg, &rxd, rxq, staterr);
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 0e3e36a..c01da19 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -102,7 +102,6 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
>  	}
>  
>  	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
> -		user_mbp_priv->mbuf_data_room_size +
>  		user_mbp_priv->mbuf_priv_size);
>  
>  	mbp_priv = rte_mempool_get_priv(mp);

I don't see the point here.
Many parts of dpdk rely on this code without issue. I enabled the
asserts and see no issue with testpmd. Can you give more details?


> @@ -233,6 +232,8 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
>  	}
>  	if (nb_segs != 0)
>  		rte_panic("bad nb_segs\n");
> +	if (m_seg != NULL)
> +		rte_panic("bad m_seg\n");
>  }

A similar check was recently sumbitted in another patch. Please see:
https://dpdk.org/dev/patchwork/patch/32057/

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Ananyev, Konstantin]

Hi Chunmei,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of chunmei Liu
> Sent: Wednesday, December 13, 2017 11:45 PM
> To: dev@dpdk.org
> Cc: Liu, Chunmei <chunmei.liu@intel.com>
> Subject: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk
> 
> From: chunmei <chunmei.liu@intel.com>
> 
> ceph async messenger has some run time error with this dpdk library,
> 1) need set rxm->next= null at end of packet since not init it to null
> when allocate a buffer other wise rte_mbuf_sanity_check will report error.
> 2) when check the size, can't calculate mbuf_data_room_size
> because async messenger dpdk will allocate this part later
> not at create mempool.
> 
> Signed-off-by: chunmei Liu <chunmei.liu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c | 3 ++-
>  lib/librte_mbuf/rte_mbuf.c     | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 1e07895..918946b 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2190,7 +2190,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>  			rxm->next = next_rxe->mbuf;
>  			next_sc_entry->fbuf = first_seg;
>  			goto next_desc;
> -		}
> +		} else
> +			rxm->next = NULL;

Shouldn't be a problem I think, but wonder why do we need it at all?
As I remember, rte_pktmbuf_free_seg() always sets m->next to NULL.
Konstantin 

> 
>  		/* Initialize the first mbuf of the returned packet */
>  		ixgbe_fill_cluster_head_buf(first_seg, &rxd, rxq, staterr);
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 0e3e36a..c01da19 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -102,7 +102,6 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
>  	}
> 
>  	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
> -		user_mbp_priv->mbuf_data_room_size +
>  		user_mbp_priv->mbuf_priv_size);
> 
>  	mbp_priv = rte_mempool_get_priv(mp);
> @@ -233,6 +232,8 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
>  	}
>  	if (nb_segs != 0)
>  		rte_panic("bad nb_segs\n");
> +	if (m_seg != NULL)
> +		rte_panic("bad m_seg\n");
>  }
> 
>  /* dump a mbuf on console */
> --
> 2.7.4

[dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[chunmei Liu]

From: chunmei <chunmei.liu@intel.com>

ceph async messenger has some run time error with this dpdk library,
1) need set rxm->next= null at end of packet since not init it to null
when allocate a buffer other wise rte_mbuf_sanity_check will report error.
2) when check the size, can't calculate mbuf_data_room_size
because async messenger dpdk will allocate this part later
not at create mempool.

Signed-off-by: chunmei Liu <chunmei.liu@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 3 ++-
 lib/librte_mbuf/rte_mbuf.c     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 1e07895..a0acc5c 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2190,7 +2190,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
 			rxm->next = next_rxe->mbuf;
 			next_sc_entry->fbuf = first_seg;
 			goto next_desc;
-		}
+		}else
+			rxm->next = NULL;
 
 		/* Initialize the first mbuf of the returned packet */
 		ixgbe_fill_cluster_head_buf(first_seg, &rxd, rxq, staterr);
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 0e3e36a..c01da19 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -102,7 +102,6 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	}
 
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
-		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
 
 	mbp_priv = rte_mempool_get_priv(mp);
@@ -233,6 +232,8 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
 	}
 	if (nb_segs != 0)
 		rte_panic("bad nb_segs\n");
+	if (m_seg != NULL)
+		rte_panic("bad m_seg\n");
 }
 
 /* dump a mbuf on console */
-- 
2.7.4

[dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[chunmei Liu]

From: chunmei <chunmei.liu@intel.com>

ceph async messenger has some run time error with this dpdk library,
1) need init mb->next= null when allocate a buffer other wise
rte_mbuf_sanity_check will report error.
2) when check the size, can't calculate mbuf_data_room_size
because async messenger dpdk will allocate this part later
not at create mempool.

Signed-off-by: chunmei Liu <chunmei.liu@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 1 +
 lib/librte_mbuf/rte_mbuf.c     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 1e07895..30fdbc1 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1589,6 +1589,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
 		}
 
 		rte_mbuf_refcnt_set(mb, 1);
+		mb->next = NULL;
 		mb->data_off = RTE_PKTMBUF_HEADROOM;
 
 		/* populate the descriptors */
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 0e3e36a..c01da19 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -102,7 +102,6 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	}
 
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
-		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
 
 	mbp_priv = rte_mempool_get_priv(mp);
@@ -233,6 +232,8 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
 	}
 	if (nb_segs != 0)
 		rte_panic("bad nb_segs\n");
+	if (m_seg != NULL)
+		rte_panic("bad m_seg\n");
 }
 
 /* dump a mbuf on console */
-- 
2.7.4

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Thomas Monjalon]

13/12/2017 20:32, chunmei Liu:
>  drivers/net/ixgbe/ixgbe_rxtx.c | 1 +
>  lib/librte_mbuf/rte_mbuf.c     | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)

+Cc Olivier

Please note that such a change in lib/librte_mbuf/ cannot be applied
without the approval of Olivier Matz.

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Stephen Hemminger]

On Wed, 13 Dec 2017 11:32:32 -0800
chunmei Liu <chunmei.liu@intel.com> wrote:

> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 1e07895..30fdbc1 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1589,6 +1589,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
>  		}
>  
>  		rte_mbuf_refcnt_set(mb, 1);
> +		mb->next = NULL;
>  		mb->data_off = RTE_PKTMBUF_HEADROOM;
>  

This will have measurable performance impact because the next pointer is
on the second cache line.

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Liu, Chunmei]

But if not initialize allocated mbuf->next = NULL, when do ip package assemble, the last mbuf->next maybe not be null, all the logical has error, the application use null pointer determine a packaged end. DPDK NIC doesn't work now.

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
Sent: Wednesday, December 13, 2017 1:31 PM
To: Liu, Chunmei <chunmei.liu@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

On Wed, 13 Dec 2017 11:32:32 -0800
chunmei Liu <chunmei.liu@intel.com> wrote:

> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c 
> b/drivers/net/ixgbe/ixgbe_rxtx.c index 1e07895..30fdbc1 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1589,6 +1589,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
>  		}
>  
>  		rte_mbuf_refcnt_set(mb, 1);
> +		mb->next = NULL;
>  		mb->data_off = RTE_PKTMBUF_HEADROOM;
>  

This will have measurable performance impact because the next pointer is on the second cache line.

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Liu, Chunmei]

Hold on, Let me try another place to fix this issue. 

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
Sent: Wednesday, December 13, 2017 1:31 PM
To: Liu, Chunmei <chunmei.liu@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

On Wed, 13 Dec 2017 11:32:32 -0800
chunmei Liu <chunmei.liu@intel.com> wrote:

> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c 
> b/drivers/net/ixgbe/ixgbe_rxtx.c index 1e07895..30fdbc1 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1589,6 +1589,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
>  		}
>  
>  		rte_mbuf_refcnt_set(mb, 1);
> +		mb->next = NULL;
>  		mb->data_off = RTE_PKTMBUF_HEADROOM;
>  

This will have measurable performance impact because the next pointer is on the second cache line.

Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[Stephen Hemminger]

On Wed, 13 Dec 2017 22:46:30 +0000
"Liu, Chunmei" <chunmei.liu@intel.com> wrote:

> Hold on, Let me try another place to fix this issue. 
> 
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org] 
> Sent: Wednesday, December 13, 2017 1:31 PM
> To: Liu, Chunmei <chunmei.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk
> 
> On Wed, 13 Dec 2017 11:32:32 -0800
> chunmei Liu <chunmei.liu@intel.com> wrote:
> 
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c 
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index 1e07895..30fdbc1 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -1589,6 +1589,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
> >  		}
> >  
> >  		rte_mbuf_refcnt_set(mb, 1);
> > +		mb->next = NULL;
> >  		mb->data_off = RTE_PKTMBUF_HEADROOM;
> >    
> 
> This will have measurable performance impact because the next pointer is on the second cache line.

Please do not do top posting on open source mailing lists.

[dpdk-dev] [PATCH] fix ceph async messenger runtime bug based on spdk/dpdk

[chunmei Liu]

From: chunmei <chunmei.liu@intel.com>

ceph async messenger has some run time error with this dpdk library,
1) need init mb->next= null when allocate a buffer other wise
rte_mbuf_sanity_check will report error.
2) when check the size, can't calculate mbuf_data_room_size
because async messenger dpdk will allocate this part later
not at create mempool.
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 1 +
 lib/librte_mbuf/rte_mbuf.c     | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 1e07895..30fdbc1 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1589,6 +1589,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
 		}
 
 		rte_mbuf_refcnt_set(mb, 1);
+		mb->next = NULL;
 		mb->data_off = RTE_PKTMBUF_HEADROOM;
 
 		/* populate the descriptors */
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 0e3e36a..2b1a3d1 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -102,7 +102,6 @@ rte_pktmbuf_pool_init(struct rte_mempool *mp, void *opaque_arg)
 	}
 
 	RTE_ASSERT(mp->elt_size >= sizeof(struct rte_mbuf) +
-		user_mbp_priv->mbuf_data_room_size +
 		user_mbp_priv->mbuf_priv_size);
 
 	mbp_priv = rte_mempool_get_priv(mp);
@@ -233,6 +232,9 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header)
 	}
 	if (nb_segs != 0)
 		rte_panic("bad nb_segs\n");
+ 	if (m_seg != NULL)
+		rte_panic("bad m_seg\n");
+
 }
 
 /* dump a mbuf on console */
-- 
2.7.4