[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
Until now, the objects stored in a mempool were internally stored in a
ring. This patch introduces the possibility to register external handlers
replacing the ring.
The default behavior remains unchanged, but calling the new function
rte_mempool_set_handler() right after rte_mempool_create_empty() allows
the user to change the handler that will be used when populating
the mempool.
v7 changes:
* Moved the flags handling from rte_mempool_create_empty to
rte_mempool_create, as it's only there for backward compatibility
* Various comment additions and cleanup
* Renamed rte_mempool_handler to rte_mempool_ops
* Added a union for *pool and u64 pool_id in struct rte_mempool
v6 changes:
* split the original patch into a few parts for easier review.
* rename functions with _ext_ to _ops_.
* addressed some review comments
* renamed put and get functions to enqueue and dequeue
* renamed rte_mempool_handler struct to rte_mempool_handler_ops
* changed occurences of rte_mempool_handler_ops to const, as they
contain function pointers (security)
* added some extra comments
v5 changes: rebasing on top of 35 patch set mempool work.
Signed-off-by: Olivier Matz
Signed-off-by: David Hunt
---
lib/librte_mempool/Makefile | 1 +
lib/librte_mempool/rte_mempool.c | 71 --
lib/librte_mempool/rte_mempool.h | 235 ---
lib/librte_mempool/rte_mempool_handler.c | 141 +++
4 files changed, 384 insertions(+), 64 deletions(-)
create mode 100644 lib/librte_mempool/rte_mempool_handler.c
diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
index 43423e0..793745f 100644
--- a/lib/librte_mempool/Makefile
+++ b/lib/librte_mempool/Makefile
@@ -42,6 +42,7 @@ LIBABIVER := 2
# all source are stored in SRCS-y
SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool.c
+SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_handler.c
# install includes
SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index b54de43..9f34d30 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj,
phys_addr_t physaddr)
#endif
/* enqueue in ring */
- rte_ring_sp_enqueue(mp->ring, obj);
+ rte_mempool_ops_enqueue_bulk(mp, &obj, 1);
}
/* call obj_cb() for each mempool element */
@@ -303,40 +303,6 @@ rte_mempool_xmem_usage(__rte_unused void *vaddr, uint32_t
elt_num,
return (size_t)paddr_idx << pg_shift;
}
-/* create the internal ring */
-static int
-rte_mempool_ring_create(struct rte_mempool *mp)
-{
- int rg_flags = 0, ret;
- char rg_name[RTE_RING_NAMESIZE];
- struct rte_ring *r;
-
- ret = snprintf(rg_name, sizeof(rg_name),
- RTE_MEMPOOL_MZ_FORMAT, mp->name);
- if (ret < 0 || ret >= (int)sizeof(rg_name))
- return -ENAMETOOLONG;
-
- /* ring flags */
- if (mp->flags & MEMPOOL_F_SP_PUT)
- rg_flags |= RING_F_SP_ENQ;
- if (mp->flags & MEMPOOL_F_SC_GET)
- rg_flags |= RING_F_SC_DEQ;
-
- /* Allocate the ring that will be used to store objects.
-* Ring functions will return appropriate errors if we are
-* running as a secondary process etc., so no checks made
-* in this function for that condition.
-*/
- r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
- mp->socket_id, rg_flags);
- if (r == NULL)
- return -rte_errno;
-
- mp->ring = r;
- mp->flags |= MEMPOOL_F_RING_CREATED;
- return 0;
-}
-
/* free a memchunk allocated with rte_memzone_reserve() */
static void
rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr *memhdr,
@@ -354,7 +320,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
void *elt;
while (!STAILQ_EMPTY(&mp->elt_list)) {
- rte_ring_sc_dequeue(mp->ring, &elt);
+ rte_mempool_ops_dequeue_bulk(mp, &elt, 1);
(void)elt;
STAILQ_REMOVE_HEAD(&mp->elt_list, next);
mp->populated_size--;
@@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
*vaddr,
unsigned i = 0;
size_t off;
struct rte_mempool_memhdr *memhdr;
- int ret;
/* create the internal ring if not already done */
if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
- ret = rte_mempool_ring_create(mp);
- if (ret < 0)
- return ret;
+ rte_errno = 0;
+ mp->pool = rte_mempool_ops_alloc(mp);
+ if (mp->pool == NULL) {
+ if (rte_errno == 0)
+ return -EINVAL;
+ return -rte_errno;
+ }
}
/* mempool
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On 6/1/2016 5:19 PM, David Hunt wrote: > Until now, the objects stored in a mempool were internally stored in a > ring. This patch introduces the possibility to register external handlers > replacing the ring. > > The default behavior remains unchanged, but calling the new function > rte_mempool_set_handler() right after rte_mempool_create_empty() allows > the user to change the handler that will be used when populating > the mempool. > > v7 changes: >* Moved the flags handling from rte_mempool_create_empty to > rte_mempool_create, as it's only there for backward compatibility >* Various comment additions and cleanup >* Renamed rte_mempool_handler to rte_mempool_ops >* Added a union for *pool and u64 pool_id in struct rte_mempool These v7 changes should me merged with the v6 changes below as this is a v6 patch. Or removed altogether, as they are in the cover letter. > v6 changes: >* split the original patch into a few parts for easier review. >* rename functions with _ext_ to _ops_. >* addressed some review comments >* renamed put and get functions to enqueue and dequeue >* renamed rte_mempool_handler struct to rte_mempool_handler_ops >* changed occurences of rte_mempool_handler_ops to const, as they > contain function pointers (security) >* added some extra comments > > [...]
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
Hello David,
the rename s/handler/ops/ has a lot of residues. Sorry for that :). I tried to
mark most of them. Otherwise, I couldn't see many more serious issues for now.
Just, note the s/pool/priv/ rename suggestion.
On Wed, 1 Jun 2016 17:19:54 +0100
David Hunt wrote:
> Until now, the objects stored in a mempool were internally stored in a
> ring. This patch introduces the possibility to register external handlers
> replacing the ring.
>
> The default behavior remains unchanged, but calling the new function
> rte_mempool_set_handler() right after rte_mempool_create_empty() allows
> the user to change the handler that will be used when populating
> the mempool.
>
> v7 changes:
> * Moved the flags handling from rte_mempool_create_empty to
> rte_mempool_create, as it's only there for backward compatibility
> * Various comment additions and cleanup
> * Renamed rte_mempool_handler to rte_mempool_ops
> * Added a union for *pool and u64 pool_id in struct rte_mempool
>
> v6 changes:
> * split the original patch into a few parts for easier review.
> * rename functions with _ext_ to _ops_.
> * addressed some review comments
> * renamed put and get functions to enqueue and dequeue
> * renamed rte_mempool_handler struct to rte_mempool_handler_ops
> * changed occurences of rte_mempool_handler_ops to const, as they
> contain function pointers (security)
> * added some extra comments
>
> v5 changes: rebasing on top of 35 patch set mempool work.
>
> Signed-off-by: Olivier Matz
> Signed-off-by: David Hunt
> ---
> lib/librte_mempool/Makefile | 1 +
> lib/librte_mempool/rte_mempool.c | 71 --
> lib/librte_mempool/rte_mempool.h | 235
> ---
> lib/librte_mempool/rte_mempool_handler.c | 141 +++
> 4 files changed, 384 insertions(+), 64 deletions(-)
> create mode 100644 lib/librte_mempool/rte_mempool_handler.c
>
> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
> index 43423e0..793745f 100644
> --- a/lib/librte_mempool/Makefile
> +++ b/lib/librte_mempool/Makefile
> @@ -42,6 +42,7 @@ LIBABIVER := 2
>
> # all source are stored in SRCS-y
> SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_handler.c
> # install includes
> SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>
> diff --git a/lib/librte_mempool/rte_mempool.c
> b/lib/librte_mempool/rte_mempool.c
> index b54de43..9f34d30 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj,
> phys_addr_t physaddr)
> #endif
>
> /* enqueue in ring */
> - rte_ring_sp_enqueue(mp->ring, obj);
> + rte_mempool_ops_enqueue_bulk(mp, &obj, 1);
> }
>
> /* call obj_cb() for each mempool element */
> @@ -303,40 +303,6 @@ rte_mempool_xmem_usage(__rte_unused void *vaddr,
> uint32_t elt_num,
> return (size_t)paddr_idx << pg_shift;
> }
>
> -/* create the internal ring */
> -static int
> -rte_mempool_ring_create(struct rte_mempool *mp)
> -{
> - int rg_flags = 0, ret;
> - char rg_name[RTE_RING_NAMESIZE];
> - struct rte_ring *r;
> -
> - ret = snprintf(rg_name, sizeof(rg_name),
> - RTE_MEMPOOL_MZ_FORMAT, mp->name);
> - if (ret < 0 || ret >= (int)sizeof(rg_name))
> - return -ENAMETOOLONG;
> -
> - /* ring flags */
> - if (mp->flags & MEMPOOL_F_SP_PUT)
> - rg_flags |= RING_F_SP_ENQ;
> - if (mp->flags & MEMPOOL_F_SC_GET)
> - rg_flags |= RING_F_SC_DEQ;
> -
> - /* Allocate the ring that will be used to store objects.
> - * Ring functions will return appropriate errors if we are
> - * running as a secondary process etc., so no checks made
> - * in this function for that condition.
> - */
> - r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
> - mp->socket_id, rg_flags);
> - if (r == NULL)
> - return -rte_errno;
> -
> - mp->ring = r;
> - mp->flags |= MEMPOOL_F_RING_CREATED;
> - return 0;
> -}
> -
> /* free a memchunk allocated with rte_memzone_reserve() */
> static void
> rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr *memhdr,
> @@ -354,7 +320,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
> void *elt;
>
> while (!STAILQ_EMPTY(&mp->elt_list)) {
> - rte_ring_sc_dequeue(mp->ring, &elt);
> + rte_mempool_ops_dequeue_bulk(mp, &elt, 1);
> (void)elt;
> STAILQ_REMOVE_HEAD(&mp->elt_list, next);
> mp->populated_size--;
> @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
> *vaddr,
> unsigned i = 0;
> size_t off;
> struct rte_mempool_memhdr *memhdr;
> - int ret;
>
> /* create the internal ring if not already done */
> if ((mp->flags & M
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On 6/1/2016 6:54 PM, Jan Viktorin wrote: > Hello David, > > the rename s/handler/ops/ has a lot of residues. Sorry for that :). I tried to > mark most of them. Otherwise, I couldn't see many more serious issues for now. Ah, I had assumed that we were just talking about the rte_mempool_handler_ops structure, not a global replace of 'handler' with 'ops'. It does make sense to change it to ops, so we don't have two words describing the same entity. I'll change to ops. Just, note the s/pool/priv/ rename suggestion. I prefer your suggestion of pdata rather than priv, how about "pool_data"? Again, thanks for the comprehensive review. Regards, Dave. [...]
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On 6/1/2016 6:54 PM, Jan Viktorin wrote:
>
> mp->populated_size--;
> @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
> *vaddr,
> unsigned i = 0;
> size_t off;
> struct rte_mempool_memhdr *memhdr;
> - int ret;
>
> /* create the internal ring if not already done */
> if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> - ret = rte_mempool_ring_create(mp);
> - if (ret < 0)
> - return ret;
> + rte_errno = 0;
> + mp->pool = rte_mempool_ops_alloc(mp);
> + if (mp->pool == NULL) {
> + if (rte_errno == 0)
> + return -EINVAL;
> + return -rte_errno;
> Are you sure the rte_errno is a positive value?
If the user returns EINVAL, or similar, we want to return negative, as
per the rest of DPDK.
>> @@ -204,9 +205,13 @@ struct rte_mempool_memhdr {
>> struct rte_mempool {
>> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
>> struct rte_ring *ring; /**< Ring to store objects. */
>> +union {
>> +void *pool; /**< Ring or pool to store objects */
> What about calling this pdata or priv? I think, it can improve some doc
> comments.
> Describing a "pool" may refer to both the rte_mempool itself or to the
> mp->pool
> pointer. The "priv" would help to understand the code a bit.
>
> Then the rte_mempool_alloc_t can be called rte_mempool_priv_alloc_t. Or
> something
> similar. It's than clear enough, what the function should do...
I'd lean towards pdata or maybe pool_data. Not sure about the function
name change though,
the function does return a pool_data pointer, which we put into
mp->pool_data.
>> +uint64_t *pool_id; /**< External mempool identifier */
> Why is pool_id a pointer? Is it a typo? I've understood it should be 64b wide
> from the discussion (Olivier, Jerin, David):
Yes, typo.
> uint32_t trailer_size; /**< Size of trailer (after elt). */
>
> unsigned private_data_size; /**< Size of private data. */
> + /**
> + * Index into rte_mempool_handler_table array of mempool handler ops
> s/rte_mempool_handler_table/rte_mempool_ops_table/
Done.
>> + * structs, which contain callback function pointers.
>> + * We're using an index here rather than pointers to the callbacks
>> + * to facilitate any secondary processes that may want to use
>> + * this mempool.
>> + */
>> +int32_t handler_idx;
> s/handler_idx/ops_idx/
>
> What about ops_index? Not a big deal...
I agree ops_index is better. Changed.
>>
>> struct rte_mempool_cache *local_cache; /**< Per-lcore local cache */
>>
>> @@ -325,6 +338,199 @@ void rte_mempool_check_cookies(const struct
>> rte_mempool *mp,
>> #define __mempool_check_cookies(mp, obj_table_const, n, free) do {}
>> while(0)
>> #endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
>>
>> +#define RTE_MEMPOOL_HANDLER_NAMESIZE 32 /**< Max length of handler name. */
>> +
>> +/**
>> + * typedef for allocating the external pool.
> What about:
>
> function prototype to provide an implementation specific data
>
>> + * The handler's alloc function does whatever it needs to do to grab memory
>> + * for this handler, and sets the *pool opaque pointer in the rte_mempool
>> + * struct. In the default handler, *pool points to a ring,in other handlers,
> What about:
>
> The function should provide a memory heap representation or another private
> data
> used for allocation by the rte_mempool_ops. E.g. the default ops provides an
> instance of the rte_ring for this purpose.
>
>> + * it will mostlikely point to a different type of data structure.
>> + * It will be transparent to the application programmer.
> I'd add something like this:
>
> The function should not touch the given *mp instance.
Agreed. Reworked somewhat.
> ...because it's goal is NOT to set the mp->pool, this is done by the
> rte_mempool_populate_phys - the caller of this rte_mempool_ops_alloc.
>
> This is why I've suggested to pass the rte_mempool as const in the v5.
> Is there any reason to modify the rte_mempool contents by the implementation?
> I think, we just need to read the flags, socket_id, etc.
Yes, I agree it should be const. Changed.
>> + */
>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>> +
>> +/** Free the external pool opaque data (that pointed to by *pool) */
> What about:
>
> /** Free the opaque private data stored in the mp->pool pointer. */
I've merged the two versions of the comment:
/** Free the opaque private data pointed to by mp->pool_data pointer */
>> +typedef void (*rte_mempool_free_t)(void *p);
>> +
>> +/**
>> + * Put an object in the external pool.
>> + * The *p pointer is the opaque data for a given mempool handler (ring,
>> + * array, linked list, etc)
> The obj_table is not documented. Is it really a table? I'd called an ar
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On Thu, 2 Jun 2016 12:23:41 +0100
"Hunt, David" wrote:
> On 6/1/2016 6:54 PM, Jan Viktorin wrote:
> >
> > mp->populated_size--;
> > @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp,
> > char *vaddr,
> > unsigned i = 0;
> > size_t off;
> > struct rte_mempool_memhdr *memhdr;
> > - int ret;
> >
> > /* create the internal ring if not already done */
> > if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> > - ret = rte_mempool_ring_create(mp);
> > - if (ret < 0)
> > - return ret;
> > + rte_errno = 0;
> > + mp->pool = rte_mempool_ops_alloc(mp);
> > + if (mp->pool == NULL) {
> > + if (rte_errno == 0)
> > + return -EINVAL;
> > + return -rte_errno;
> > Are you sure the rte_errno is a positive value?
>
> If the user returns EINVAL, or similar, we want to return negative, as
> per the rest of DPDK.
Oh, yes... you're right.
>
> >> @@ -204,9 +205,13 @@ struct rte_mempool_memhdr {
> >> struct rte_mempool {
> >>char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
> >>struct rte_ring *ring; /**< Ring to store objects. */
> >> + union {
> >> + void *pool; /**< Ring or pool to store objects */
> > What about calling this pdata or priv? I think, it can improve some doc
> > comments.
> > Describing a "pool" may refer to both the rte_mempool itself or to the
> > mp->pool
> > pointer. The "priv" would help to understand the code a bit.
> >
> > Then the rte_mempool_alloc_t can be called rte_mempool_priv_alloc_t. Or
> > something
> > similar. It's than clear enough, what the function should do...
>
> I'd lean towards pdata or maybe pool_data. Not sure about the function
> name change though,
> the function does return a pool_data pointer, which we put into
> mp->pool_data.
Yes. But from the name of the function, it is difficult to understand what is
its
purpose.
>
> >> + uint64_t *pool_id; /**< External mempool identifier */
> > Why is pool_id a pointer? Is it a typo? I've understood it should be 64b
> > wide
> > from the discussion (Olivier, Jerin, David):
>
[...]
>
> >> +typedef void (*rte_mempool_free_t)(void *p);
> >> +
> >> +/**
> >> + * Put an object in the external pool.
> >> + * The *p pointer is the opaque data for a given mempool handler (ring,
> >> + * array, linked list, etc)
> > The obj_table is not documented. Is it really a table? I'd called an array
> > instead.
>
> You're probably right, but it's always been called obj_table, and I'm
> not sure
> this patchset is the place to change it. Maybe a follow up patch?
In that case, it's OK.
>
> >> + */
> >> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table,
> >> unsigned n);
> > unsigned int
> Done
> >
> >> +
> >> +/** Get an object from the external pool. */
> >> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n);
> > unsigned int
> Done
> >
> >> +
> >> +/** Return the number of available objects in the external pool. */
> > Is the number of available objects or the total number of all objects
> > (so probably a constant value)?
>
> It's intended to be the umber of available objects
OK, forget it.
>
> >
> >> +typedef unsigned (*rte_mempool_get_count)(void *p);
> > Should it be const void *p?
>
> Yes, it could be. Changed.
>
> >
[...]
>
> >> + * @return
> >> + * - 0: Sucess; the new handler is configured.
> >> + * - EINVAL - Invalid handler name provided
> >> + * - EEXIST - mempool already has a handler assigned
> > They are returned as -EINVAL and -EEXIST.
> >
> > IMHO, using "-" here is counter-intuitive:
> >
> > - EINVAL
> >
> > does it mean a positive with "-" or negative value?
>
> EINVAL is positive, so it's returning negative. Common usage in DPDK,
> afaics.
Yes, of course. But it is not so clear from the doc. I've already wrote
a code checking the positive error codes. That was because of no "minus
sign" in the doc. So, my code was wrong and it took me a while to find
out the reason... I supposed, the positive value was intentional. Finally,
I had to lookup the source code (the calling path) to verify...
>
>
> >> + */
> >> +int
> >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
> > rte_mempool_set_ops
> >
> > What about rte_mempool_set_ops_byname? Not a big deal...
>
> I agree. rte_mempool_set_ops_byname
>
> >> +
> >> +/**
> >> + * Register an external pool handler.
> > Register mempool operations
>
> Done
>
> >> + *
> >> + * @param h
> >> + * Pointer to the external pool handler
> >> + * @return
> >> + * - >=0: Sucess; return the index of the handler in the table.
> >> + * - EINVAL - missing callbacks while registering handler
> >> + * - ENOSPC - the maximum number of handlers has been reached
> > - -EINVAL
> > - -ENOSPC
>
> :)
Simila
