On Tuesday 19 December 2017 04:32 PM, Olivier MATZ wrote:
> On Tue, Dec 19, 2017 at 04:16:33PM +0530, Hemant Agrawal wrote:
>> Hi Olivier,
>>
>> On 12/19/2017 3:54 PM, Olivier MATZ wrote:
>>> Hi Hemant,
>>>
>>> On Wed, Dec 06, 2017 at 06:01:12PM +0530, Hemant Agrawal wrote:
>>>> This is required for the optimizations w.r.t hw mempools.
>>>> They will use different kind of optimizations if the buffers
>>>> are from single contiguous memzone.
>>>>
>>>> Signed-off-by: Hemant Agrawal <hemant.agra...@nxp.com>
>>>> ---
>>>> lib/librte_mempool/rte_mempool.c | 7 +++++--
>>>> lib/librte_mempool/rte_mempool.h | 5 +++++
>>>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/librte_mempool/rte_mempool.c
>>>> b/lib/librte_mempool/rte_mempool.c
>>>> index d50dba4..9d3737c 100644
>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>> @@ -387,13 +387,16 @@ rte_mempool_populate_iova(struct rte_mempool *mp,
>>>> char *vaddr,
>>>> total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
>>>>
>>>> /* Detect pool area has sufficient space for elements */
>>>> - if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
>>>> - if (len < total_elt_sz * mp->size) {
>>>> + if (len < total_elt_sz * mp->size) {
>>>> + if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
>>>> RTE_LOG(ERR, MEMPOOL,
>>>> "pool area %" PRIx64 " not enough\n",
>>>> (uint64_t)len);
>>>> return -ENOSPC;
>>>> }
>>>> + } else {
>>>> + /* Memory will be allocated from multiple memzones */
>>>> + mp->flags |= MEMPOOL_F_MULTI_MEMZONE;
>>>> }
>>>>
>>>> memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0);
>>>> diff --git a/lib/librte_mempool/rte_mempool.h
>>>> b/lib/librte_mempool/rte_mempool.h
>>>> index 721227f..394a4fe 100644
>>>> --- a/lib/librte_mempool/rte_mempool.h
>>>> +++ b/lib/librte_mempool/rte_mempool.h
>>>> @@ -292,6 +292,11 @@ struct rte_mempool {
>>>> */
>>>> #define MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS 0x0080
>>>>
>>>> +/* Indicates that the mempool buffers are allocated from multiple memzones
>>>> + * the buffer may or may not be physically contiguous.
>>>> + */
>>>> +#define MEMPOOL_F_MULTI_MEMZONE 0x0100
>>>> +
>>>> /**
>>>> * @internal When debug is enabled, store some statistics.
>>>> *
>>>> --
>>>> 2.7.4
>>>>
>>> I'm not confortable with adding more and more flags, as I explained
>>> here: http://dpdk.org/ml/archives/dev/2017-December/083909.html
>> This particular flag is not about how to populate mempool. This is just
>> indicating how the mempool was populated - a status flag. This information
>> is just helpful for the PMDs.
>>
>> At least I am not able to see that this particular flag is being very driver
>> specific.
> That's true, I commented too fast :)
> And what about using mp->nb_mem_chunks instead? Would it do the job
> in your use-case?
>
>
>>> It makes the generic code very complex, and probably buggy (many
>>> flags are incompatible with other flags).
>>>
>>> I'm thinking about moving the populate_* functions in the drivers
>>> (this is described a bit more in the link above). What do you think
>>> about this approach?
>>>
>> The idea is good and it will give fine control to the individual mempools to
>> populate the memory the way they want. However, on the downside, it will
>> also lead to lot of duplicate code or similar code. It may also lead to a
>> maintenance issue for the mempool PMD owner.
> Yes, that will be the drawback. If we do this, we should try to keep some
> common helpers in the mempool lib.
Sorry for jumping late on this and not responding to other thread.
Olivier, We in-fact I tried said approach for ONA mempool driver but never
proposed ;) for the reason which
was pointed by Hemant.. meaning more code duplication across mempool PMD thus
more maintenance burden.
However, I'm in favor of giving more control to driver.
