DPDK patches and discussions
 help / color / mirror / Atom feed
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
>>
>>

  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).