[PATCH] app/test-crypto-perf: fix invalid mbuf next operation

[PATCH] app/test-crypto-perf: fix invalid mbuf next operation

[Suanming Mou]

In fill_multi_seg_mbuf(), when remaining_segments is 0,
rte_mbuf m's next should pointer to NULL instead of a
new rte_mbuf, that casues setting m->next as NULL out
of the while loop to the invalid mbuf.

This commit fixes the invalid mbuf next operation.

Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---
 app/test-crypto-perf/cperf_test_common.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/app/test-crypto-perf/cperf_test_common.c b/app/test-crypto-perf/cperf_test_common.c
index 932aab16df..ad2076dd2e 100644
--- a/app/test-crypto-perf/cperf_test_common.c
+++ b/app/test-crypto-perf/cperf_test_common.c
@@ -72,13 +72,15 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		rte_mbuf_refcnt_set(m, 1);
 		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
 					mbuf_hdr_size + segment_sz);
-		m->next = next_mbuf;
-		m = next_mbuf;
-		remaining_segments--;
 
+		remaining_segments--;
+		if (remaining_segments > 0) {
+			m->next = next_mbuf;
+			m = next_mbuf;
+		} else {
+			m->next = NULL;
+		}
 	} while (remaining_segments > 0);
-
-	m->next = NULL;
 }
 
 static void
-- 
2.34.1

RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation

[Anoob Joseph]

Hi Suanming,

Good catch. Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Wednesday, January 3, 2024 9:24 AM
> To: Ciara Power <ciara.power@intel.com>
> Cc: dev@dpdk.org
> Subject: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation
> 
> External Email
> 
> ----------------------------------------------------------------------
> In fill_multi_seg_mbuf(), when remaining_segments is 0, rte_mbuf m's next
> should pointer to NULL instead of a new rte_mbuf, that casues setting m->next
> as NULL out of the while loop to the invalid mbuf.
> 
> This commit fixes the invalid mbuf next operation.
> 
> Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> ---
>  app/test-crypto-perf/cperf_test_common.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-crypto-perf/cperf_test_common.c b/app/test-crypto-
> perf/cperf_test_common.c
> index 932aab16df..ad2076dd2e 100644
> --- a/app/test-crypto-perf/cperf_test_common.c
> +++ b/app/test-crypto-perf/cperf_test_common.c
> @@ -72,13 +72,15 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct
> rte_mempool *mp,
>  		rte_mbuf_refcnt_set(m, 1);
>  		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
>  					mbuf_hdr_size + segment_sz);
> -		m->next = next_mbuf;
> -		m = next_mbuf;
> -		remaining_segments--;
> 
> +		remaining_segments--;
> +		if (remaining_segments > 0) {

[Anoob] Would it make sense to move assignment of next_mbuf also to here? That way, the checks will become self explanatory.
  		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
  					mbuf_hdr_size + segment_sz);

> +			m->next = next_mbuf;
> +			m = next_mbuf;
> +		} else {
> +			m->next = NULL;
> +		}
>  	} while (remaining_segments > 0);
> -
> -	m->next = NULL;
>  }
> 
>  static void
> --
> 2.34.1

RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation

[Suanming Mou]

Hi,

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday, January 3, 2024 7:22 PM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: dev@dpdk.org; Ciara Power <ciara.power@intel.com>
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation
> 
> Hi Suanming,
> 
> Good catch. Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Suanming Mou <suanmingm@nvidia.com>
> > Sent: Wednesday, January 3, 2024 9:24 AM
> > To: Ciara Power <ciara.power@intel.com>
> > Cc: dev@dpdk.org
> > Subject: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next
> > operation
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > In fill_multi_seg_mbuf(), when remaining_segments is 0, rte_mbuf m's
> > next should pointer to NULL instead of a new rte_mbuf, that casues
> > setting m->next as NULL out of the while loop to the invalid mbuf.
> >
> > This commit fixes the invalid mbuf next operation.
> >
> > Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > ---
> >  app/test-crypto-perf/cperf_test_common.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/app/test-crypto-perf/cperf_test_common.c
> > b/app/test-crypto- perf/cperf_test_common.c index
> > 932aab16df..ad2076dd2e 100644
> > --- a/app/test-crypto-perf/cperf_test_common.c
> > +++ b/app/test-crypto-perf/cperf_test_common.c
> > @@ -72,13 +72,15 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct
> > rte_mempool *mp,
> >  		rte_mbuf_refcnt_set(m, 1);
> >  		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
> >  					mbuf_hdr_size + segment_sz);
> > -		m->next = next_mbuf;
> > -		m = next_mbuf;
> > -		remaining_segments--;
> >
> > +		remaining_segments--;
> > +		if (remaining_segments > 0) {
> 
> [Anoob] Would it make sense to move assignment of next_mbuf also to here?
> That way, the checks will become self explanatory.
>   		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
>   					mbuf_hdr_size + segment_sz);
> 

