Re: [PATCH] vfio/type1: conditional rescheduling while unpinning

On Thu, 23 Jul 2026 08:20:00 -0700
Samuel Crossley <[email protected]> wrote:

> Tearing down a large device-passthrough DMA mapping can unpin tens to
> hundreds of millions of pages in a single VFIO_IOMMU_UNMAP_DMA.
> vfio_unpin_pages_remote() walks the whole contiguous range in one
> uninterrupted pass with no reschedule point:
> 
>   - has_rsvd (reserved / device-memory mappings, e.g. GPU HBM/BAR mapped
>     for peer DMA): a per-page put_pfn() loop, each doing a
>     pfn_valid()/is_invalid_reserved_pfn() section lookup;
>   - otherwise: the batched unpin_user_page_range_dirty_lock() folio walk.
> 
> The existing cond_resched() calls in the unmap path only run between
> regions/chunks, so they cannot break up one giant call.
> 
> Observed on a GPU-passthrough host: unmapping a 128 GiB device-memory
> region (~33.6M reserved 4K pages, has_rsvd) held a CPU 22s in the per-page
> loop and tripped the soft-lockup watchdog, panicking the host:
> 
>   watchdog: BUG: soft lockup - CPU#74 stuck for 22s! [qemu-system-x86]
>    put_pfn / is_invalid_reserved_pfn / pfn_valid
>    vfio_unpin_pages_remote / vfio_sync_unpin / vfio_unmap_unpin
>    vfio_remove_dma / vfio_iommu_type1_ioctl (VFIO_IOMMU_UNMAP_DMA)

This reconstructed backtrace is difficult to parse, seems to reverse
direction in the middle.

> 
> The v6.18 batching series (d10872050ffe, d14de5b92578) optimized only the
> non-reserved folio path, so it does not help this has_rsvd loop. Bound the
> work per iteration and cond_resched() between chunks, mirroring the
> pin-side fix in commit b1779e4f209c ("vfio/type1: conditional
> rescheduling while pinning").
> 
> cond_resched() is safe on this path: the only lock held across
> vfio_unpin_pages_remote() is iommu->lock, a mutex, so sleeping is allowed,
> and it is taken in vfio_dma_do_unmap() and held continuously through
> vfio_remove_dma() without being dropped on this path. No spinlock,
> preempt-disabled, IRQ-disabled or RCU read-side section is held anywhere in
> the type1 unmap path. The loop's callees (put_pfn(),
> unpin_user_page_range_dirty_lock()) drop any transient folio reference and
> folio lock before returning, so only the mutex is held at the reschedule
> point. The path is already demonstrably sleepable: the same unmap chain
> already calls cond_resched() at the region/chunk level, and the
> non-reserved dirty path already takes folio_lock().
> 
> Signed-off-by: Samuel Crossley <[email protected]>
> ---
> Signed-off-by: Sam Crossley <[email protected]>
> ---

Something is a bit off in your tooling here.

>  drivers/vfio/vfio_iommu_type1.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c8151ba54de3..ce7f6051f799 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -814,22 +814,42 @@ static inline void put_valid_unreserved_pfns(unsigned long start_pfn,
>  					 prot & IOMMU_WRITE);
>  }
>  
> +/* Pages to unpin per cond_resched() when tearing down a large mapping. */
> +#define VFIO_UNPIN_RESCHED_PAGES	(16UL * 1024)	/* 64MB @ 4K pages */

Nothing in the commit log or comment describes how this value is
derived.  This makes 2K chunks out of the 128GB BAR, why is this a good
value?

> +
>  static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
>  				    unsigned long pfn, unsigned long npage,
>  				    bool do_accounting)
>  {
>  	long unlocked = 0, locked = vpfn_pages(dma, iova, npage);
> +	unsigned long remaining = npage;
>  
> -	if (dma->has_rsvd) {
> -		unsigned long i;
> +	/*
> +	 * A single unmap of a very large device-passthrough mapping can unpin
> +	 * hundreds of millions of pages here.  Bound the work per iteration and
> +	 * cond_resched() so one VFIO_IOMMU_UNMAP_DMA cannot hold a CPU past the
> +	 * soft-lockup watchdog.  Mirrors the pin-side reschedule in commit
> +	 * edeca59cb88d2 ("vfio/type1: conditional rescheduling while pinning").

This commit ID doesn't exist, I think you're referring to b1779e4f209c.

> +	 */
> +	while (remaining) {
> +		unsigned long batch = min(remaining, VFIO_UNPIN_RESCHED_PAGES);
>  
> -		for (i = 0; i < npage; i++)
> -			if (put_pfn(pfn++, dma->prot))
> -				unlocked++;
> -	} else {
> -		put_valid_unreserved_pfns(pfn, npage, dma->prot);
> -		unlocked = npage;
> +		if (dma->has_rsvd) {
> +			unsigned long i;
> +
> +			for (i = 0; i < batch; i++)
> +				if (put_pfn(pfn++, dma->prot))
> +					unlocked++;
> +		} else {
> +			put_valid_unreserved_pfns(pfn, batch, dma->prot);
> +			unlocked += batch;
> +			pfn += batch;

The commit log describes the reserved pfn side as troublesome, but then
quietly applies chunking to both paths.  What evidence suggests it's
needed on this path too, and if it is, should it be in the mm layer
rather than here?

> +		}
> +
> +		remaining -= batch;
> +		cond_resched();
>  	}
> +
>  	if (do_accounting)
>  		vfio_lock_acct(dma, locked - unlocked, true);
>  
> 

I think there's an optimization we can do here that avoids the overhead
entirely rather than just splitting it into scheduler friendly chunks.

We track whether a vfio_dma has reserved pages, but we don't know if
it's only reserved pages or some mix of reserved and non-reserved
pages, so we iterate per page.  I think that mixed case requires some
atypical userspace behavior and this check is a no-op in the case where
there are only reserved pages.  So (untested), I think we could do
something like this:

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c8151ba54de3..f7addfbe1aab 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -94,6 +94,7 @@ struct vfio_dma {
        bool                    lock_cap;       /* capable(CAP_IPC_LOCK) */
        bool                    vaddr_invalid;
        bool                    has_rsvd;       /* has 1 or more rsvd pfns */
+       bool                    has_non_rsvd;   /* has 1 or more !rsvd pfns */
        struct task_struct      *task;
        struct rb_root          pfn_list;       /* Ex-user pinned pfn list */
        unsigned long           *bitmap;
@@ -791,6 +792,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
 
 out:
        dma->has_rsvd |= rsvd;
+       dma->has_non_rsvd |= !rsvd;
        ret = vfio_lock_acct(dma, lock_acct, false);
 
 unpin_out:
@@ -821,11 +823,13 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
        long unlocked = 0, locked = vpfn_pages(dma, iova, npage);
 
        if (dma->has_rsvd) {
-               unsigned long i;
+               if (dma->has_non_rsvd) {
+                       unsigned long i;
 
-               for (i = 0; i < npage; i++)
-                       if (put_pfn(pfn++, dma->prot))
-                               unlocked++;
+                       for (i = 0; i < npage; i++)
+                               if (put_pfn(pfn++, dma->prot))
+                                       unlocked++;
+               }
        } else {
                put_valid_unreserved_pfns(pfn, npage, dma->prot);
                unlocked = npage;


Possibly that inner loop could be split to have a cond_resched() at
regular intervals as hardening against softlockup_panic configurations,
but aiui, typical userspace wouldn't hit it.  Thanks,

Alex