DPDK patches and discussions
 help / color / mirror / Atom feed
From: Akhil Goyal <akhil.goyal@nxp.com>
To: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
	"Coyle, David" <david.coyle@intel.com>,
	"Doherty, Declan" <declan.doherty@intel.com>,
	"Trahe, Fiona" <fiona.trahe@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"Ryan, Brendan" <brendan.ryan@intel.com>,
	"O'loingsigh, Mairtin" <mairtin.oloingsigh@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1] app/crypto-perf: set mbuf lengths correctly for DOCSIS tests
Date: Tue, 28 Jul 2020 19:44:13 +0000
Message-ID: <AM5PR04MB3153965F5D793A61975759E6E6730@AM5PR04MB3153.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <SN6PR11MB3101E72D919F004088CDAD7484790@SN6PR11MB3101.namprd11.prod.outlook.com>

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>




  reply	other threads:[~2020-07-28 19:44 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-16 15:31 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 [this message]
2020-07-29 16:18         ` Akhil Goyal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM5PR04MB3153965F5D793A61975759E6E6730@AM5PR04MB3153.eurprd04.prod.outlook.com \
    --to=akhil.goyal@nxp.com \
    --cc=brendan.ryan@intel.com \
    --cc=david.coyle@intel.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=fiona.trahe@intel.com \
    --cc=mairtin.oloingsigh@intel.com \
    --cc=pablo.de.lara.guarch@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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