Make sense. Maybe just like that:
  		m->next = (struct rte_mbuf *) ((uint8_t *) m +
  					mbuf_hdr_size + segment_sz);
		m = m->next;

What do you think?

> > +			m->next = next_mbuf;
> > +			m = next_mbuf;
> > +		} else {
> > +			m->next = NULL;
> > +		}
> >  	} while (remaining_segments > 0);
> > -
> > -	m->next = NULL;
> >  }
> >
> >  static void
> > --
> > 2.34.1

RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation

[Anoob Joseph]

Hi Suanming,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Wednesday, January 3, 2024 6:06 PM
> To: Anoob Joseph <anoobj@marvell.com>
> Cc: dev@dpdk.org; Ciara Power <ciara.power@intel.com>
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation
> 
> Hi,
> 
> > -----Original Message-----
> > From: Anoob Joseph <anoobj@marvell.com>
> > Sent: Wednesday, January 3, 2024 7:22 PM
> > To: Suanming Mou <suanmingm@nvidia.com>
> > Cc: dev@dpdk.org; Ciara Power <ciara.power@intel.com>
> > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next
> > operation
> >
> > Hi Suanming,
> >
> > Good catch. Please see inline.
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: Suanming Mou <suanmingm@nvidia.com>
> > > Sent: Wednesday, January 3, 2024 9:24 AM
> > > To: Ciara Power <ciara.power@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next
> > > operation
> > >
> > > External Email
> > >
> > > --------------------------------------------------------------------
> > > -- In fill_multi_seg_mbuf(), when remaining_segments is 0, rte_mbuf
> > > m's next should pointer to NULL instead of a new rte_mbuf, that
> > > casues setting m->next as NULL out of the while loop to the invalid
> > > mbuf.
> > >
> > > This commit fixes the invalid mbuf next operation.
> > >
> > > Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> > >
> > > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > > ---
> > >  app/test-crypto-perf/cperf_test_common.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/app/test-crypto-perf/cperf_test_common.c
> > > b/app/test-crypto- perf/cperf_test_common.c index
> > > 932aab16df..ad2076dd2e 100644
> > > --- a/app/test-crypto-perf/cperf_test_common.c
> > > +++ b/app/test-crypto-perf/cperf_test_common.c
> > > @@ -72,13 +72,15 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct
> > > rte_mempool *mp,
> > >  		rte_mbuf_refcnt_set(m, 1);
> > >  		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
> > >  					mbuf_hdr_size + segment_sz);
> > > -		m->next = next_mbuf;
> > > -		m = next_mbuf;
> > > -		remaining_segments--;
> > >
> > > +		remaining_segments--;
> > > +		if (remaining_segments > 0) {
> >
> > [Anoob] Would it make sense to move assignment of next_mbuf also to here?
> > That way, the checks will become self explanatory.
> >   		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
> >   					mbuf_hdr_size + segment_sz);
> >
> 
> Make sense. Maybe just like that:
>   		m->next = (struct rte_mbuf *) ((uint8_t *) m +
>   					mbuf_hdr_size + segment_sz);
> 		m = m->next;
> 
> What do you think?

[Anoob] Yes. That's even better. 

