[dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[David Coyle]

Set the source mbuf data and packet lengths correctly for DOCSIS
performance tests.

Fixes: d4a131a9498d ("test/crypto-perf: support DOCSIS protocol")

Signed-off-by: David Coyle <david.coyle@intel.com>
---
 app/test-crypto-perf/cperf_ops.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
index f851509ec..3da835a9c 100644
--- a/app/test-crypto-perf/cperf_ops.c
+++ b/app/test-crypto-perf/cperf_ops.c
@@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op **ops,
 			} else
 				buf_sz = options->test_buffer_size;
 
+			sym_op->m_src->buf_len = options->segment_sz;
+			sym_op->m_src->data_len = buf_sz;
+			sym_op->m_src->pkt_len = buf_sz;
+
 			/* DOCSIS header is not CRC'ed */
 			sym_op->auth.data.offset = options->docsis_hdr_sz;
 			sym_op->auth.data.length = buf_sz -
-- 
2.17.1

Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[De Lara Guarch, Pablo]

Hi David,

> -----Original Message-----
> From: Coyle, David <david.coyle@intel.com>
> Sent: Thursday, July 16, 2020 4:31 PM
> To: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; De
> Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>
> Cc: dev@dpdk.org; Ryan, Brendan <brendan.ryan@intel.com>; O'loingsigh,
> Mairtin <mairtin.oloingsigh@intel.com>; Coyle, David <david.coyle@intel.com>
> Subject: [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
> 
> Set the source mbuf data and packet lengths correctly for DOCSIS performance
> tests.
> 
> Fixes: d4a131a9498d ("test/crypto-perf: support DOCSIS protocol")
> 
> Signed-off-by: David Coyle <david.coyle@intel.com>
> ---
>  app/test-crypto-perf/cperf_ops.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
> index f851509ec..3da835a9c 100644
> --- a/app/test-crypto-perf/cperf_ops.c
> +++ b/app/test-crypto-perf/cperf_ops.c
> @@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op **ops,
>  			} else
>  				buf_sz = options->test_buffer_size;
> 
> +			sym_op->m_src->buf_len = options->segment_sz;
> +			sym_op->m_src->data_len = buf_sz;
> +			sym_op->m_src->pkt_len = buf_sz;
> +

Actually, I am wondering why this is needed at all (for DOCSIS and PDCP). This is already set in " fill_multi_seg_mbuf" or " fill_single_seg_mbuf" (and this was already working without this patch, right?).

Regards,
Pablo

Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[Coyle, David]

Hi Pablo,

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Friday, July 17, 2020 8:04 PM
> > @@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op **ops,
> >  			} else
> >  				buf_sz = options->test_buffer_size;
> >
> > +			sym_op->m_src->buf_len = options->segment_sz;
> > +			sym_op->m_src->data_len = buf_sz;
> > +			sym_op->m_src->pkt_len = buf_sz;
> > +
> 
> Actually, I am wondering why this is needed at all (for DOCSIS and PDCP). This
> is already set in " fill_multi_seg_mbuf" or " fill_single_seg_mbuf" (and this
> was already working without this patch, right?).

[DC] I have found that if a number of buffer sizes are specified like this on the
cmd line "--buffer-sz 64,256,1024", then the pkt_len and data_len filled in
"fill_multi_seg_mbuf" or " fill_single_seg_mbuf" is always the largest of the
sizes specified. The cipher/auth lengths are then set based on the --buffer-sz
option.

For DOCSIS, I tried to be more accurate and set the correct pkt_len and data_len
in the mbuf. This followed what PDCP did too, even though I'm not sure of the
background why PDCP did it - possibly spotted the same issue. I have also found
that DOCSIS performance figures can be better if the correct pkt_len and data_len
are set in the mbuf - I don't have any proper explanation for this though as the cipher/
auth lengths are always the same.

I've dug around a bit more on this now though and this is actually a problem across
the perf tool. Some of the crypto PMDs have logic based on the mbuf pkt_len
and data_len, but because the perf tool isn't always setting these fields correctly,
that logic may not work as expected.
> 

Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[De Lara Guarch, Pablo]

Hi David,

