Re: [dpdk-dev] Fwd: [PATCH v2 1/5] mem: add function for checking memsegs IOVAs addresses

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 04-Oct-18 1:59 PM, Alejandro Lucero wrote:
> I sent this email only to Anatoly. Sending it again to mailing list.
> 
> On Wed, Oct 3, 2018 at 1:43 PM Burakov, Anatoly <anatoly.burakov@intel.com>
> wrote:
> 
>> On 31-Aug-18 1:50 PM, Alejandro Lucero wrote:
>>> A device can suffer addressing limitations. This functions checks
>>> memsegs have iovas within the supported range based on dma mask.
>>>
>>> PMD should use this during initialization if supported devices
>>> suffer addressing limitations, returning an error if this function
>>> returns memsegs out of range.
>>>
>>> Another potential usage is for emulated IOMMU hardware with addressing
>>> limitations.
>>>
>>> It is necessary to save the most restricted dma mask for checking
>>> memory allocated dynamically after initialization.
>>>
>>> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
>>> ---
>>>    lib/librte_eal/common/eal_common_memory.c         | 56
>> +++++++++++++++++++++++
>>>    lib/librte_eal/common/include/rte_eal_memconfig.h |  3 ++
>>>    lib/librte_eal/common/include/rte_memory.h        |  3 ++
>>>    lib/librte_eal/common/malloc_heap.c               | 12 +++++
>>>    lib/librte_eal/linuxapp/eal/eal.c                 |  2 +
>>>    lib/librte_eal/rte_eal_version.map                |  1 +
>>>    6 files changed, 77 insertions(+)
>>>
>>> diff --git a/lib/librte_eal/common/eal_common_memory.c
>> b/lib/librte_eal/common/eal_common_memory.c
>>> index fbfb1b0..bdd8f44 100644
>>> --- a/lib/librte_eal/common/eal_common_memory.c
>>> +++ b/lib/librte_eal/common/eal_common_memory.c
>>> @@ -383,6 +383,62 @@ struct virtiova {
>>>        rte_memseg_walk(dump_memseg, f);
>>>    }
>>>
>>> +static int
>>> +check_iova(const struct rte_memseg_list *msl __rte_unused,
>>> +             const struct rte_memseg *ms, void *arg)
>>> +{
>>> +     uint64_t *mask = arg;
>>> +     rte_iova_t iova;
>>> +
>>> +     /* higher address within segment */
>>> +     iova = (ms->iova + ms->len) - 1;
>>> +     if (!(iova & *mask))
>>> +             return 0;
>>> +
>>> +     RTE_LOG(INFO, EAL, "memseg iova %"PRIx64", len %zx, out of
>> range\n",
>>> +                        ms->iova, ms->len);
>>> +
>>> +     RTE_LOG(INFO, EAL, "\tusing dma mask %"PRIx64"\n", *mask);
>>
>> IMO putting these as INFO is overkill. I'd prefer not to spam the output
>> unless it's really important. Can this go under DEBUG?
>>
>>
> This checks comes from a device or from the alloc_pages_on_heap when
> expanding memory. If the check discovers an address out of mask, a device
> can not be used or the new memory can not be allocated. I think having this
> info will help to understand why the device initialization or the memory
> allocation are failing.
> 

If this text is only displayed whenever there's an error, the log output 
should be ERR, not INFO. If the error may or may not happen depending on 
who called this function, then this information is not important enough 
to display to the user (it should be displayed in the error handler of 
the caller), and DEBUG should suffice.

> 
>> Also, the message is misleading. You stop before you have a chance to
>> check other masks, which may restrict them even further. You're
>> outputting the message about using DMA mask XXX but this may not be the
>> final DMA mask.
>>
> 
> Well, this is the first triggering, and it is enough for reporting the
> problem and avoiding the device or the new memory to be used.
> 
> Note that the mask is per device, and for the memory allocation case, it is
> the most restrictive dma mask. So there are no other masks to try.

Fair enough.

> 
> 
> 
>>
>>> +     /* Stop the walk and change mask */
>>> +     *mask = 0;
>>> +     return 1;

No need for out-of-band communication, _walk() function will return 1 if 
walk was stopped prematurely. Just check return value of walk().

-- 
Thanks,
Anatoly