I think we can have line lengths upto 100 characters now. In case you find it easier to put in single line.

> 
> > > +			m->next = next_mbuf;
> > > +			m = next_mbuf;
> > > +		} else {
> > > +			m->next = NULL;
> > > +		}
> > >  	} while (remaining_segments > 0);
> > > -
> > > -	m->next = NULL;
> > >  }
> > >
> > >  static void
> > > --
> > > 2.34.1

RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation

[Suanming Mou]

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday, January 3, 2024 11:43 PM
> To: Suanming Mou <suanmingm@nvidia.com>
> Cc: dev@dpdk.org; Ciara Power <ciara.power@intel.com>
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next operation
> 
> Hi Suanming,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Suanming Mou <suanmingm@nvidia.com>
> > Sent: Wednesday, January 3, 2024 6:06 PM
> > To: Anoob Joseph <anoobj@marvell.com>
> > Cc: dev@dpdk.org; Ciara Power <ciara.power@intel.com>
> > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next
> > operation
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Anoob Joseph <anoobj@marvell.com>
> > > Sent: Wednesday, January 3, 2024 7:22 PM
> > > To: Suanming Mou <suanmingm@nvidia.com>
> > > Cc: dev@dpdk.org; Ciara Power <ciara.power@intel.com>
> > > Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf
> > > next operation
> > >
> > > Hi Suanming,
> > >
> > > Good catch. Please see inline.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > > -----Original Message-----
> > > > From: Suanming Mou <suanmingm@nvidia.com>
> > > > Sent: Wednesday, January 3, 2024 9:24 AM
> > > > To: Ciara Power <ciara.power@intel.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next
> > > > operation
> > > >
> > > > External Email
> > > >
> > > > ------------------------------------------------------------------
> > > > --
> > > > -- In fill_multi_seg_mbuf(), when remaining_segments is 0,
> > > > rte_mbuf m's next should pointer to NULL instead of a new
> > > > rte_mbuf, that casues setting m->next as NULL out of the while
> > > > loop to the invalid mbuf.
> > > >
> > > > This commit fixes the invalid mbuf next operation.
> > > >
> > > > Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> > > >
> > > > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > > > ---
> > > >  app/test-crypto-perf/cperf_test_common.c | 12 +++++++-----
> > > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/app/test-crypto-perf/cperf_test_common.c
> > > > b/app/test-crypto- perf/cperf_test_common.c index
> > > > 932aab16df..ad2076dd2e 100644
> > > > --- a/app/test-crypto-perf/cperf_test_common.c
> > > > +++ b/app/test-crypto-perf/cperf_test_common.c
> > > > @@ -72,13 +72,15 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct
> > > > rte_mempool *mp,
> > > >  		rte_mbuf_refcnt_set(m, 1);
> > > >  		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
> > > >  					mbuf_hdr_size + segment_sz);
> > > > -		m->next = next_mbuf;
> > > > -		m = next_mbuf;
> > > > -		remaining_segments--;
> > > >
> > > > +		remaining_segments--;
> > > > +		if (remaining_segments > 0) {
> > >
> > > [Anoob] Would it make sense to move assignment of next_mbuf also to here?
> > > That way, the checks will become self explanatory.
> > >   		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
> > >   					mbuf_hdr_size + segment_sz);
> > >
> >
> > Make sense. Maybe just like that:
> >   		m->next = (struct rte_mbuf *) ((uint8_t *) m +
> >   					mbuf_hdr_size + segment_sz);
> > 		m = m->next;
> >
> > What do you think?
> 
> [Anoob] Yes. That's even better.
> 
> I think we can have line lengths upto 100 characters now. In case you find it
> easier to put in single line.

OK, thanks for the suggestion.

> 
> >
> > > > +			m->next = next_mbuf;
> > > > +			m = next_mbuf;
> > > > +		} else {
> > > > +			m->next = NULL;
> > > > +		}
> > > >  	} while (remaining_segments > 0);
> > > > -
> > > > -	m->next = NULL;
> > > >  }
> > > >
> > > >  static void
> > > > --
> > > > 2.34.1

