[dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom

De Lara Guarch, Pablo pablo.de.lara.guarch at intel.com
Tue Jul 10 15:27:39 CEST 2018



> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph at caviumnetworks.com]
> Sent: Tuesday, July 10, 2018 1:23 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Doherty, Declan
> <declan.doherty at intel.com>
> Cc: Akhil Goyal <akhil.goyal at nxp.com>; Ankur Dwivedi
> <ankur.dwivedi at caviumnetworks.com>; Jerin Jacob
> <jerin.jacob at caviumnetworks.com>; Narayana Prasad
> <narayanaprasad.athreya at caviumnetworks.com>; dev at dpdk.org
> Subject: Re: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> headroom/tailroom
> 
> Hi Pablo,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> On 10-07-2018 17:18, De Lara Guarch, Pablo wrote:
> > External Email
> >
> >> -----Original Message-----
> >> From: De Lara Guarch, Pablo
> >> Sent: Tuesday, July 10, 2018 12:17 PM
> >> To: 'Anoob Joseph' <anoob.joseph at caviumnetworks.com>; Doherty, Declan
> >> <declan.doherty at intel.com>
> >> Cc: 'Akhil Goyal' <akhil.goyal at nxp.com>; 'Ankur Dwivedi'
> >> <ankur.dwivedi at caviumnetworks.com>; 'Jerin Jacob'
> >> <jerin.jacob at caviumnetworks.com>; 'Narayana Prasad'
> >> <narayanaprasad.athreya at caviumnetworks.com>; 'dev at dpdk.org'
> >> <dev at dpdk.org>
> >> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> >> headroom/tailroom
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: De Lara Guarch, Pablo
> >>> Sent: Tuesday, July 10, 2018 12:08 PM
> >>> To: 'Anoob Joseph' <anoob.joseph at caviumnetworks.com>; Doherty,
> >>> Declan <declan.doherty at intel.com>
> >>> Cc: Akhil Goyal <akhil.goyal at nxp.com>; Ankur Dwivedi
> >>> <ankur.dwivedi at caviumnetworks.com>; Jerin Jacob
> >>> <jerin.jacob at caviumnetworks.com>; Narayana Prasad
> >>> <narayanaprasad.athreya at caviumnetworks.com>; dev at dpdk.org
> >>> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> >>> headroom/tailroom
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Anoob Joseph [mailto:anoob.joseph at caviumnetworks.com]
> >>>> Sent: Wednesday, July 4, 2018 2:56 PM
> >>>> To: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch,
> >>>> Pablo <pablo.de.lara.guarch at intel.com>
> >>>> Cc: Anoob Joseph <anoob.joseph at caviumnetworks.com>; Akhil Goyal
> >>>> <akhil.goyal at nxp.com>; Ankur Dwivedi
> >>>> <ankur.dwivedi at caviumnetworks.com>; Jerin Jacob
> >>>> <jerin.jacob at caviumnetworks.com>; Narayana Prasad
> >>>> <narayanaprasad.athreya at caviumnetworks.com>; dev at dpdk.org
> >>>> Subject: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
> >>>> headroom/tailroom
> >>>>
> >>>> Crypto dev would specify its headroom and tailroom requirement and
> >>>> the application is expected to honour this while creating buffers.
> >>>>
> >>>> Signed-off-by: Anoob Joseph <anoob.joseph at caviumnetworks.com>
> >>> ...
> >>>
> >>>> --- a/app/test-crypto-perf/cperf_test_common.c
> >>>> +++ b/app/test-crypto-perf/cperf_test_common.c
> >>> ...
> >>>
> >>>> fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
> >>>>            m->buf_iova = next_seg_phys_addr;
> >>>>            next_seg_phys_addr += mbuf_hdr_size + segment_sz;
> >>>>            m->buf_len = segment_sz;
> >>>> -         m->data_len = segment_sz;
> >>>> +         m->data_len = data_len;
> >>>>
> >>>> -         /* No headroom needed for the buffer */
> >>>> -         m->data_off = 0;
> >>>> +         /* Use headroom specified for the buffer */
> >>>> +         m->data_off = headroom;
> >>> Headroom is only applicable for the first segment/s.
> >>> This is adding headroom in all the segments, which looks wrong.
> >>>
> >> I think "max_size" needs to be recalculated in
> >> "cperf_alloc_common_memory", adding headroom and tailroom size, which
> >> will potentially increase the number of segments required.
> >> Then, headroom size needs to be checked in case it is bigger than
> >> segment size, so data might need to start in the next segment.
> >> Similar thing for tailroom.
> > Actually, forget about this. I have been thinking about it, and it looks artificial
> to do this.
> > Generally, in a mbuf pool, headroom is the same for all mbufs/segments.
> Do I need to revisit this patch? Or is this fine?

I'd say it is ok then. I might rework the app in the future, to deal better with the pool creation
(without needing to set the mbuf parameters manually).

> >
> > In any case, I have a concern though about this. Headroom size is got from a
> compile time option:
> > CONFIG_RTE_PKTMBUF_HEADROOM=128. PMDs generally use this value to
> set
> > "data_off", but they could use another different value.
> > So what happens if min_mbuf_headroom is more than this value?
> > since this is not configurable, this won't work.
> Since this is a PMD specific issue, we can have an extra check in the driver to
> make sure "CONFIG_RTE_PKTMBUF_HEADROOM">= min_mbuf_headroom for
> the PMD. If this check isn't satisfied, the driver probe would fail.
> Is this approach fine?

Probably ok, although eventually, a check in the actual headroom, per operation, will be required.

> 
> If application chooses to ignore CONFIG_RTE_PKTMBUF_HEADROOM
> altogether, then it will be a problem for most PMDs. With protocol offloads etc,
> headroom would be used internally, right?

I am not sure what can be done here. Headroom availability depends on the network driver,
but then the application can prepend data and make the headroom smaller.

> > Also, generally, headroom and tailroom are used for encapsulation, so I am
> not sure if this is the best place.
> Is your concern about whether there is enough space in headroom for
> encapsulation & PMD's usage? Application can probe the individual values and
> see if there is enough space, right? In our use case, the headroom requirement is
> 24 bytes & tailroom requirement is 8 bytes.

Right, although this will have to be done in data path, right?
Headroom and tailroom can only be known once packets are received.

> > What about using the private size of the mbuf? That is actually
> > configurable, even though that data is not necessarily contiguous to the mbuf
> data.
> That memory being non contiguous is the problem. We use the headroom to
> specify the command so that one single buffer can be sent to the h/w for
> processing. If there is a gap of 128 bytes (headroom which lies in between
> private space & data), it will not work.

Ok, I understand. Then I'd say this is the only way to do it.

> >
> > Sorry for the confusion and this last minute concern.
> >
> > Thanks,
> > Pablo
> >
> >
> >> Thanks,
> >> Pablo
> >>
> >>



More information about the dev mailing list