> -----Original Message-----
> From: Coyle, David <david.coyle@intel.com>
> Sent: Monday, July 20, 2020 1:59 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; Trahe,
> Fiona <fiona.trahe@intel.com>
> Cc: dev@dpdk.org; Ryan, Brendan <brendan.ryan@intel.com>; O'loingsigh,
> Mairtin <mairtin.oloingsigh@intel.com>
> Subject: RE: [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS
> tests
> 
> Hi Pablo,
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > Sent: Friday, July 17, 2020 8:04 PM
> > > @@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op **ops,
> > >  			} else
> > >  				buf_sz = options->test_buffer_size;
> > >
> > > +			sym_op->m_src->buf_len = options->segment_sz;
> > > +			sym_op->m_src->data_len = buf_sz;
> > > +			sym_op->m_src->pkt_len = buf_sz;
> > > +
> >
> > Actually, I am wondering why this is needed at all (for DOCSIS and
> > PDCP). This is already set in " fill_multi_seg_mbuf" or "
> > fill_single_seg_mbuf" (and this was already working without this patch, right?).
> 
> [DC] I have found that if a number of buffer sizes are specified like this on the
> cmd line "--buffer-sz 64,256,1024", then the pkt_len and data_len filled in
> "fill_multi_seg_mbuf" or " fill_single_seg_mbuf" is always the largest of the sizes
> specified. The cipher/auth lengths are then set based on the --buffer-sz option.
> 
> For DOCSIS, I tried to be more accurate and set the correct pkt_len and data_len
> in the mbuf. This followed what PDCP did too, even though I'm not sure of the
> background why PDCP did it - possibly spotted the same issue. I have also found
> that DOCSIS performance figures can be better if the correct pkt_len and
> data_len are set in the mbuf - I don't have any proper explanation for this
> though as the cipher/ auth lengths are always the same.
> 
> I've dug around a bit more on this now though and this is actually a problem
> across the perf tool. Some of the crypto PMDs have logic based on the mbuf
> pkt_len and data_len, but because the perf tool isn't always setting these fields
> correctly, that logic may not work as expected.
> >

Right, thanks for checking this. If I remember correctly, it was fine to have this set to the maximum size as the important field for crypto PMDs to check is the cipher/auth lengths, as you said. If there is more logic that depends on data_len on other PMDs, I agree it might be a problem. The only usage I knew for it was the multi segment case (in AES-GCM PMD), where data_len is checked in each segment size to see if all the cipher/auth length resides within these segments, but in the tool we set data_len for each segment when "going multi-segment". I see that other PMDs like DPAA2_SEC use these fields for something which I am not sure what's for. It would be good if the maintainers check if this is a problem for them, and in that case, this should be fixed for the other functions (for "normal" crypto).

Thanks,
Pablo

Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[Akhil Goyal]

Hi Pablo/David,
> 
> Hi David,
> 
> > Hi Pablo,
> >
> > > -----Original Message-----
> > > From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > > Sent: Friday, July 17, 2020 8:04 PM
> > > > @@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op **ops,
> > > >  			} else
> > > >  				buf_sz = options->test_buffer_size;
> > > >
> > > > +			sym_op->m_src->buf_len = options->segment_sz;
> > > > +			sym_op->m_src->data_len = buf_sz;
> > > > +			sym_op->m_src->pkt_len = buf_sz;
> > > > +
> > >
> > > Actually, I am wondering why this is needed at all (for DOCSIS and
> > > PDCP). This is already set in " fill_multi_seg_mbuf" or "
> > > fill_single_seg_mbuf" (and this was already working without this patch,
> right?).
> >
> > [DC] I have found that if a number of buffer sizes are specified like this on the
> > cmd line "--buffer-sz 64,256,1024", then the pkt_len and data_len filled in
> > "fill_multi_seg_mbuf" or " fill_single_seg_mbuf" is always the largest of the
> sizes
> > specified. The cipher/auth lengths are then set based on the --buffer-sz option.
> >
> > For DOCSIS, I tried to be more accurate and set the correct pkt_len and
> data_len
> > in the mbuf. This followed what PDCP did too, even though I'm not sure of the
> > background why PDCP did it - possibly spotted the same issue. I have also
> found
> > that DOCSIS performance figures can be better if the correct pkt_len and
> > data_len are set in the mbuf - I don't have any proper explanation for this
> > though as the cipher/ auth lengths are always the same.
> >
> > I've dug around a bit more on this now though and this is actually a problem
> > across the perf tool. Some of the crypto PMDs have logic based on the mbuf
> > pkt_len and data_len, but because the perf tool isn't always setting these fields
> > correctly, that logic may not work as expected.
> > >
>
> Right, thanks for checking this. If I remember correctly, it was fine to have this
> set to the maximum size as the important field for crypto PMDs to check is the
> cipher/auth lengths, as you said. If there is more logic that depends on data_len
> on other PMDs, I agree it might be a problem. The only usage I knew for it was
> the multi segment case (in AES-GCM PMD), where data_len is checked in each
> segment size to see if all the cipher/auth length resides within these segments,
> but in the tool we set data_len for each segment when "going multi-segment". I
> see that other PMDs like DPAA2_SEC use these fields for something which I am
> not sure what's for. It would be good if the maintainers check if this is a problem
> for them, and in that case, this should be fixed for the other functions (for
> "normal" crypto).
> 