[PATCH v2] app/test-crypto-perf: fix invalid mbuf next operation

[Suanming Mou]

In fill_multi_seg_mbuf(), when remaining_segments is 0,
rte_mbuf m's next should pointer to NULL instead of a
new rte_mbuf, that causes setting m->next as NULL out
of the while loop to the invalid mbuf.

This commit fixes the invalid mbuf next operation.

Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")

Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---

v2: move next_mbuf inside remaining_segments check.

---
 app/test-crypto-perf/cperf_test_common.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/app/test-crypto-perf/cperf_test_common.c b/app/test-crypto-perf/cperf_test_common.c
index 932aab16df..b3bf9f67e8 100644
--- a/app/test-crypto-perf/cperf_test_common.c
+++ b/app/test-crypto-perf/cperf_test_common.c
@@ -49,7 +49,6 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 {
 	uint16_t mbuf_hdr_size = sizeof(struct rte_mbuf);
 	uint16_t remaining_segments = segments_nb;
-	struct rte_mbuf *next_mbuf;
 	rte_iova_t next_seg_phys_addr = rte_mempool_virt2iova(obj) +
 			 mbuf_offset + mbuf_hdr_size;
 
@@ -70,15 +69,15 @@ fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
 		m->nb_segs = segments_nb;
 		m->port = 0xff;
 		rte_mbuf_refcnt_set(m, 1);
-		next_mbuf = (struct rte_mbuf *) ((uint8_t *) m +
-					mbuf_hdr_size + segment_sz);
-		m->next = next_mbuf;
-		m = next_mbuf;
-		remaining_segments--;
 
+		remaining_segments--;
+		if (remaining_segments > 0) {
+			m->next = (struct rte_mbuf *)((uint8_t *) m + mbuf_hdr_size + segment_sz);
+			m = m->next;
+		} else {
+			m->next = NULL;
+		}
 	} while (remaining_segments > 0);
-
-	m->next = NULL;
 }
 
 static void
-- 
2.34.1

RE: [EXT] [PATCH v2] app/test-crypto-perf: fix invalid mbuf next operation

[Anoob Joseph]

> In fill_multi_seg_mbuf(), when remaining_segments is 0, rte_mbuf m's next
> should pointer to NULL instead of a new rte_mbuf, that causes setting m->next
> as NULL out of the while loop to the invalid mbuf.
> 
> This commit fixes the invalid mbuf next operation.
> 
> Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>

Acked-by: Anoob Joseph <anoobj@marvell.com>

RE: [PATCH v2] app/test-crypto-perf: fix invalid mbuf next operation

[Power, Ciara]

> -----Original Message-----
> From: Suanming Mou <suanmingm@nvidia.com>
> Sent: Thursday, January 4, 2024 2:24 AM
> To: anoobj@marvell.com; Power, Ciara <ciara.power@intel.com>
> Cc: dev@dpdk.org
> Subject: [PATCH v2] app/test-crypto-perf: fix invalid mbuf next operation
> 
> In fill_multi_seg_mbuf(), when remaining_segments is 0, rte_mbuf m's next
> should pointer to NULL instead of a new rte_mbuf, that causes setting m-
> >next as NULL out of the while loop to the invalid mbuf.
> 
> This commit fixes the invalid mbuf next operation.
> 
> Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> ---
> 
> v2: move next_mbuf inside remaining_segments check.
> 
> ---
>  app/test-crypto-perf/cperf_test_common.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 

Acked-by: Ciara Power <ciara.power@intel.com>

RE: [EXT] [PATCH v2] app/test-crypto-perf: fix invalid mbuf next operation

[Akhil Goyal]

> In fill_multi_seg_mbuf(), when remaining_segments is 0,
> rte_mbuf m's next should pointer to NULL instead of a
> new rte_mbuf, that causes setting m->next as NULL out
> of the while loop to the invalid mbuf.
> 
> This commit fixes the invalid mbuf next operation.
> 
> Fixes: bf9d6702eca9 ("app/crypto-perf: use single mempool")
> 
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
Cc: stable@dpdk.org
Applied to dpdk-next-crypto
Thanks.