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

[Suanming Mou <suanmingm@nvidia.com>]

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