[v1,2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom
Checks
Commit Message
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@caviumnetworks.com>
---
v1:
* Accomodated the name change of new fields
app/test-crypto-perf/cperf_options.h | 2 ++
app/test-crypto-perf/cperf_test_common.c | 33 +++++++++++++++++++++-----------
app/test-crypto-perf/main.c | 17 ++++++++++++++++
3 files changed, 41 insertions(+), 11 deletions(-)
Comments
> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> Sent: Wednesday, July 4, 2018 2:56 PM
> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Ankur Dwivedi
> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@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.
> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Tuesday, July 10, 2018 12:08 PM
> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com]
> > Sent: Wednesday, July 4, 2018 2:56 PM
> > To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Akhil Goyal
> > <akhil.goyal@nxp.com>; Ankur Dwivedi
> > <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> > <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> > <narayanaprasad.athreya@caviumnetworks.com>; dev@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@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.
Thanks,
Pablo
> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Tuesday, July 10, 2018 12:17 PM
> To: 'Anoob Joseph' <anoob.joseph@caviumnetworks.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Cc: 'Akhil Goyal' <akhil.goyal@nxp.com>; 'Ankur Dwivedi'
> <ankur.dwivedi@caviumnetworks.com>; 'Jerin Jacob'
> <jerin.jacob@caviumnetworks.com>; 'Narayana Prasad'
> <narayanaprasad.athreya@caviumnetworks.com>; 'dev@dpdk.org'
> <dev@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@caviumnetworks.com>; Doherty, Declan
> > <declan.doherty@intel.com>
> > Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
> > <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> > <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> > <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com]
> > > Sent: Wednesday, July 4, 2018 2:56 PM
> > > To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
> > > Pablo <pablo.de.lara.guarch@intel.com>
> > > Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Akhil Goyal
> > > <akhil.goyal@nxp.com>; Ankur Dwivedi
> > > <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> > > <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> > > <narayanaprasad.athreya@caviumnetworks.com>; dev@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@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.
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.
Also, generally, headroom and tailroom are used for encapsulation, so I am not sure if this is the best place.
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.
Sorry for the confusion and this last minute concern.
Thanks,
Pablo
>
> Thanks,
> Pablo
>
>
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@caviumnetworks.com>; Doherty, Declan
>> <declan.doherty@intel.com>
>> Cc: 'Akhil Goyal' <akhil.goyal@nxp.com>; 'Ankur Dwivedi'
>> <ankur.dwivedi@caviumnetworks.com>; 'Jerin Jacob'
>> <jerin.jacob@caviumnetworks.com>; 'Narayana Prasad'
>> <narayanaprasad.athreya@caviumnetworks.com>; 'dev@dpdk.org'
>> <dev@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@caviumnetworks.com>; Doherty, Declan
>>> <declan.doherty@intel.com>
>>> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
>>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
>>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
>>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com]
>>>> Sent: Wednesday, July 4, 2018 2:56 PM
>>>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
>>>> Pablo <pablo.de.lara.guarch@intel.com>
>>>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Akhil Goyal
>>>> <akhil.goyal@nxp.com>; Ankur Dwivedi
>>>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
>>>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
>>>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@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
>>
>>
> -----Original Message-----
> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
> Sent: Tuesday, July 10, 2018 1:23 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>
> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com>; Doherty, Declan
> >> <declan.doherty@intel.com>
> >> Cc: 'Akhil Goyal' <akhil.goyal@nxp.com>; 'Ankur Dwivedi'
> >> <ankur.dwivedi@caviumnetworks.com>; 'Jerin Jacob'
> >> <jerin.jacob@caviumnetworks.com>; 'Narayana Prasad'
> >> <narayanaprasad.athreya@caviumnetworks.com>; 'dev@dpdk.org'
> >> <dev@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@caviumnetworks.com>; Doherty,
> >>> Declan <declan.doherty@intel.com>
> >>> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
> >>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> >>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> >>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com]
> >>>> Sent: Wednesday, July 4, 2018 2:56 PM
> >>>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
> >>>> Pablo <pablo.de.lara.guarch@intel.com>
> >>>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Akhil Goyal
> >>>> <akhil.goyal@nxp.com>; Ankur Dwivedi
> >>>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
> >>>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
> >>>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@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
> >>
> >>
Hi Pablo,
On 10-07-2018 18:57, De Lara Guarch, Pablo wrote:
> External Email
>
>> -----Original Message-----
>> From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
>> Sent: Tuesday, July 10, 2018 1:23 PM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
>> <declan.doherty@intel.com>
>> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com>; Doherty, Declan
>>>> <declan.doherty@intel.com>
>>>> Cc: 'Akhil Goyal' <akhil.goyal@nxp.com>; 'Ankur Dwivedi'
>>>> <ankur.dwivedi@caviumnetworks.com>; 'Jerin Jacob'
>>>> <jerin.jacob@caviumnetworks.com>; 'Narayana Prasad'
>>>> <narayanaprasad.athreya@caviumnetworks.com>; 'dev@dpdk.org'
>>>> <dev@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@caviumnetworks.com>; Doherty,
>>>>> Declan <declan.doherty@intel.com>
>>>>> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
>>>>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
>>>>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
>>>>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@caviumnetworks.com]
>>>>>> Sent: Wednesday, July 4, 2018 2:56 PM
>>>>>> To: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
>>>>>> Pablo <pablo.de.lara.guarch@intel.com>
>>>>>> Cc: Anoob Joseph <anoob.joseph@caviumnetworks.com>; Akhil Goyal
>>>>>> <akhil.goyal@nxp.com>; Ankur Dwivedi
>>>>>> <ankur.dwivedi@caviumnetworks.com>; Jerin Jacob
>>>>>> <jerin.jacob@caviumnetworks.com>; Narayana Prasad
>>>>>> <narayanaprasad.athreya@caviumnetworks.com>; dev@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@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).
Agreed.
>>> 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.
Ideally yes. But with spec change we can move this responsibility to the
application, and could skip the check from PMD. Is that a reasonable
approach?
With this change we explicitly say the PMD needs this much headroom &
tailroom. So application can be expected to honor that.
>
>>> 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
>>>>
>>>>
@@ -76,6 +76,8 @@ struct cperf_options {
uint32_t pool_sz;
uint32_t total_ops;
+ uint32_t headroom_sz;
+ uint32_t tailroom_sz;
uint32_t segment_sz;
uint32_t test_buffer_size;
uint32_t *imix_buffer_sizes;
@@ -11,12 +11,15 @@ struct obj_params {
uint32_t src_buf_offset;
uint32_t dst_buf_offset;
uint16_t segment_sz;
+ uint16_t headroom_sz;
+ uint16_t data_len;
uint16_t segments_nb;
};
static void
fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
- void *obj, uint32_t mbuf_offset, uint16_t segment_sz)
+ void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
+ uint16_t headroom, uint16_t data_len)
{
uint32_t mbuf_hdr_size = sizeof(struct rte_mbuf);
@@ -26,10 +29,10 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
m->buf_iova = rte_mempool_virt2iova(obj) +
mbuf_offset + mbuf_hdr_size;
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;
/* init some constant fields */
m->pool = mp;
@@ -42,7 +45,7 @@ fill_single_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
static void
fill_multi_seg_mbuf(struct rte_mbuf *m, struct rte_mempool *mp,
void *obj, uint32_t mbuf_offset, uint16_t segment_sz,
- uint16_t segments_nb)
+ uint16_t headroom, uint16_t data_len, uint16_t segments_nb)
{
uint16_t mbuf_hdr_size = sizeof(struct rte_mbuf);
uint16_t remaining_segments = segments_nb;
@@ -57,10 +60,10 @@ 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;
/* init some constant fields */
m->pool = mp;
@@ -99,10 +102,12 @@ mempool_obj_init(struct rte_mempool *mp,
op->sym->m_src = m;
if (params->segments_nb == 1)
fill_single_seg_mbuf(m, mp, obj, params->src_buf_offset,
- params->segment_sz);
+ params->segment_sz, params->headroom_sz,
+ params->data_len);
else
fill_multi_seg_mbuf(m, mp, obj, params->src_buf_offset,
- params->segment_sz, params->segments_nb);
+ params->segment_sz, params->headroom_sz,
+ params->data_len, params->segments_nb);
/* Set destination buffer */
@@ -110,7 +115,8 @@ mempool_obj_init(struct rte_mempool *mp,
m = (struct rte_mbuf *) ((uint8_t *) obj +
params->dst_buf_offset);
fill_single_seg_mbuf(m, mp, obj, params->dst_buf_offset,
- params->segment_sz);
+ params->segment_sz, params->headroom_sz,
+ params->data_len);
op->sym->m_dst = m;
} else
op->sym->m_dst = NULL;
@@ -172,6 +178,11 @@ cperf_alloc_common_memory(const struct cperf_options *options,
struct obj_params params = {
.segment_sz = options->segment_sz,
+ .headroom_sz = options->headroom_sz,
+ /* Data len = segment size - (headroom + tailroom) */
+ .data_len = options->segment_sz -
+ options->headroom_sz -
+ options->tailroom_sz,
.segments_nb = segments_nb,
.src_buf_offset = crypto_op_total_size_padded,
.dst_buf_offset = 0
@@ -149,6 +149,23 @@ cperf_initialize_cryptodev(struct cperf_options *opts, uint8_t *enabled_cdevs,
.nb_descriptors = opts->nb_descriptors
};
+ /**
+ * Device info specifies the min headroom and tailroom
+ * requirement for the crypto PMD. This need to be honoured
+ * by the application, while creating mbuf.
+ */
+ if (opts->headroom_sz < cdev_info.min_mbuf_headroom_req) {
+ /* Update headroom */
+ opts->headroom_sz = cdev_info.min_mbuf_headroom_req;
+ }
+ if (opts->tailroom_sz < cdev_info.min_mbuf_tailroom_req) {
+ /* Update tailroom */
+ opts->tailroom_sz = cdev_info.min_mbuf_tailroom_req;
+ }
+
+ /* Update segment size to include headroom & tailroom */
+ opts->segment_sz += (opts->headroom_sz + opts->tailroom_sz);
+
if (session_pool_socket[socket_id] == NULL) {
char mp_name[RTE_MEMPOOL_NAMESIZE];
struct rte_mempool *sess_mp;