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

[Alejandro Lucero <alejandro.lucero@netronome.com>]

On Thu, Oct 4, 2018 at 4:39 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> 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.
>
>
Ok. Makes sense. I will change it.
Thanks


> >
> >> 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().
>
>
Yes, that's right. I will use the return from the walk function instead.
Thanks



> --
> Thanks,
> Anatoly
>