diff mbox series

[4/5] mempool: introduce function to get mempool page size

Message ID 20191028140122.9592-5-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series mempool: avoid objects allocations across pages |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Olivier Matz Oct. 28, 2019, 2:01 p.m. UTC 
  In rte_mempool_populate_default(), we determine the page size,
which is needed for calc_size and allocation of memory.

Move this in a function and export it, it will be used in next
commit.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_mempool/rte_mempool.c           | 51 ++++++++++++++--------
 lib/librte_mempool/rte_mempool.h           |  7 +++
 lib/librte_mempool/rte_mempool_version.map |  1 +
 3 files changed, 40 insertions(+), 19 deletions(-)

Comments

Andrew Rybchenko Oct. 29, 2019, 10:31 a.m. UTC | #1 
On 10/28/19 5:01 PM, Olivier Matz wrote:
> In rte_mempool_populate_default(), we determine the page size,
> which is needed for calc_size and allocation of memory.
>
> Move this in a function and export it, it will be used in next
> commit.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

One question below:
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

[snip]

> diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> index 17cbca460..4eff2767d 100644
> --- a/lib/librte_mempool/rte_mempool_version.map
> +++ b/lib/librte_mempool/rte_mempool_version.map
> @@ -56,5 +56,6 @@ DPDK_18.05 {
>   EXPERIMENTAL {
>   	global:
>   
> +	rte_mempool_get_page_size;
>   	rte_mempool_ops_get_info;
>   };

Should internal function be here?
Olivier Matz Oct. 29, 2019, 5:20 p.m. UTC | #2 
On Tue, Oct 29, 2019 at 01:31:22PM +0300, Andrew Rybchenko wrote:
> On 10/28/19 5:01 PM, Olivier Matz wrote:
> > In rte_mempool_populate_default(), we determine the page size,
> > which is needed for calc_size and allocation of memory.
> > 
> > Move this in a function and export it, it will be used in next
> > commit.
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> One question below:
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> [snip]
> 
> > diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> > index 17cbca460..4eff2767d 100644
> > --- a/lib/librte_mempool/rte_mempool_version.map
> > +++ b/lib/librte_mempool/rte_mempool_version.map
> > @@ -56,5 +56,6 @@ DPDK_18.05 {
> >   EXPERIMENTAL {
> >   	global:
> > +	rte_mempool_get_page_size;
> >   	rte_mempool_ops_get_info;
> >   };
> 
> Should internal function be here?
> 

Good question. Let me ask a friend ;)
Olivier Matz Oct. 30, 2019, 8:32 a.m. UTC | #3 
On Tue, Oct 29, 2019 at 06:20:47PM +0100, Olivier Matz wrote:
> On Tue, Oct 29, 2019 at 01:31:22PM +0300, Andrew Rybchenko wrote:
> > On 10/28/19 5:01 PM, Olivier Matz wrote:
> > > In rte_mempool_populate_default(), we determine the page size,
> > > which is needed for calc_size and allocation of memory.
> > > 
> > > Move this in a function and export it, it will be used in next
> > > commit.
> > > 
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > 
> > One question below:
> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > 
> > [snip]
> > 
> > > diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> > > index 17cbca460..4eff2767d 100644
> > > --- a/lib/librte_mempool/rte_mempool_version.map
> > > +++ b/lib/librte_mempool/rte_mempool_version.map
> > > @@ -56,5 +56,6 @@ DPDK_18.05 {
> > >   EXPERIMENTAL {
> > >   	global:
> > > +	rte_mempool_get_page_size;
> > >   	rte_mempool_ops_get_info;
> > >   };
> > 
> > Should internal function be here?
> > 
> 
> Good question. Let me ask a friend ;)

I was influenced by a warning saying "rte_mempool_get_page_size is
flagged as experimental but is not listed in version map", but actually
it should not be flagged as experimental. I'll remove both.

My friend also suggested me to add it in a private header, which is a
good idea, but I think it should be in another patch because there are
already several functions in this case.
Olivier Matz Oct. 30, 2019, 2:29 p.m. UTC | #4 
On Wed, Oct 30, 2019 at 09:32:08AM +0100, Olivier Matz wrote:
> On Tue, Oct 29, 2019 at 06:20:47PM +0100, Olivier Matz wrote:
> > On Tue, Oct 29, 2019 at 01:31:22PM +0300, Andrew Rybchenko wrote:
> > > On 10/28/19 5:01 PM, Olivier Matz wrote:
> > > > In rte_mempool_populate_default(), we determine the page size,
> > > > which is needed for calc_size and allocation of memory.
> > > > 
> > > > Move this in a function and export it, it will be used in next
> > > > commit.
> > > > 
> > > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > 
> > > One question below:
> > > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > > 
> > > [snip]
> > > 
> > > > diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> > > > index 17cbca460..4eff2767d 100644
> > > > --- a/lib/librte_mempool/rte_mempool_version.map
> > > > +++ b/lib/librte_mempool/rte_mempool_version.map
> > > > @@ -56,5 +56,6 @@ DPDK_18.05 {
> > > >   EXPERIMENTAL {
> > > >   	global:
> > > > +	rte_mempool_get_page_size;
> > > >   	rte_mempool_ops_get_info;
> > > >   };
> > > 
> > > Should internal function be here?
> > > 
> > 
> > Good question. Let me ask a friend ;)
> 
> I was influenced by a warning saying "rte_mempool_get_page_size is
> flagged as experimental but is not listed in version map", but actually
> it should not be flagged as experimental. I'll remove both.
> 
> My friend also suggested me to add it in a private header, which is a
> good idea, but I think it should be in another patch because there are
> already several functions in this case.

Finally, I had to keep it in the API, because the octeontx2 driver will
need it. I also kept the @internal tag in the comment, because I think
this function should only be used in mempool drivers.
diff mbox series

Patch

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 5e1f202dc..7664764e5 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -411,6 +411,33 @@  rte_mempool_populate_virt(struct rte_mempool *mp, char *addr,
 	return ret;
 }
 
+/* Get the minimal page size used in a mempool before populating it. */
+int
+rte_mempool_get_page_size(struct rte_mempool *mp, size_t *pg_sz)
+{
+	bool need_iova_contig_obj;
+	bool alloc_in_ext_mem;
+	int ret;
+
+	/* check if we can retrieve a valid socket ID */
+	ret = rte_malloc_heap_socket_is_external(mp->socket_id);
+	if (ret < 0)
+		return -EINVAL;
+	alloc_in_ext_mem = (ret == 1);
+	need_iova_contig_obj = !(mp->flags & MEMPOOL_F_NO_IOVA_CONTIG);
+
+	if (!need_iova_contig_obj)
+		*pg_sz = 0;
+	else if (!alloc_in_ext_mem && rte_eal_iova_mode() == RTE_IOVA_VA)
+		*pg_sz = 0;
+	else if (rte_eal_has_hugepages() || alloc_in_ext_mem)
+		*pg_sz = get_min_page_size(mp->socket_id);
+	else
+		*pg_sz = getpagesize();
+
+	return 0;
+}
+
 /* Default function to populate the mempool: allocate memory in memzones,
  * and populate them. Return the number of objects added, or a negative
  * value on error.
@@ -422,12 +449,11 @@  rte_mempool_populate_default(struct rte_mempool *mp)
 	char mz_name[RTE_MEMZONE_NAMESIZE];
 	const struct rte_memzone *mz;
 	ssize_t mem_size;
-	size_t align, pg_sz, pg_shift;
+	size_t align, pg_sz, pg_shift = 0;
 	rte_iova_t iova;
 	unsigned mz_id, n;
 	int ret;
 	bool need_iova_contig_obj;
-	bool alloc_in_ext_mem;
 
 	ret = mempool_ops_alloc_once(mp);
 	if (ret != 0)
@@ -482,26 +508,13 @@  rte_mempool_populate_default(struct rte_mempool *mp)
 	 * synonymous with IOVA contiguousness will not hold.
 	 */
 
-	/* check if we can retrieve a valid socket ID */
-	ret = rte_malloc_heap_socket_is_external(mp->socket_id);
-	if (ret < 0)
-		return -EINVAL;
-	alloc_in_ext_mem = (ret == 1);
 	need_iova_contig_obj = !(mp->flags & MEMPOOL_F_NO_IOVA_CONTIG);
+	ret = rte_mempool_get_page_size(mp, &pg_sz);
+	if (ret < 0)
+		return ret;
 
-	if (!need_iova_contig_obj) {
-		pg_sz = 0;
-		pg_shift = 0;
-	} else if (!alloc_in_ext_mem && rte_eal_iova_mode() == RTE_IOVA_VA) {
-		pg_sz = 0;
-		pg_shift = 0;
-	} else if (rte_eal_has_hugepages() || alloc_in_ext_mem) {
-		pg_sz = get_min_page_size(mp->socket_id);
-		pg_shift = rte_bsf32(pg_sz);
-	} else {
-		pg_sz = getpagesize();
+	if (pg_sz != 0)
 		pg_shift = rte_bsf32(pg_sz);
-	}
 
 	for (mz_id = 0, n = mp->size; n > 0; mz_id++, n -= ret) {
 		size_t min_chunk_size;
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 8fa3c04e5..6d98b3743 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -1691,6 +1691,13 @@  uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 void rte_mempool_walk(void (*func)(struct rte_mempool *, void *arg),
 		      void *arg);
 
+/**
+ * @internal Get page size used for mempool object allocation.
+ */
+__rte_experimental
+int
+rte_mempool_get_page_size(struct rte_mempool *mp, size_t *pg_sz);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
index 17cbca460..4eff2767d 100644
--- a/lib/librte_mempool/rte_mempool_version.map
+++ b/lib/librte_mempool/rte_mempool_version.map
@@ -56,5 +56,6 @@  DPDK_18.05 {
 EXPERIMENTAL {
 	global:
 
+	rte_mempool_get_page_size;
 	rte_mempool_ops_get_info;
 };