From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anoob Joseph Subject: Re: [PATCH v1 2/3] app/crypto-perf: honour cryptodev's min headroom/tailroom Date: Tue, 10 Jul 2018 17:53:28 +0530 Message-ID: <38fdd0af-1d77-a648-3bd3-ef7ab2899c40@caviumnetworks.com> References: <1529389574-6643-1-git-send-email-anoob.joseph@caviumnetworks.com> <1530712550-18099-1-git-send-email-anoob.joseph@caviumnetworks.com> <1530712550-18099-3-git-send-email-anoob.joseph@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Akhil Goyal , Ankur Dwivedi , Jerin Jacob , Narayana Prasad , "dev@dpdk.org" To: "De Lara Guarch, Pablo" , "Doherty, Declan" Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0067.outbound.protection.outlook.com [104.47.40.67]) by dpdk.org (Postfix) with ESMTP id D3CFC49E1 for ; Tue, 10 Jul 2018 14:23:53 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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' ; Doherty, Declan >> >> Cc: 'Akhil Goyal' ; 'Ankur Dwivedi' >> ; 'Jerin Jacob' >> ; 'Narayana Prasad' >> ; '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' ; Doherty, Declan >>> >>> Cc: Akhil Goyal ; Ankur Dwivedi >>> ; Jerin Jacob >>> ; Narayana Prasad >>> ; 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 ; De Lara Guarch, >>>> Pablo >>>> Cc: Anoob Joseph ; Akhil Goyal >>>> ; Ankur Dwivedi >>>> ; Jerin Jacob >>>> ; Narayana Prasad >>>> ; 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 >>> ... >>> >>>> --- 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 >> >>