DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
@ 2020-07-16 15:31 David Coyle
  2020-07-17 19:03 ` De Lara Guarch, Pablo
  0 siblings, 1 reply; 6+ messages in thread
From: David Coyle @ 2020-07-16 15:31 UTC (permalink / raw)
  To: akhil.goyal, declan.doherty, pablo.de.lara.guarch, fiona.trahe
  Cc: dev, brendan.ryan, mairtin.oloingsigh, 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
  2020-07-16 15:31 [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests David Coyle
@ 2020-07-17 19:03 ` De Lara Guarch, Pablo
  2020-07-20 12:59   ` Coyle, David
  0 siblings, 1 reply; 6+ messages in thread
From: De Lara Guarch, Pablo @ 2020-07-17 19:03 UTC (permalink / raw)
  To: Coyle, David, akhil.goyal, Doherty, Declan, Trahe, Fiona
  Cc: dev, Ryan, Brendan, O'loingsigh, Mairtin

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
  2020-07-17 19:03 ` De Lara Guarch, Pablo
@ 2020-07-20 12:59   ` Coyle, David
  2020-07-22  8:52     ` De Lara Guarch, Pablo
  0 siblings, 1 reply; 6+ messages in thread
From: Coyle, David @ 2020-07-20 12:59 UTC (permalink / raw)
  To: De Lara Guarch, Pablo, akhil.goyal, Doherty, Declan, Trahe, Fiona
  Cc: dev, Ryan, Brendan, O'loingsigh, Mairtin

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.
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
  2020-07-20 12:59   ` Coyle, David
@ 2020-07-22  8:52     ` De Lara Guarch, Pablo
  2020-07-28 19:44       ` Akhil Goyal
  0 siblings, 1 reply; 6+ messages in thread
From: De Lara Guarch, Pablo @ 2020-07-22  8:52 UTC (permalink / raw)
  To: Coyle, David, akhil.goyal, Doherty, Declan, Trahe, Fiona
  Cc: dev, Ryan, Brendan, O'loingsigh, Mairtin

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
  2020-07-22  8:52     ` De Lara Guarch, Pablo
@ 2020-07-28 19:44       ` Akhil Goyal
  2020-07-29 16:18         ` Akhil Goyal
  0 siblings, 1 reply; 6+ messages in thread
From: Akhil Goyal @ 2020-07-28 19:44 UTC (permalink / raw)
  To: De Lara Guarch, Pablo, Coyle, David, Doherty, Declan, Trahe, Fiona
  Cc: dev, Ryan, Brendan, O'loingsigh, Mairtin

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>




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
  2020-07-28 19:44       ` Akhil Goyal
@ 2020-07-29 16:18         ` Akhil Goyal
  0 siblings, 0 replies; 6+ messages in thread
From: Akhil Goyal @ 2020-07-29 16:18 UTC (permalink / raw)
  To: Akhil Goyal, De Lara Guarch, Pablo, Coyle, David, Doherty,
	Declan, Trahe, Fiona
  Cc: dev, Ryan, Brendan, O'loingsigh, Mairtin

> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-29 16:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 15:31 [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests David Coyle
2020-07-17 19:03 ` De Lara Guarch, Pablo
2020-07-20 12:59   ` Coyle, David
2020-07-22  8:52     ` De Lara Guarch, Pablo
2020-07-28 19:44       ` Akhil Goyal
2020-07-29 16:18         ` Akhil Goyal

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git