[dpdk-dev] [RFC v2 01/17] mempool: fix phys contig check if populate default skipped

[santosh]

On Thursday 01 February 2018 03:30 PM, Andrew Rybchenko wrote:
> On 02/01/2018 12:30 PM, santosh wrote:
>> 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 at dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko at solarflare.com>
>>>>>>> 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?
>
> The problem that it is fragile. If application does not use
> rte_mempool_populate_default() it has to care about addition
> of mempool capability flags into mempool flags. If it is not done,

Capability flags should get updated to mempool flags. Or else
_get_ops_capabilities() to update capa flags to mempool flags internally,
I recall that I proposed same in the past.  

[...]

> The idea of the patch is to make it a bit more robust. I have no
> idea how it can break something. If capability flags are already
> there - no problem. If no, just make sure that we locally have them.
>
I would prefer _get_ops_capabilities() updates capa flags to mp->flag for once,
rather than doing (mp->flags | mp_flags) across mempool func.