> -----Original Message----- > From: Anoob Joseph [mailto:[email protected]] > Sent: Tuesday, July 10, 2018 1:23 PM > To: De Lara Guarch, Pablo <[email protected]>; Doherty, Declan > <[email protected]> > Cc: Akhil Goyal <[email protected]>; Ankur Dwivedi > <[email protected]>; Jerin Jacob > <[email protected]>; Narayana Prasad > <[email protected]>; [email protected] > 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' <[email protected]>; Doherty, Declan > >> <[email protected]> > >> Cc: 'Akhil Goyal' <[email protected]>; 'Ankur Dwivedi' > >> <[email protected]>; 'Jerin Jacob' > >> <[email protected]>; 'Narayana Prasad' > >> <[email protected]>; '[email protected]' > >> <[email protected]> > >> 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' <[email protected]>; Doherty, > >>> Declan <[email protected]> > >>> Cc: Akhil Goyal <[email protected]>; Ankur Dwivedi > >>> <[email protected]>; Jerin Jacob > >>> <[email protected]>; Narayana Prasad > >>> <[email protected]>; [email protected] > >>> Subject: RE: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min > >>> headroom/tailroom > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Anoob Joseph [mailto:[email protected]] > >>>> Sent: Wednesday, July 4, 2018 2:56 PM > >>>> To: Doherty, Declan <[email protected]>; De Lara Guarch, > >>>> Pablo <[email protected]> > >>>> Cc: Anoob Joseph <[email protected]>; Akhil Goyal > >>>> <[email protected]>; Ankur Dwivedi > >>>> <[email protected]>; Jerin Jacob > >>>> <[email protected]>; Narayana Prasad > >>>> <[email protected]>; [email protected] > >>>> 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 <[email protected]> > >>> ... > >>> > >>>> --- 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 > >> > >>
