Hi Pablo,


On 10-07-2018 18:57, De Lara Guarch, Pablo wrote:

External Email

quoted

-----Original Message-----
From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
Sent: Tuesday, July 10, 2018 1:23 PM
To: De Lara Guarch, Pablo <redacted>; Doherty, Declan
[off-list ref]
Cc: Akhil Goyal <redacted>; Ankur Dwivedi
[off-list ref]; Jerin Jacob
[off-list ref]; Narayana Prasad
[off-list ref]; 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:

quoted

External Email

quoted

-----Original Message-----
From: De Lara Guarch, Pablo
Sent: Tuesday, July 10, 2018 12:17 PM
To: 'Anoob Joseph' <redacted>; Doherty, Declan
[off-list ref]
Cc: 'Akhil Goyal' <redacted>; 'Ankur Dwivedi'
[off-list ref]; 'Jerin Jacob'
[off-list ref]; 'Narayana Prasad'
[off-list ref]; 'dev@dpdk.org'
[off-list ref]
Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
headroom/tailroom

quoted

-----Original Message-----
From: De Lara Guarch, Pablo
Sent: Tuesday, July 10, 2018 12:08 PM
To: 'Anoob Joseph' <redacted>; Doherty,
Declan [off-list ref]
Cc: Akhil Goyal <redacted>; Ankur Dwivedi
[off-list ref]; Jerin Jacob
[off-list ref]; Narayana Prasad
[off-list ref]; dev@dpdk.org
Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min
headroom/tailroom

quoted

-----Original Message-----
From: Anoob Joseph [mailto:anoob.joseph@caviumnetworks.com]
Sent: Wednesday, July 4, 2018 2:56 PM
To: Doherty, Declan <redacted>; De Lara Guarch,
Pablo [off-list ref]
Cc: Anoob Joseph <redacted>; Akhil Goyal
[off-list ref]; Ankur Dwivedi
[off-list ref]; Jerin Jacob
[off-list ref]; Narayana Prasad
[off-list ref]; 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 <redacted>

...

quoted

--- a/app/test-crypto-perf/cperf_test_common.c
+++ b/app/test-crypto-perf/cperf_test_common.c

...

quoted

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.

quoted

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.

quoted

quoted

In any case, I have a concern though about this. Headroom size is got from a

compile time option:

quoted

CONFIG_RTE_PKTMBUF_HEADROOM=128. PMDs generally use this value to

set

quoted

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

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

Sorry for the confusion and this last minute concern.

Thanks,
Pablo

quoted

Thanks,
Pablo