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

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

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.

>   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.

> + *
> + *  @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.

@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