[dpdk-dev] [RFC PATCH 2/6] mempool: implement clustered object allocation
Andrew Rybchenko
arybchenko at solarflare.com
Wed Jan 17 16:03:52 CET 2018
On 12/14/2017 04:37 PM, Olivier MATZ wrote:
> On Fri, Nov 24, 2017 at 04:06:27PM +0000, Andrew Rybchenko wrote:
>> From: "Artem V. Andreev" <Artem.Andreev at oktetlabs.ru>
>>
>> Clustered allocation is required to simplify packaging objects into
>> buckets and search of the bucket control structure by an object.
>>
>> Signed-off-by: Artem V. Andreev <Artem.Andreev at oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko at solarflare.com>
>> ---
>> lib/librte_mempool/rte_mempool.c | 39 +++++++++++++++++++++++++++++++++++----
>> lib/librte_mempool/rte_mempool.h | 23 +++++++++++++++++++++--
>> test/test/test_mempool.c | 2 +-
>> 3 files changed, 57 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index d50dba4..43455a3 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -239,7 +239,8 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
>> */
>> size_t
>> rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
>> - unsigned int flags)
>> + unsigned int flags,
>> + const struct rte_mempool_info *info)
>> {
>> size_t obj_per_page, pg_num, pg_sz;
>> unsigned int mask;
>> @@ -252,6 +253,17 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
>> if (total_elt_sz == 0)
>> return 0;
>>
>> + if (flags & MEMPOOL_F_CAPA_ALLOCATE_IN_CLUSTERS) {
>> + unsigned int align_shift =
>> + rte_bsf32(
>> + rte_align32pow2(total_elt_sz *
>> + info->cluster_size));
>> + if (pg_shift < align_shift) {
>> + return ((elt_num / info->cluster_size) + 2)
>> + << align_shift;
>> + }
>> + }
>> +
> +Cc Santosh for this
>
> To be honnest, that was my fear when introducing
> MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS and MEMPOOL_F_CAPA_PHYS_CONTIG to see more
> and more specific flags in generic code.
>
> I feel that the hidden meaning of these flags is more "if driver == foo",
> which shows that something is wrong is the current design.
>
> We have to think about another way to do. Let me try to propose
> something (to be deepen).
>
> The standard way to create a mempool is:
>
> mp = create_empty(...)
> set_ops_by_name(mp, "my-driver") // optional
> populate_default(mp) // or populate_*()
> obj_iter(mp, callback, arg) // optional, to init objects
> // and optional local func to init mempool priv
>
> First, we can consider deprecating some APIs like:
> - rte_mempool_xmem_create()
> - rte_mempool_xmem_size()
> - rte_mempool_xmem_usage()
> - rte_mempool_populate_iova_tab()
>
> These functions were introduced for xen, which was recently
> removed. They are complex to use, and are not used anywhere else in
> DPDK.
>
> Then, instead of having flags (quite hard to understand without knowing
> the underlying driver), we can let the mempool drivers do the
> populate_default() operation. For that we can add a populate_default
> field in mempool ops. Same for populate_virt(), populate_anon(), and
> populate_phys() which can return -ENOTSUP if this is not
> implemented/implementable on a specific driver, or if flags
> (NO_CACHE_ALIGN, NO_SPREAD, ...) are not supported. If the function
> pointer is NULL, use the generic function.
>
> Thanks to this, the generic code would remain understandable and won't
> have to care about how memory should be allocated for a specific driver.
>
> Thoughts?
Yes, I agree. This week we'll provide updated version of the RFC which
covers it including transition of the mempool/octeontx. I think it is
sufficient
to introduce two new ops:
1. To calculate memory space required to store specified number of objects
2. To populate objects in the provided memory chunk (the op will be called
from rte_mempool_populate_iova() which is a leaf function for all
rte_mempool_populate_*() calls.
It will allow to avoid duplication and keep memchunks housekeeping inside
mempool library.
> [...]
>
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index 3c59d36..9bcb8b7 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -220,7 +220,10 @@ struct rte_mempool_memhdr {
>> /*
>> * Additional information about the mempool
>> */
>> -struct rte_mempool_info;
>> +struct rte_mempool_info {
>> + /** Number of objects in a cluster */
>> + unsigned int cluster_size;
>> +};
> I think what I'm proposing would also prevent to introduce this
> structure, which is generic but only applies to this driver.
Yes
More information about the dev
mailing list