[dpdk-dev,v1,8/9] mempool: ensure the mempool is initialized before populating
Checks
Commit Message
From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
Callback to calculate required memory area size may require mempool
driver data to be already allocated and initialized.
Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
RFCv2 -> v1:
- rename helper function as mempool_ops_alloc_once()
lib/librte_mempool/rte_mempool.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
Comments
On Sat, Mar 10, 2018 at 03:39:41PM +0000, Andrew Rybchenko wrote:
> From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
>
> Callback to calculate required memory area size may require mempool
> driver data to be already allocated and initialized.
>
> Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> RFCv2 -> v1:
> - rename helper function as mempool_ops_alloc_once()
>
> lib/librte_mempool/rte_mempool.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 844d907..12085cd 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -322,6 +322,21 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
> }
> }
>
> +static int
> +mempool_ops_alloc_once(struct rte_mempool *mp)
> +{
> + int ret;
> +
> + /* create the internal ring if not already done */
> + if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
> + ret = rte_mempool_ops_alloc(mp);
> + if (ret != 0)
> + return ret;
> + mp->flags |= MEMPOOL_F_POOL_CREATED;
> + }
> + return 0;
> +}
> +
> /* Add objects in the pool, using a physically contiguous memory
> * zone. Return the number of objects added, or a negative value
> * on error.
> @@ -336,13 +351,9 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
> struct rte_mempool_memhdr *memhdr;
> int ret;
>
> - /* create the internal ring if not already done */
> - if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
> - ret = rte_mempool_ops_alloc(mp);
> - if (ret != 0)
> - return ret;
> - mp->flags |= MEMPOOL_F_POOL_CREATED;
> - }
> + ret = mempool_ops_alloc_once(mp);
> + if (ret != 0)
> + return ret;
>
> /* mempool is already populated */
> if (mp->populated_size >= mp->size)
> @@ -515,6 +526,10 @@ rte_mempool_populate_default(struct rte_mempool *mp)
> unsigned mz_id, n;
> int ret;
>
> + ret = mempool_ops_alloc_once(mp);
> + if (ret != 0)
> + return ret;
> +
> /* mempool must not be populated */
> if (mp->nb_mem_chunks != 0)
> return -EEXIST;
Is there a reason why we need to add it in
rte_mempool_populate_default() but not in rte_mempool_populate_virt() and
rte_mempool_populate_iova_tab()?
On 03/19/2018 08:06 PM, Olivier Matz wrote:
> On Sat, Mar 10, 2018 at 03:39:41PM +0000, Andrew Rybchenko wrote:
>> From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
>>
>> Callback to calculate required memory area size may require mempool
>> driver data to be already allocated and initialized.
>>
>> Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>> RFCv2 -> v1:
>> - rename helper function as mempool_ops_alloc_once()
>>
>> lib/librte_mempool/rte_mempool.c | 29 ++++++++++++++++++++++-------
>> 1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 844d907..12085cd 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -322,6 +322,21 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
>> }
>> }
>>
>> +static int
>> +mempool_ops_alloc_once(struct rte_mempool *mp)
>> +{
>> + int ret;
>> +
>> + /* create the internal ring if not already done */
>> + if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
>> + ret = rte_mempool_ops_alloc(mp);
>> + if (ret != 0)
>> + return ret;
>> + mp->flags |= MEMPOOL_F_POOL_CREATED;
>> + }
>> + return 0;
>> +}
>> +
>> /* Add objects in the pool, using a physically contiguous memory
>> * zone. Return the number of objects added, or a negative value
>> * on error.
>> @@ -336,13 +351,9 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
>> struct rte_mempool_memhdr *memhdr;
>> int ret;
>>
>> - /* create the internal ring if not already done */
>> - if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
>> - ret = rte_mempool_ops_alloc(mp);
>> - if (ret != 0)
>> - return ret;
>> - mp->flags |= MEMPOOL_F_POOL_CREATED;
>> - }
>> + ret = mempool_ops_alloc_once(mp);
>> + if (ret != 0)
>> + return ret;
>>
>> /* mempool is already populated */
>> if (mp->populated_size >= mp->size)
>> @@ -515,6 +526,10 @@ rte_mempool_populate_default(struct rte_mempool *mp)
>> unsigned mz_id, n;
>> int ret;
>>
>> + ret = mempool_ops_alloc_once(mp);
>> + if (ret != 0)
>> + return ret;
>> +
>> /* mempool must not be populated */
>> if (mp->nb_mem_chunks != 0)
>> return -EEXIST;
>
> Is there a reason why we need to add it in
> rte_mempool_populate_default() but not in rte_mempool_populate_virt() and
> rte_mempool_populate_iova_tab()?
The reason is rte_mempool_ops_calc_mem_size() call
from rte_mempool_populate_default(). rte_mempool_ops_*() are not
called directly from rte_mempool_populate_virt() and
rte_mempool_populate_iova_tab().
In fact I've found out that rte_mempool_ops_calc_mem_size() is called
from get_anon_size() which is called from rte_mempool_populate_anon().
So, we need to add to get_anon_size() as well.
May be it is even better to make the patch the first in series to make
sure that it is already OK when rte_mempool_ops_calc_mem_size()
is added. What do you think?
On Tue, Mar 20, 2018 at 04:32:04PM +0300, Andrew Rybchenko wrote:
> On 03/19/2018 08:06 PM, Olivier Matz wrote:
> > On Sat, Mar 10, 2018 at 03:39:41PM +0000, Andrew Rybchenko wrote:
> > > From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
> > >
> > > Callback to calculate required memory area size may require mempool
> > > driver data to be already allocated and initialized.
> > >
> > > Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
> > > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > > ---
> > > RFCv2 -> v1:
> > > - rename helper function as mempool_ops_alloc_once()
> > >
> > > lib/librte_mempool/rte_mempool.c | 29 ++++++++++++++++++++++-------
> > > 1 file changed, 22 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> > > index 844d907..12085cd 100644
> > > --- a/lib/librte_mempool/rte_mempool.c
> > > +++ b/lib/librte_mempool/rte_mempool.c
> > > @@ -322,6 +322,21 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
> > > }
> > > }
> > > +static int
> > > +mempool_ops_alloc_once(struct rte_mempool *mp)
> > > +{
> > > + int ret;
> > > +
> > > + /* create the internal ring if not already done */
> > > + if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
> > > + ret = rte_mempool_ops_alloc(mp);
> > > + if (ret != 0)
> > > + return ret;
> > > + mp->flags |= MEMPOOL_F_POOL_CREATED;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > /* Add objects in the pool, using a physically contiguous memory
> > > * zone. Return the number of objects added, or a negative value
> > > * on error.
> > > @@ -336,13 +351,9 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
> > > struct rte_mempool_memhdr *memhdr;
> > > int ret;
> > > - /* create the internal ring if not already done */
> > > - if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
> > > - ret = rte_mempool_ops_alloc(mp);
> > > - if (ret != 0)
> > > - return ret;
> > > - mp->flags |= MEMPOOL_F_POOL_CREATED;
> > > - }
> > > + ret = mempool_ops_alloc_once(mp);
> > > + if (ret != 0)
> > > + return ret;
> > > /* mempool is already populated */
> > > if (mp->populated_size >= mp->size)
> > > @@ -515,6 +526,10 @@ rte_mempool_populate_default(struct rte_mempool *mp)
> > > unsigned mz_id, n;
> > > int ret;
> > > + ret = mempool_ops_alloc_once(mp);
> > > + if (ret != 0)
> > > + return ret;
> > > +
> > > /* mempool must not be populated */
> > > if (mp->nb_mem_chunks != 0)
> > > return -EEXIST;
> >
> > Is there a reason why we need to add it in
> > rte_mempool_populate_default() but not in rte_mempool_populate_virt() and
> > rte_mempool_populate_iova_tab()?
>
> The reason is rte_mempool_ops_calc_mem_size() call
> from rte_mempool_populate_default(). rte_mempool_ops_*() are not
> called directly from rte_mempool_populate_virt() and
> rte_mempool_populate_iova_tab().
>
> In fact I've found out that rte_mempool_ops_calc_mem_size() is called
> from get_anon_size() which is called from rte_mempool_populate_anon().
> So, we need to add to get_anon_size() as well.
>
> May be it is even better to make the patch the first in series to make
> sure that it is already OK when rte_mempool_ops_calc_mem_size()
> is added. What do you think?
Yes, sounds good.
Olivier
@@ -322,6 +322,21 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
}
}
+static int
+mempool_ops_alloc_once(struct rte_mempool *mp)
+{
+ int ret;
+
+ /* create the internal ring if not already done */
+ if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
+ ret = rte_mempool_ops_alloc(mp);
+ if (ret != 0)
+ return ret;
+ mp->flags |= MEMPOOL_F_POOL_CREATED;
+ }
+ return 0;
+}
+
/* Add objects in the pool, using a physically contiguous memory
* zone. Return the number of objects added, or a negative value
* on error.
@@ -336,13 +351,9 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
struct rte_mempool_memhdr *memhdr;
int ret;
- /* create the internal ring if not already done */
- if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
- ret = rte_mempool_ops_alloc(mp);
- if (ret != 0)
- return ret;
- mp->flags |= MEMPOOL_F_POOL_CREATED;
- }
+ ret = mempool_ops_alloc_once(mp);
+ if (ret != 0)
+ return ret;
/* mempool is already populated */
if (mp->populated_size >= mp->size)
@@ -515,6 +526,10 @@ rte_mempool_populate_default(struct rte_mempool *mp)
unsigned mz_id, n;
int ret;
+ ret = mempool_ops_alloc_once(mp);
+ if (ret != 0)
+ return ret;
+
/* mempool must not be populated */
if (mp->nb_mem_chunks != 0)
return -EEXIST;