[PATCH v11 09/12] vduse: take out allocations from vduse_dev_alloc_coherent

Eugenio Pérez posted 12 patches 4 weeks, 1 day ago
There is a newer version of this series
[PATCH v11 09/12] vduse: take out allocations from vduse_dev_alloc_coherent
Posted by Eugenio Pérez 4 weeks, 1 day ago
The function vduse_dev_alloc_coherent will be called under rwlock in
next patches.  Make it out of the lock to avoid increasing its fail
rate.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
v11: Remove duplicated call to free_pages_exact (Jason).
---
 drivers/vdpa/vdpa_user/iova_domain.c | 10 ++--------
 drivers/vdpa/vdpa_user/iova_domain.h |  2 +-
 drivers/vdpa/vdpa_user/vduse_dev.c   | 13 +++++++++++--
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
index 309cd5a039d1..3955690696fe 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.c
+++ b/drivers/vdpa/vdpa_user/iova_domain.c
@@ -495,14 +495,13 @@ void vduse_domain_unmap_page(struct vduse_iova_domain *domain,
 
 void *vduse_domain_alloc_coherent(struct vduse_iova_domain *domain,
 				  size_t size, dma_addr_t *dma_addr,
-				  gfp_t flag)
+				  void *orig)
 {
 	struct iova_domain *iovad = &domain->consistent_iovad;
 	unsigned long limit = domain->iova_limit;
 	dma_addr_t iova = vduse_domain_alloc_iova(iovad, size, limit);
-	void *orig = alloc_pages_exact(size, flag);
 
-	if (!iova || !orig)
+	if (!iova)
 		goto err;
 
 	spin_lock(&domain->iotlb_lock);
@@ -519,8 +518,6 @@ void *vduse_domain_alloc_coherent(struct vduse_iova_domain *domain,
 	return orig;
 err:
 	*dma_addr = DMA_MAPPING_ERROR;
-	if (orig)
-		free_pages_exact(orig, size);
 	if (iova)
 		vduse_domain_free_iova(iovad, iova, size);
 
@@ -533,7 +530,6 @@ void vduse_domain_free_coherent(struct vduse_iova_domain *domain, size_t size,
 	struct iova_domain *iovad = &domain->consistent_iovad;
 	struct vhost_iotlb_map *map;
 	struct vdpa_map_file *map_file;
-	phys_addr_t pa;
 
 	spin_lock(&domain->iotlb_lock);
 	map = vhost_iotlb_itree_first(domain->iotlb, (u64)dma_addr,
@@ -545,12 +541,10 @@ void vduse_domain_free_coherent(struct vduse_iova_domain *domain, size_t size,
 	map_file = (struct vdpa_map_file *)map->opaque;
 	fput(map_file->file);
 	kfree(map_file);
-	pa = map->addr;
 	vhost_iotlb_map_free(domain->iotlb, map);
 	spin_unlock(&domain->iotlb_lock);
 
 	vduse_domain_free_iova(iovad, dma_addr, size);
-	free_pages_exact(phys_to_virt(pa), size);
 }
 
 static vm_fault_t vduse_domain_mmap_fault(struct vm_fault *vmf)
diff --git a/drivers/vdpa/vdpa_user/iova_domain.h b/drivers/vdpa/vdpa_user/iova_domain.h
index 081f06c52cdc..1854fdc25597 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.h
+++ b/drivers/vdpa/vdpa_user/iova_domain.h
@@ -67,7 +67,7 @@ void vduse_domain_unmap_page(struct vduse_iova_domain *domain,
 
 void *vduse_domain_alloc_coherent(struct vduse_iova_domain *domain,
 				  size_t size, dma_addr_t *dma_addr,
-				  gfp_t flag);
+				  void *orig);
 
 void vduse_domain_free_coherent(struct vduse_iova_domain *domain, size_t size,
 				dma_addr_t dma_addr, unsigned long attrs);
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 82ee476d45e0..675da1465e0e 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -923,16 +923,24 @@ static void *vduse_dev_alloc_coherent(union virtio_map token, size_t size,
 	if (!token.group)
 		return NULL;
 
+	addr = alloc_pages_exact(size, flag);
+	if (!addr)
+		return NULL;
+
 	vdev = token.group->dev;
 	domain = vdev->domain;
 	addr = vduse_domain_alloc_coherent(domain, size,
-					   (dma_addr_t *)&iova, flag);
+					   (dma_addr_t *)&iova, addr);
 	if (!addr)
-		return NULL;
+		goto err;
 
 	*dma_addr = (dma_addr_t)iova;
 
 	return addr;
+
+err:
+	free_pages_exact(addr, size);
+	return NULL;
 }
 
 static void vduse_dev_free_coherent(union virtio_map token, size_t size,
@@ -949,6 +957,7 @@ static void vduse_dev_free_coherent(union virtio_map token, size_t size,
 	domain = vdev->domain;
 
 	vduse_domain_free_coherent(domain, size, dma_addr, attrs);
+	free_pages_exact(vaddr, size);
 }
 
 static bool vduse_dev_need_sync(union virtio_map token, dma_addr_t dma_addr)
-- 
2.52.0

Re: [PATCH v11 09/12] vduse: take out allocations from vduse_dev_alloc_coherent
Posted by Michael S. Tsirkin 4 weeks ago
On Fri, Jan 09, 2026 at 04:24:27PM +0100, Eugenio Pérez wrote:
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 82ee476d45e0..675da1465e0e 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -923,16 +923,24 @@ static void *vduse_dev_alloc_coherent(union virtio_map token, size_t size,
>  	if (!token.group)
>  		return NULL;
>  
> +	addr = alloc_pages_exact(size, flag);
> +	if (!addr)
> +		return NULL;
> +

So addr has allocated pages here ...


>  	vdev = token.group->dev;
>  	domain = vdev->domain;
>  	addr = vduse_domain_alloc_coherent(domain, size,
> -					   (dma_addr_t *)&iova, flag);
> +					   (dma_addr_t *)&iova, addr);

and then is overwritten here ...

>  	if (!addr)
> -		return NULL;
> +		goto err;

except on error where we go to err ...

>  
>  	*dma_addr = (dma_addr_t)iova;
>  
>  	return addr;
> +
> +err:
> +	free_pages_exact(addr, size);

only to try and free NULL. will leak the original pages, will it not.

> +	return NULL;
>  }
>  
>  static void vduse_dev_free_coherent(union virtio_map token, size_t size,
Re: [PATCH v11 09/12] vduse: take out allocations from vduse_dev_alloc_coherent
Posted by Eugenio Perez Martin 3 weeks, 5 days ago
On Sun, Jan 11, 2026 at 12:54 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jan 09, 2026 at 04:24:27PM +0100, Eugenio Pérez wrote:
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 82ee476d45e0..675da1465e0e 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -923,16 +923,24 @@ static void *vduse_dev_alloc_coherent(union virtio_map token, size_t size,
> >       if (!token.group)
> >               return NULL;
> >
> > +     addr = alloc_pages_exact(size, flag);
> > +     if (!addr)
> > +             return NULL;
> > +
>
> So addr has allocated pages here ...
>
>
> >       vdev = token.group->dev;
> >       domain = vdev->domain;
> >       addr = vduse_domain_alloc_coherent(domain, size,
> > -                                        (dma_addr_t *)&iova, flag);
> > +                                        (dma_addr_t *)&iova, addr);
>
> and then is overwritten here ...
>
> >       if (!addr)
> > -             return NULL;
> > +             goto err;
>
> except on error where we go to err ...
>
> >
> >       *dma_addr = (dma_addr_t)iova;
> >
> >       return addr;
> > +
> > +err:
> > +     free_pages_exact(addr, size);
>
> only to try and free NULL. will leak the original pages, will it not.
>

Right, I missed that in the conversion. I'll change the function
vduse_domain_alloc_coherent so it returns dma_addr instead of orig,
making all the code simpler. Thanks!

> > +     return NULL;
> >  }
> >
> >  static void vduse_dev_free_coherent(union virtio_map token, size_t size,
>