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.jos...@caviumnetworks.com>; Doherty, Declan
<declan.dohe...@intel.com>
Cc: 'Akhil Goyal' <akhil.go...@nxp.com>; 'Ankur Dwivedi'
<ankur.dwiv...@caviumnetworks.com>; 'Jerin Jacob'
<jerin.ja...@caviumnetworks.com>; 'Narayana Prasad'
<narayanaprasad.athr...@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.jos...@caviumnetworks.com>; Doherty, Declan
<declan.dohe...@intel.com>
Cc: Akhil Goyal <akhil.go...@nxp.com>; Ankur Dwivedi
<ankur.dwiv...@caviumnetworks.com>; Jerin Jacob
<jerin.ja...@caviumnetworks.com>; Narayana Prasad
<narayanaprasad.athr...@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.jos...@caviumnetworks.com]
Sent: Wednesday, July 4, 2018 2:56 PM
To: Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch,
Pablo <pablo.de.lara.gua...@intel.com>
Cc: Anoob Joseph <anoob.jos...@caviumnetworks.com>; Akhil Goyal
<akhil.go...@nxp.com>; Ankur Dwivedi
<ankur.dwiv...@caviumnetworks.com>; Jerin Jacob
<jerin.ja...@caviumnetworks.com>; Narayana Prasad
<narayanaprasad.athr...@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.jos...@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
