[dpdk-dev] [RFC PATCH 2/6] mempool: implement clustered object allocation

[Andrew Rybchenko]

On 01/17/2018 06:55 PM, santosh wrote:
> 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.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.
>>
> There is also a downside of letting mempool driver to populate, which was raised in other thread.
> http://dpdk.org/dev/patchwork/patch/31943/

I've seen the note about code duplication. Let's discuss it when v2 is sent.
I think our approach minimizes it and allows to have only specific code 
in the
driver callback.