From: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
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: [dpdk-dev] [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom
Date: Tue, 10 Jul 2018 13:27:39 +0000 [thread overview]
Message-ID: <E115CCD9D858EF4F90C690B0DCB4D8977F8F457D@IRSMSX107.ger.corp.intel.com> (raw)
In-Reply-To: <38fdd0af-1d77-a648-3bd3-ef7ab2899c40@caviumnetworks.com>
> -----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
> >>
> >>
next prev parent reply other threads:[~2018-07-10 13:27 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
2018-07-10 13:27 ` De Lara Guarch, Pablo [this message]
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=E115CCD9D858EF4F90C690B0DCB4D8977F8F457D@IRSMSX107.ger.corp.intel.com \
--to=pablo.de.lara.guarch@intel.com \
--cc=akhil.goyal@nxp.com \
--cc=ankur.dwivedi@caviumnetworks.com \
--cc=anoob.joseph@caviumnetworks.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=narayanaprasad.athreya@caviumnetworks.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).