From mboxrd@z Thu Jan 1 00:00:00 1970 From: santosh Subject: Re: [RFC v2 01/17] mempool: fix phys contig check if populate default skipped Date: Thu, 1 Feb 2018 15:00:00 +0530 Message-ID: <53796b3f-b2c2-dcfc-a10b-71ce1a68d3d2@caviumnetworks.com> References: <1511539591-20966-1-git-send-email-arybchenko@solarflare.com> <1516713372-10572-1-git-send-email-arybchenko@solarflare.com> <1516713372-10572-2-git-send-email-arybchenko@solarflare.com> <20180131164504.cnfgfwo2x3ftxnaj@platinum> <90005aad-1508-44bc-92a4-7c5d5f7b0246@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, stable@dpdk.org, jerin.jacob@caviumnetworks.com To: Andrew Rybchenko , Olivier Matz Return-path: 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" On Thursday 01 February 2018 02:48 PM, Andrew Rybchenko wrote: > On 02/01/2018 12:09 PM, santosh wrote: >> On Thursday 01 February 2018 12:24 PM, Andrew Rybchenko wrote: >>> On 02/01/2018 08:05 AM, santosh wrote: >>>> On Wednesday 31 January 2018 10:15 PM, Olivier Matz wrote: >>>>> On Tue, Jan 23, 2018 at 01:15:56PM +0000, Andrew Rybchenko wrote: >>>>>> There is not specified dependency between rte_mempool_populate_default() >>>>>> and rte_mempool_populate_iova(). So, the second should not rely on the >>>>>> fact that the first adds capability flags to the mempool flags. >>>>>> >>>>>> Fixes: 65cf769f5e6a ("mempool: detect physical contiguous objects") >>>>>> Cc: stable@dpdk.org >>>>>> >>>>>> Signed-off-by: Andrew Rybchenko >>>>> Looks good to me. I agree it's strange that the mp->flags are >>>>> updated with capabilities only in rte_mempool_populate_default(). >>>>> I see that this behavior is removed later in the patchset since the >>>>> get_capa() is removed! >>>>> >>>>> However maybe this single patch could go in 18.02. >>>>> +Santosh +Jerin since it's mostly about Octeon. >>>> rte_mempool_xmem_size should return correct size if MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS flag >>>> is set in 'mp->flags'. Thats why _ops_get_capabilities() called in _populate_default() but not >>>> at _populate_iova(). >>>> I think, this 'alone' patch may break octeontx mempool. >>> The patch does not touch rte_mempool_populate_default(). >>> _ops_get_capabilities() is still called there before >>> rte_mempool_xmem_size(). The theoretical problem which >>> the patch tries to fix is the case when >>> rte_mempool_populate_default() is not called at all. I.e. application >>> calls _ops_get_capabilities() to get flags, then, together with >>> mp->flags, calls rte_mempool_xmem_size() directly, allocates >>> calculated amount of memory and calls _populate_iova(). >>> >> In that case, Application does like below: >> >>     /* Get mempool capabilities */ >>     mp_flags = 0; >>     ret = rte_mempool_ops_get_capabilities(mp, &mp_flags); >>     if ((ret < 0) && (ret != -ENOTSUP)) >>         return ret; >> >>     /* update mempool capabilities */ >>     mp->flags |= mp_flags; > > Above line is not mandatory. "mp->flags | mp_flags" could be simply > passed to  rte_mempool_xmem_size() below. > That depends and again upto application requirement, if app further down wants to refer mp->flags for _align/_contig then better update to mp->flags. But that wasn't the point of discussion, I'm trying to understand that w/o this patch, whats could be the application level problem?