[v10] Add multiple address spaces support to VDUSE

[PATCH v10 6/8] vduse: take out allocations from vduse_dev_alloc_coherent

Posted by Eugenio Pérez 1 month, 3 weeks 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>
---
 drivers/vdpa/vdpa_user/iova_domain.c |  7 ++-----
 drivers/vdpa/vdpa_user/iova_domain.h |  2 +-
 drivers/vdpa/vdpa_user/vduse_dev.c   | 13 +++++++++++--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
index 309cd5a039d1..0ae52890518c 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);
 
diff --git a/drivers/vdpa/vdpa_user/iova_domain.h b/drivers/vdpa/vdpa_user/iova_domain.h
index 42090cd1a622..86b7c7eadfd0 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.h
+++ b/drivers/vdpa/vdpa_user/iova_domain.h
@@ -69,7 +69,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 a278cff7a4fa..767abcb7e375 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -924,16 +924,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,
@@ -950,6 +958,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 v10 6/8] vduse: take out allocations from vduse_dev_alloc_coherent

Posted by Jason Wang 1 month, 3 weeks ago

On Wed, Dec 17, 2025 at 7:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> 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>
> ---
>  drivers/vdpa/vdpa_user/iova_domain.c |  7 ++-----
>  drivers/vdpa/vdpa_user/iova_domain.h |  2 +-
>  drivers/vdpa/vdpa_user/vduse_dev.c   | 13 +++++++++++--
>  3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
> index 309cd5a039d1..0ae52890518c 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);
>
> diff --git a/drivers/vdpa/vdpa_user/iova_domain.h b/drivers/vdpa/vdpa_user/iova_domain.h
> index 42090cd1a622..86b7c7eadfd0 100644
> --- a/drivers/vdpa/vdpa_user/iova_domain.h
> +++ b/drivers/vdpa/vdpa_user/iova_domain.h
> @@ -69,7 +69,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 a278cff7a4fa..767abcb7e375 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -924,16 +924,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,
> @@ -950,6 +958,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);

There looks like a double-free as there's another free_page_exact() in
vduse_domain_free_coherent.

Thanks

>  }
>
>  static bool vduse_dev_need_sync(union virtio_map token, dma_addr_t dma_addr)
> --
> 2.52.0
>

Re: [PATCH v10 6/8] vduse: take out allocations from vduse_dev_alloc_coherent

Posted by Eugenio Perez Martin 1 month, 3 weeks ago

On Thu, Dec 18, 2025 at 6:45 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Dec 17, 2025 at 7:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > 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>
> > ---
> >  drivers/vdpa/vdpa_user/iova_domain.c |  7 ++-----
> >  drivers/vdpa/vdpa_user/iova_domain.h |  2 +-
> >  drivers/vdpa/vdpa_user/vduse_dev.c   | 13 +++++++++++--
> >  3 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
> > index 309cd5a039d1..0ae52890518c 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);
> >
> > diff --git a/drivers/vdpa/vdpa_user/iova_domain.h b/drivers/vdpa/vdpa_user/iova_domain.h
> > index 42090cd1a622..86b7c7eadfd0 100644
> > --- a/drivers/vdpa/vdpa_user/iova_domain.h
> > +++ b/drivers/vdpa/vdpa_user/iova_domain.h
> > @@ -69,7 +69,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 a278cff7a4fa..767abcb7e375 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -924,16 +924,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,
> > @@ -950,6 +958,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);
>
> There looks like a double-free as there's another free_page_exact() in
> vduse_domain_free_coherent.
>

You're right, removing the duplicated one. Thanks!

[PATCH v10 1/8] vduse: add v1 API definition
[PATCH v10 2/8] vduse: add vq group support
[PATCH v10 3/8] vduse: return internal vq group struct as map token
[PATCH v10 4/8] vduse: refactor vdpa_dev_add for goto err handling
[PATCH v10 5/8] vduse: remove unused vaddr parameter of vduse_domain_free_coherent
[PATCH v10 6/8] vduse: take out allocations from vduse_dev_alloc_coherent
[PATCH v10 7/8] vduse: add vq group asid support
[PATCH v10 8/8] vduse: bump version number