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

[Leon Romanovsky <leon@kernel.org>]

On Sun, May 25, 2025 at 06:34:56PM +0900, Daisuke Matsuda wrote:
> 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'

Sorry, I pushed the code too fast.

> 
> > +	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?


<...>

> > +		hmm_dma_map_free(dev->dma_device, &umem_odp->map);
> 
> OPTIONAL: Here too.

Sure, will change.

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

Sorry about that too.

> 
> Thanks again,
> Daisuke
> 
>