From: Anoob Joseph <anoob.joseph@caviumnetworks.com>
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" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom
Date: Tue, 10 Jul 2018 17:53:28 +0530 [thread overview]
Message-ID: <38fdd0af-1d77-a648-3bd3-ef7ab2899c40@caviumnetworks.com> (raw)
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8977F8F4472@IRSMSX107.ger.corp.intel.com>
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
>>
>>
next prev parent reply other threads:[~2018-07-10 12:23 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-19 6:26 [dpdk-dev] [PATCH 0/2] add head/tailroom requirement for crypto PMDs Anoob Joseph
2018-06-19 6:26 ` [dpdk-dev] [PATCH 1/2] cryptodev: add min headroom and tailroom requirement Anoob Joseph
2018-06-21 14:24 ` Akhil Goyal
2018-06-22 6:52 ` Joseph, Anoob
2018-06-22 10:03 ` Akhil Goyal
2018-06-26 10:12 ` Doherty, Declan
2018-06-28 2:56 ` Joseph, Anoob
2018-06-28 11:41 ` De Lara Guarch, Pablo
2018-06-28 11:59 ` Joseph, Anoob
2018-06-19 6:26 ` [dpdk-dev] [PATCH 2/2] app/crypto-perf: honour cryptodev's min headroom/tailroom Anoob Joseph
2018-06-28 11:42 ` De Lara Guarch, Pablo
2018-07-04 13:55 ` [dpdk-dev] [PATCH v1 0/3] add head/tailroom requirement for crypto PMDs Anoob Joseph
2018-07-04 13:55 ` [dpdk-dev] [PATCH v1 1/3] cryptodev: add min headroom and tailroom requirement Anoob Joseph
2018-07-10 10:26 ` De Lara Guarch, Pablo
2018-07-10 10:50 ` Anoob Joseph
2018-07-04 13:55 ` [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom Anoob Joseph
2018-07-10 11:07 ` De Lara Guarch, Pablo
2018-07-10 11:16 ` De Lara Guarch, Pablo
2018-07-10 11:48 ` De Lara Guarch, Pablo
2018-07-10 12:23 ` Anoob Joseph [this message]
2018-07-10 13:27 ` De Lara Guarch, Pablo
2018-07-10 14:08 ` Anoob Joseph
2018-07-04 13:55 ` [dpdk-dev] [PATCH v1 3/3] test/crypto: skip validation of head/tailroom used by PMD Anoob Joseph
2018-07-10 14:42 ` [dpdk-dev] [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs Anoob Joseph
2018-07-10 14:42 ` [dpdk-dev] [PATCH v2 1/4] cryptodev: add min headroom and tailroom requirement Anoob Joseph
2018-07-10 14:42 ` [dpdk-dev] [PATCH v2 2/4] app/crypto-perf: honour cryptodev's min headroom/tailroom Anoob Joseph
2018-07-10 14:42 ` [dpdk-dev] [PATCH v2 3/4] test/crypto: skip validation of head/tailroom used by PMD Anoob Joseph
2018-07-10 14:42 ` [dpdk-dev] [PATCH v2 4/4] crypto/scheduler: add minimum head/tailroom requirement Anoob Joseph
2018-07-10 17:20 ` [dpdk-dev] [PATCH v2 0/4] add head/tailroom requirement for crypto PMDs De Lara Guarch, Pablo
2018-07-10 17:29 ` De Lara Guarch, Pablo
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=38fdd0af-1d77-a648-3bd3-ef7ab2899c40@caviumnetworks.com \
--to=anoob.joseph@caviumnetworks.com \
--cc=akhil.goyal@nxp.com \
--cc=ankur.dwivedi@caviumnetworks.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=narayanaprasad.athreya@caviumnetworks.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).