On Wednesday 17 January 2018 08:33 PM, Andrew Rybchenko wrote:
> 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.andr...@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.andr...@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <arybche...@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.
>
There is also a downside of letting mempool driver to populate, which was
raised in other thread.
http://dpdk.org/dev/patchwork/patch/31943/
Thanks.
