Re: [PATCH for-next v3] RDMA/core: Avoid hmm_dma_map_alloc() for virtual DMA devices

[Daisuke Matsuda <dskmtsd@gmail.com>]

Hi Leon,

Thank you for amending the patch.
I've run the test and confirmed that the bug has been resolved.

We still have two build errors. Please see my reply below.

On 2025/05/25 16:08, Leon Romanovsky wrote:
> On Sat, May 24, 2025 at 02:43:28PM +0000, Daisuke Matsuda wrote:
<...>
>>   drivers/infiniband/core/device.c   | 17 +++++++++++++++++
>>   drivers/infiniband/core/umem_odp.c | 11 ++++++++---
>>   include/rdma/ib_verbs.h            |  4 ++++
>>   3 files changed, 29 insertions(+), 3 deletions(-)
> 
> Please include changelogs when you submit vX patches next time.

Oh, sorry. I will be careful from next time.

> 
> I ended with the following patch:
> 
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index 51d518989914e..cf16549919e02 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -60,9 +60,11 @@ static int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>   {
>   	struct ib_device *dev = umem_odp->umem.ibdev;
>   	size_t page_size = 1UL << umem_odp->page_shift;
> +	struct hmm_dma_map *map;
>   	unsigned long start;
>   	unsigned long end;
> -	int ret;
> +	size_t nr_entries;
> +	int ret = 0;
>   
>   	umem_odp->umem.is_odp = 1;
>   	mutex_init(&umem_odp->umem_mutex);
> @@ -75,9 +77,20 @@ static int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>   	if (unlikely(end < page_size))
>   		return -EOVERFLOW;
>   
> -	ret = hmm_dma_map_alloc(dev->dma_device, &umem_odp->map,
> -				(end - start) >> PAGE_SHIFT,
> -				1 << umem_odp->page_shift);
> +	nr_entries = (end - start) >> PAGE_SHIFT;
> +	if (!(nr_entries * PAGE_SIZE / page_size))
> +		return -EINVAL;
> +
> +	nap = &umem_odp->map;

BUILD ERROR: 'nap' should be 'map'

> +	if (ib_uses_virt_dma(dev)) {
> +		map->pfn_list = kvcalloc(nr_entries, sizeof(*map->pfn_list),
> +					 GFP_KERNEL | __GFP_NOWARN);
> +		if (!map->pfn_list)
> +			ret = -ENOMEM;
> +	} else
> +		ret = hmm_dma_map_alloc(dev->dma_device, &umem_odp->map,

OPTIONAL: Perhaps we can just pass 'map' for the 2nd arg?

> +					(end - start) >> PAGE_SHIFT,
> +					1 << umem_odp->page_shift);
>   	if (ret)
>   		return ret;
>   
> @@ -90,7 +103,10 @@ static int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>   	return 0;
>   
>   out_free_map:
> -	hmm_dma_map_free(dev->dma_device, &umem_odp->map);
> +	if (ib_uses_virt_dma(dev))
> +		kfree(map->pfn_list);
> +	else
> +		hmm_dma_map_free(dev->dma_device, &umem_odp->map);

OPTIONAL: Here too.

>   	return ret;
>   }
>   
> @@ -259,7 +275,10 @@ static void ib_umem_odp_free(struct ib_umem_odp *umem_odp)
>   				    ib_umem_end(umem_odp));
>   	mutex_unlock(&umem_odp->umem_mutex);
>   	mmu_interval_notifier_remove(&umem_odp->notifier);
> -	hmm_dma_map_free(dev->dma_device, &umem_odp->map);
> +	if (ib_uses_virt_dma(dev))
> +		kfree(umem_odp->map->pfn_list);

BUILD ERROR:     'umem_odp->map.pfn_list' is correct.

Thanks again,
Daisuke