Re: [RFC PATCH v2 1/7] IB/mlx5: Change ib_umem_odp_map_dma_single_page() to retain umem_mutex

[lizhijian@xxxxxxxxxxx]

On 11/11/2022 17:22, Daisuke Matsuda wrote:
> ib_umem_odp_map_dma_single_page(), which has been used only by the mlx5
> driver, holds umem_mutex on success and releases on failure. This
> behavior is not convenient for other drivers to use it, so change it to
> always retain mutex on return.
> 
> Signed-off-by: Daisuke Matsuda <matsuda-daisuke@xxxxxxxxxxx>
> ---
>   drivers/infiniband/core/umem_odp.c | 8 +++-----
>   drivers/infiniband/hw/mlx5/odp.c   | 4 +++-
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index e9fa22d31c23..49da6735f7c8 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -328,8 +328,8 @@ static int ib_umem_odp_map_dma_single_page(
>    *
>    * Maps the range passed in the argument to DMA addresses.
>    * The DMA addresses of the mapped pages is updated in umem_odp->dma_list.
> - * Upon success the ODP MR will be locked to let caller complete its device
> - * page table update.
> + * The umem mutex is locked in this function. Callers are responsible for
> + * releasing the lock.
>    *


>    * Returns the number of pages mapped in success, negative error code
>    * for failure.
> @@ -453,11 +453,9 @@ int ib_umem_odp_map_dma_and_lock(struct ib_umem_odp *umem_odp, u64 user_virt,
>   			break;
>   		}
>   	}
> -	/* upon success lock should stay on hold for the callee */
> +
>   	if (!ret)
>   		ret = dma_index - start_idx;
> -	else
> -		mutex_unlock(&umem_odp->umem_mutex);
>   
>   out_put_mm:
>   	mmput_async(owning_mm);
> diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> index bc97958818bb..a0de27651586 100644
> --- a/drivers/infiniband/hw/mlx5/odp.c
> +++ b/drivers/infiniband/hw/mlx5/odp.c
> @@ -572,8 +572,10 @@ static int pagefault_real_mr(struct mlx5_ib_mr *mr, struct ib_umem_odp *odp,
>   		access_mask |= ODP_WRITE_ALLOWED_BIT;
>   
>   	np = ib_umem_odp_map_dma_and_lock(odp, user_va, bcnt, access_mask, fault);
> -	if (np < 0)
> +	if (np < 0) {
> +		mutex_unlock(&odp->umem_mutex);
>   		return np;
> +	}

refer to the comments of ib_umem_odp_map_dma_and_lock:
334  * Returns the number of pages mapped in success, negative error 
code
335  * for failure.

I don't think it's correct to release the lock in all failure case, for 
example when it reaches below error path.

346 int ib_umem_odp_map_dma_and_lock(struct ib_umem_odp *umem_odp, u64 
user_virt,
347                                  u64 bcnt, u64 access_mask, bool 
fault)
348                         __acquires(&umem_odp->umem_mutex) 

349 { 

350         struct task_struct *owning_process  = NULL; 

351         struct mm_struct *owning_mm = umem_odp->umem.owning_mm; 

352         int pfn_index, dma_index, ret = 0, start_idx; 

353         unsigned int page_shift, hmm_order, pfn_start_idx; 

354         unsigned long num_pfns, current_seq; 

355         struct hmm_range range = {}; 

356         unsigned long timeout; 

357 

358         if (access_mask == 0) 

359                 return -EINVAL;   <<<<<   no lock is hold yet 

360 

361         if (user_virt < ib_umem_start(umem_odp) || 

362             user_virt + bcnt > ib_umem_end(umem_odp)) 

363                 return -EFAULT;   <<<<<   no lock is hold yet


Further more, you changed public API's the behavior, do it matter for 
other out-of-tree drivers which is using it, i'm not familiar with this, 
maybe kernel has no restriction on it ?


>   
>   	/*
>   	 * No need to check whether the MTTs really belong to this MR, since