In case of test-crypto-perf, the buffers are flat and there is no case of multi segment.
So this is not because of that.
In case of PDCP and probably all the protocol offload cases would need the buf_len/
data_len/pkt_len to be set properly. As the complete buffer is given to hardware
and depending on the headers added, HW/PMD will adjust these lengths when the
packet is dequeued, provided it has room available to expand.

We may not need this in cases of pure crypto which have fixed lengths and PMD does
not control them.

So in my opinion this patch is fine.

Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests

[Akhil Goyal]

> Hi Pablo/David,
> >
> > Hi David,
> >
> > > Hi Pablo,
> > >
> > > > -----Original Message-----
> > > > From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > > > Sent: Friday, July 17, 2020 8:04 PM
> > > > > @@ -48,6 +48,10 @@ cperf_set_ops_security(struct rte_crypto_op
> **ops,
> > > > >  			} else
> > > > >  				buf_sz = options->test_buffer_size;
> > > > >
> > > > > +			sym_op->m_src->buf_len = options-
> >segment_sz;
> > > > > +			sym_op->m_src->data_len = buf_sz;
> > > > > +			sym_op->m_src->pkt_len = buf_sz;
> > > > > +
> > > >
> > > > Actually, I am wondering why this is needed at all (for DOCSIS and
> > > > PDCP). This is already set in " fill_multi_seg_mbuf" or "
> > > > fill_single_seg_mbuf" (and this was already working without this patch,
> > right?).
> > >
> > > [DC] I have found that if a number of buffer sizes are specified like this on
> the
> > > cmd line "--buffer-sz 64,256,1024", then the pkt_len and data_len filled in
> > > "fill_multi_seg_mbuf" or " fill_single_seg_mbuf" is always the largest of the
> > sizes
> > > specified. The cipher/auth lengths are then set based on the --buffer-sz
> option.
> > >
> > > For DOCSIS, I tried to be more accurate and set the correct pkt_len and
> > data_len
> > > in the mbuf. This followed what PDCP did too, even though I'm not sure of
> the
> > > background why PDCP did it - possibly spotted the same issue. I have also
> > found
> > > that DOCSIS performance figures can be better if the correct pkt_len and
> > > data_len are set in the mbuf - I don't have any proper explanation for this
> > > though as the cipher/ auth lengths are always the same.
> > >
> > > I've dug around a bit more on this now though and this is actually a problem
> > > across the perf tool. Some of the crypto PMDs have logic based on the mbuf
> > > pkt_len and data_len, but because the perf tool isn't always setting these
> fields
> > > correctly, that logic may not work as expected.
> > > >
> >
> > Right, thanks for checking this. If I remember correctly, it was fine to have this
> > set to the maximum size as the important field for crypto PMDs to check is the
> > cipher/auth lengths, as you said. If there is more logic that depends on
> data_len
> > on other PMDs, I agree it might be a problem. The only usage I knew for it was
> > the multi segment case (in AES-GCM PMD), where data_len is checked in each
> > segment size to see if all the cipher/auth length resides within these segments,
> > but in the tool we set data_len for each segment when "going multi-segment".
> I
> > see that other PMDs like DPAA2_SEC use these fields for something which I am
> > not sure what's for. It would be good if the maintainers check if this is a
> problem
> > for them, and in that case, this should be fixed for the other functions (for
> > "normal" crypto).
> >
> 
> In case of test-crypto-perf, the buffers are flat and there is no case of multi
> segment.
> So this is not because of that.
> In case of PDCP and probably all the protocol offload cases would need the
> buf_len/
> data_len/pkt_len to be set properly. As the complete buffer is given to hardware
> and depending on the headers added, HW/PMD will adjust these lengths when
> the
> packet is dequeued, provided it has room available to expand.
> 
> We may not need this in cases of pure crypto which have fixed lengths and PMD
> does
> not control them.
> 
> So in my opinion this patch is fine.
> 
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> 
> 
This patch was applied yesterday to dpdk-next-crypto
And is now pulled to master as well.