Re: [dpdk-dev] [PATCH 6/7] mem: add safe and unsafe versions for checking DMA mask

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

On Thu, Nov 1, 2018 at 10:38 AM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:

> On 31-Oct-18 5:29 PM, Alejandro Lucero wrote:
> > During memory initialization calling rte_mem_check_dma_mask
> > leads to a deadlock because memory_hotplug_lock is locked by a
> > writer, the current code in execution, and rte_memseg_walk
> > tries to lock as a reader.
> >
> > This patch adds safe and unsafe versions for invoking the final
> > function specifying if the memory_hotplug_lock needs to be
> > acquired, this is for the safe version, or not, the unsafe one.
> > PMDs should use the safe version and just internal EAL memory
> > code should use the unsafe one.
> >
> > Fixes: 223b7f1d5ef6 ("mem: add function for checking memseg IOVA")
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
>
> I don't think _safe and _unsafe are good names. _unsafe implies
> something might blow up, which isn't the case :) I think following the
> naming convention established by other functions (rte_mem_check_dma_mask
> and rte_mem_check_dma_mask_thread_unsafe) is better. User/driver code is
> only supposed to use rte_mem_check_dma_mask safe version anyway, so
> there's no need to differentiate between the two if the other one is
> never supposed to be used.
>
>
It makes sense. I will do it in next version.
Thanks


> >   drivers/net/nfp/nfp_net.c                  |  2 +-
> >   lib/librte_eal/common/eal_common_memory.c  | 24 +++++++++++++++---
> >   lib/librte_eal/common/include/rte_memory.h | 29 +++++++++++++++++++---
> >   lib/librte_eal/common/malloc_heap.c        |  2 +-
> >   lib/librte_eal/rte_eal_version.map         |  3 ++-
> >   5 files changed, 51 insertions(+), 9 deletions(-)
> >
>
> <...>
>
> > -/* check memsegs iovas are within a range based on dma mask */
> > -int __rte_experimental rte_mem_check_dma_mask(uint8_t maskbits);
> > +/**
> > + *  * @warning
>
> Here and in other places - same issue with extra star.
>
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + *  Check memsegs iovas are within a range based on dma mask.
>
> The comments make it seem like the parameter is an actual DMA mask,
> rather than DMA mask *width*. In fact, you seem to have tripped yourself
> up on that already :)
>
> Suggested rewording:
>
> Check if all currently allocated memory segments are compliant with
> supplied DMA address width.
>
>
Ok.


> > + *
> > + *  @param maskbits
> > + *    Address width to check against.
> > + */
> > +int __rte_experimental rte_mem_check_dma_mask_safe(uint8_t maskbits);
> > +
> > +/**
> > + *  * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + *  Check memsegs iovas are within a range based on dma mask without
> acquiring
> > + *  memory_hotplug_lock first.
> > + *
> > + *  This function is just for EAL core memory internal use. Drivers
> should
> > + *  use the previous safe one.
>
> This is IMO too detailed. Suggested rewording:
>
> Check if all currently allocated memory segments are compliant with
> supplied DMA address width.
>
>
Ok


> @warning This function is not thread-safe and is for internal use only.
>
> > + *
> > + *  @param maskbits
> > + *    Address width to check against.
> > + */
> > +int __rte_experimental rte_mem_check_dma_mask_unsafe(uint8_t maskbits);
> >
> >   /**
> >    *  * @warning
> >    * @b EXPERIMENTAL: this API may change without prior notice
> >    *
> >    *  Set dma mask to use once memory initialization is done.
> > - *  Previous function rte_mem_check_dma_mask can not be used
> > + *  Previous functions rte_mem_check_dma_mask_safe/unsafe can not be
> used
> >    *  safely until memory has been initialized.
> >    */
> >   void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits);
> > diff --git a/lib/librte_eal/common/malloc_heap.c
> b/lib/librte_eal/common/malloc_heap.c
> > index 711622f19..dd8b983e7 100644
> > --- a/lib/librte_eal/common/malloc_heap.c
> > +++ b/lib/librte_eal/common/malloc_heap.c
> > @@ -335,7 +335,7 @@ alloc_pages_on_heap(struct malloc_heap *heap,
> uint64_t pg_sz, size_t elt_size,
> >        * executed. For 2) implies the new memory can not be added.
> >        */
> >       if (mcfg->dma_maskbits) {
> > -             if (rte_mem_check_dma_mask(mcfg->dma_maskbits)) {
> > +             if (rte_mem_check_dma_mask_unsafe(mcfg->dma_maskbits)) {
> >                       /* Currently this can only happen if IOMMU is
> enabled
> >                        * with RTE_ARCH_X86. It is not safe to use this
> memory
> >                        * so returning an error here.
> > diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> > index ae24b5c73..f863903b6 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -296,7 +296,8 @@ EXPERIMENTAL {
> >       rte_devargs_remove;
> >       rte_devargs_type_count;
> >       rte_mem_check_dma_mask;
> > -     rte_mem_set_dma_mask;
> > +     rte_mem_set_dma_mask_safe;
> > +     rte_mem_set_dma_mask_unsafe;
>
> Again, alphabet :)
>
> >       rte_eal_cleanup;
> >       rte_fbarray_attach;
> >       rte_fbarray_destroy;
> >
>
>
> --
> Thanks,
> Anatoly
>