On Thu, Nov 1, 2018 at 10:38 AM Burakov, Anatoly <anatoly.bura...@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.luc...@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
>
