Hi Olivier,

Reply inline.

On Tue, Jan 31, 2017 at 11:31:51AM +0100, Olivier Matz wrote:

Hi Santosh,

I guess this patch is targeted for 17.05, right?

Yes.

Please see some other comments below.

On Fri, 20 Jan 2017 20:43:41 +0530, [off-list ref]
wrote:

quoted

From: Santosh Shukla <redacted>

HW pool manager e.g. Cavium SoC need s/w to program start and
end address of pool. Currently there is no such api in ext-mempool.

Today, the mempool objects are not necessarily contiguous in
virtual or physical memory. The only assumption that can be made is
that each object is contiguous (virtually and physically). If the flag
MEMPOOL_F_NO_PHYS_CONTIG is passed, each object is assured to be
contiguous virtually.

Some clarification: IIUC then pa/va addr for each hugepage (aka mz)
is contiguous but no gaurantee of contiguity across the hugepages (aka
muli-mzs), Right?

If above said is correct then case like pool start addr in one mz and
end addr is on another mz is a problem scenario. Correct? That said then
ext-mempool drivers will get affected in such cases.

quoted

So introducing _populate_mz_range API which will let HW(pool manager)
know about hugepage mapped virtual start and end address.

rte_mempool_ops_populate_mz_range() looks a bit long. What about
rte_mempool_ops_populate() instead?

Ok.

quoted

diff --git a/lib/librte_mempool/rte_mempool.c

b/lib/librte_mempool/rte_mempool.c index 1c2aed8..9a39f5c 100644

--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c

@@ -568,6 +568,10 @@ static unsigned optimize_object_size(unsigned

obj_size) else
 			paddr = mz->phys_addr;
 
+		/* Populate mz range */
+		if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0)
+			rte_mempool_ops_populate_mz_range(mp, mz);
+
 		if (rte_eal_has_hugepages()

Given what I've said above, I think the populate() callback should be
in rte_mempool_populate_phys() instead of
rte_mempool_populate_default(). It would be called for each contiguous
zone.

Ok.

quoted

--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h

@@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct

rte_mempool *mp, */
 typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool
*mp); +/**
+ * Set the memzone va/pa addr range and len in the external pool.
+ */
+typedef void (*rte_mempool_populate_mz_range_t)(struct rte_mempool
*mp,
+		const struct rte_memzone *mz);
+

And this API would be:
typedef void (*rte_mempool_populate_t)(struct rte_mempool *mp,
		char *vaddr, phys_addr_t paddr, size_t len)
 

If your hw absolutly needs a contiguous memory, a solution could be:

- add a new flag MEMPOOL_F_CONTIG (maybe a better nale could be
  found) saying that all the mempool objects must be contiguous.
- add the ops_populate() callback in rte_mempool_populate_phys(), as
  suggested above

Then:

  /* create an empty mempool */
  rte_mempool_create_empty(...);

  /* set the handler:
   *   in the ext handler, the mempool flags are updated with
   *   MEMPOOL_F_CONTIG
   */
  rte_mempool_set_ops_byname(..., "my_hardware");

You mean, ext handler will set mempool flag using 'pool_config' param; somethng
like rte_mempool_ops_by_name(..., "my_hardware" , MEMPOOL_F_CONTIG); ?

  /* if MEMPOOL_F_CONTIG is set, all populate() functions should ensure
   * that there is only one contiguous zone
   */
  rte_mempool_populate_default(...);

I am not too sure about scope of MEMPOOL_F_CONTIG. How MEMPOOL_F_CONTIG
flag {setted by application/ driver by calling rte_mempool_create(..., flag)}
can make sure only one contiguous zone? Like to understand from you.

In my understanding:
rte_mempool_populate_default() will request for memzone based on mp->size value;
And if mp->size more than one hugepage_sz{i.e. one mz request not enough} then
it will request more hugepages{ i.e. more mz request},. So IIIU then contiguity
not gauranteed.

Regards,
Olivier