> -----Original Message----- > From: Anoob Joseph [mailto:anoob.jos...@caviumnetworks.com] > Sent: Tuesday, July 10, 2018 1:23 PM > To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.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 > > 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?
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 > >> > >>
