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

[Akhil Goyal <akhil.goyal@nxp.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>