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

[Anoob Joseph]

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

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?
> 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.
> 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.
>
> Sorry for the confusion and this last minute concern.
>
> Thanks,
> Pablo
>
>
>> Thanks,
>> Pablo
>>
>>