Hi Suanming,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Suanming Mou <suanmi...@nvidia.com>
> Sent: Wednesday, January 3, 2024 6:06 PM
> To: Anoob Joseph <ano...@marvell.com>
> Cc: dev@dpdk.org; Ciara Power <ciara.po...@intel.com>
> Subject: RE: [EXT] [PATCH] app/test-crypto-perf: fix invalid mbuf next
> operation
>
> Hi,
>
> > -----Original Message-----
> > From: Anoob Joseph <ano...@marvell.com>
> > Sent: Wednesday, January 3, 2024 7:22 PM
> > To: Suanming Mou <suanmi...@nvidia.com>
> > Cc: dev@dpdk.org; Ciara Power <ciara.po...@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 <suanmi...@nvidia.com>
> > > Sent: Wednesday, January 3, 2024 9:24 AM
> > > To: Ciara Power <ciara.po...@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 <suanmi...@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
