[dpdk-dev,RFC,v2,01/17] mempool: fix phys contig check if populate default skipped
Checks
Commit Message
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 <arybchenko@solarflare.com>
---
lib/librte_mempool/rte_mempool.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
Comments
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 <arybchenko@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.
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 <arybchenko@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.
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 <arybchenko@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().
Since later patches of the series reconsider memory size
calculation etc, it is up to you if it makes sense to apply it
in 18.02 as a fix.
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 <arybchenko@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;
/* calc xmem sz */
size = rte_mempool_xmem_size(n, total_elt_sz, pg_shift,
mp->flags);
/* rsrv memory */
mz = rte_memzone_reserve_aligned(mz_name, size,...);
/* now populate iova */
ret = rte_mempool_populate_iova(mp,,..);
won't it work?
However I understand that clubbing `_get_ops_capa() + flag-updation` into _populate_iova()
perhaps better from user PoV.
> Since later patches of the series reconsider memory size
> calculation etc, it is up to you if it makes sense to apply it
> in 18.02 as a fix.
>
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 <arybchenko@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.
> /* calc xmem sz */
> size = rte_mempool_xmem_size(n, total_elt_sz, pg_shift,
> mp->flags);
>
> /* rsrv memory */
> mz = rte_memzone_reserve_aligned(mz_name, size,...);
>
> /* now populate iova */
> ret = rte_mempool_populate_iova(mp,,..);
>
> won't it work?
>
> However I understand that clubbing `_get_ops_capa() + flag-updation` into _populate_iova()
> perhaps better from user PoV.
>
>> Since later patches of the series reconsider memory size
>> calculation etc, it is up to you if it makes sense to apply it
>> in 18.02 as a fix.
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 <arybchenko@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?
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@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@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,
rte_mempool_populate_iova/virt/iova_tab() functions will work
incorrectly since F_CAPA_PHYS_CONTIG and
F_CAPA_BLK_ALIGNED_OBJECTS are missing.
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.
On Thu, Feb 01, 2018 at 01:00:12PM +0300, 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@dpdk.org
> > > > > > > >
> > > > > > > > Signed-off-by: Andrew Rybchenko <arybchenko@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,
> rte_mempool_populate_iova/virt/iova_tab() functions will work
> incorrectly since F_CAPA_PHYS_CONTIG and
> F_CAPA_BLK_ALIGNED_OBJECTS are missing.
>
> 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.
The example given by Santosh will work, but it is *not* the role of the
application to update the mempool flags. And nothing says that it is mandatory
to call rte_mempool_ops_get_capabilities() before the populate functions.
For instance, in testpmd it calls rte_mempool_populate_anon() when using
anonymous memory. The capabilities will never be updated in mp->flags.
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@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@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.
On Thursday 01 February 2018 03:44 PM, Olivier Matz wrote:
> On Thu, Feb 01, 2018 at 01:00:12PM +0300, 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@dpdk.org
>>>>>>>>>
>>>>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@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,
>> rte_mempool_populate_iova/virt/iova_tab() functions will work
>> incorrectly since F_CAPA_PHYS_CONTIG and
>> F_CAPA_BLK_ALIGNED_OBJECTS are missing.
>>
>> 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.
> The example given by Santosh will work, but it is *not* the role of the
> application to update the mempool flags. And nothing says that it is mandatory
> to call rte_mempool_ops_get_capabilities() before the populate functions.
>
> For instance, in testpmd it calls rte_mempool_populate_anon() when using
> anonymous memory. The capabilities will never be updated in mp->flags.
Valid case and I agree with your example and explanation.
With nits change:
mp->flags |= mp_capa_flags;
Acked-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
On 02/01/2018 01:33 PM, santosh wrote:
> On Thursday 01 February 2018 03:44 PM, Olivier Matz wrote:
>> On Thu, Feb 01, 2018 at 01:00:12PM +0300, 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@dpdk.org
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@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,
>>> rte_mempool_populate_iova/virt/iova_tab() functions will work
>>> incorrectly since F_CAPA_PHYS_CONTIG and
>>> F_CAPA_BLK_ALIGNED_OBJECTS are missing.
>>>
>>> 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.
>> The example given by Santosh will work, but it is *not* the role of the
>> application to update the mempool flags. And nothing says that it is mandatory
>> to call rte_mempool_ops_get_capabilities() before the populate functions.
>>
>> For instance, in testpmd it calls rte_mempool_populate_anon() when using
>> anonymous memory. The capabilities will never be updated in mp->flags.
> Valid case and I agree with your example and explanation.
> With nits change:
> mp->flags |= mp_capa_flags;
>
> Acked-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
I'll submit the patch separately with this minor change. Thanks.
@@ -362,6 +362,7 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
void *opaque)
{
unsigned total_elt_sz;
+ unsigned int mp_cap_flags;
unsigned i = 0;
size_t off;
struct rte_mempool_memhdr *memhdr;
@@ -386,8 +387,14 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
+ /* Get mempool capabilities */
+ mp_cap_flags = 0;
+ ret = rte_mempool_ops_get_capabilities(mp, &mp_cap_flags);
+ if ((ret < 0) && (ret != -ENOTSUP))
+ return ret;
+
/* Detect pool area has sufficient space for elements */
- if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
+ if (mp_cap_flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
if (len < total_elt_sz * mp->size) {
RTE_LOG(ERR, MEMPOOL,
"pool area %" PRIx64 " not enough\n",
@@ -407,7 +414,7 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
memhdr->free_cb = free_cb;
memhdr->opaque = opaque;
- if (mp->flags & MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS)
+ if (mp_cap_flags & MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS)
/* align object start address to a multiple of total_elt_sz */
off = total_elt_sz - ((uintptr_t)vaddr % total_elt_sz);
else if (mp->flags & MEMPOOL_F_NO_CACHE_ALIGN)