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

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

[Andrew Rybchenko]

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(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 6d17022..e783b9a 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -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)
-- 
2.7.4

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

[Olivier Matz]

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.

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

[santosh]

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.

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

[Andrew Rybchenko]

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.

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

[santosh]

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.
>

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

[Andrew Rybchenko]

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.

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

[santosh]

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?

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

[Andrew Rybchenko]

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.

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

[Olivier Matz]

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.

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

[santosh]

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>

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

[Andrew Rybchenko]

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.

Re: [dpdk-stable] [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@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.

[dpdk-stable] [PATCH] mempool: fix phys contig check if populate default skipped

[Andrew Rybchenko]

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>
Acked-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
---
 lib/librte_mempool/rte_mempool.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 6fdb723..54f7f4b 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -333,6 +333,7 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
 	void *opaque)
 {
 	unsigned total_elt_sz;
+	unsigned int mp_capa_flags;
 	unsigned i = 0;
 	size_t off;
 	struct rte_mempool_memhdr *memhdr;
@@ -357,8 +358,17 @@ 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_capa_flags = 0;
+	ret = rte_mempool_ops_get_capabilities(mp, &mp_capa_flags);
+	if ((ret < 0) && (ret != -ENOTSUP))
+		return ret;
+
+	/* update mempool capabilities */
+	mp->flags |= mp_capa_flags;
+
 	/* Detect pool area has sufficient space for elements */
-	if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
+	if (mp_capa_flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
 		if (len < total_elt_sz * mp->size) {
 			RTE_LOG(ERR, MEMPOOL,
 				"pool area %" PRIx64 " not enough\n",
@@ -378,7 +388,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_capa_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)
-- 
2.7.4

Re: [dpdk-stable] [PATCH] mempool: fix phys contig check if populate default skipped

[Thomas Monjalon]

01/02/2018 15:02, Andrew Rybchenko:
> 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>
> Acked-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>

Applied, thanks

[dpdk-stable] [PATCH v2 01/11] mempool: fix memhdr leak when no objects are populated

[Andrew Rybchenko]

Fixes: 84121f197187 ("mempool: store memory chunks in a list")
Cc: stable@dpdk.org

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
v1 -> v2:
 - added in v2 as discussed in [1]

[1] https://dpdk.org/ml/archives/dev/2018-March/093329.html

 lib/librte_mempool/rte_mempool.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 54f7f4b..80bf941 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -408,12 +408,18 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
 	}
 
 	/* not enough room to store one object */
-	if (i == 0)
-		return -EINVAL;
+	if (i == 0) {
+		ret = -EINVAL;
+		goto fail;
+	}
 
 	STAILQ_INSERT_TAIL(&mp->mem_list, memhdr, next);
 	mp->nb_mem_chunks++;
 	return i;
+
+fail:
+	rte_free(memhdr);
+	return ret;
 }
 
 int
-- 
2.7.4

[dpdk-stable] [PATCH v3 01/11] mempool: fix memhdr leak when no objects are populated

[Andrew Rybchenko]

Fixes: 84121f197187 ("mempool: store memory chunks in a list")
Cc: stable@dpdk.org

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
v2 -> v3:
 - none

v1 -> v2:
 - added in v2 as discussed in [1]

[1] https://dpdk.org/ml/archives/dev/2018-March/093329.html

 lib/librte_mempool/rte_mempool.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 54f7f4b..80bf941 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -408,12 +408,18 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
 	}
 
 	/* not enough room to store one object */
-	if (i == 0)
-		return -EINVAL;
+	if (i == 0) {
+		ret = -EINVAL;
+		goto fail;
+	}
 
 	STAILQ_INSERT_TAIL(&mp->mem_list, memhdr, next);
 	mp->nb_mem_chunks++;
 	return i;
+
+fail:
+	rte_free(memhdr);
+	return ret;
 }
 
 int
-- 
2.7.4

Re: [dpdk-stable] [PATCH v3 01/11] mempool: fix memhdr leak when no objects are populated

[Olivier Matz]

On Mon, Mar 26, 2018 at 05:09:41PM +0100, Andrew Rybchenko wrote:
> Fixes: 84121f197187 ("mempool: store memory chunks in a list")
> Cc: stable@dpdk.org
> 
> Suggested-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

[dpdk-stable] [PATCH v4 01/11] mempool: fix memhdr leak when no objects are populated

[Andrew Rybchenko]

Fixes: 84121f197187 ("mempool: store memory chunks in a list")
Cc: stable@dpdk.org

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
v3 -> v4:
 - none

v2 -> v3:
 - none

v1 -> v2:
 - added in v2 as discussed in [1]

[1] https://dpdk.org/ml/archives/dev/2018-March/093329.html

 lib/librte_mempool/rte_mempool.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 103c015..3b31a55 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -421,12 +421,18 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
 	}
 
 	/* not enough room to store one object */
-	if (i == 0)
-		return -EINVAL;
+	if (i == 0) {
+		ret = -EINVAL;
+		goto fail;
+	}
 
 	STAILQ_INSERT_TAIL(&mp->mem_list, memhdr, next);
 	mp->nb_mem_chunks++;
 	return i;
+
+fail:
+	rte_free(memhdr);
+	return ret;
 }
 
 int
-- 
2.7.4