Re: [PATCH v4 2/3] xen/mm: allow deferred scrub of physmap populate allocated pages

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/01/2026 12:03 pm, Roger Pau Monne wrote:
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 376351b528c9..123202f2c025 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -710,6 +710,23 @@ static int domain_teardown(struct domain *d)
>      return 0;
>  }
>  
> +/*
> + * Called multiple times during domain destruction, to attempt to early free
> + * any stashed pages to be scrubbed.  The call from _domain_destroy() is done
> + * when the toolstack can no longer stash any pages.
> + */
> +static void domain_free_pending_scrub(struct domain *d)
> +{
> +    rspin_lock(&d->page_alloc_lock);
> +    if ( d->pending_scrub )
> +    {
> +        FREE_DOMHEAP_PAGES(d->pending_scrub, d->pending_scrub_order);
> +        d->pending_scrub_order = 0;
> +        d->pending_scrub_index = 0;
> +    }
> +    rspin_unlock(&d->page_alloc_lock);
> +}
> +
>  /*
>   * Destroy a domain once all references to it have been dropped.  Used either
>   * from the RCU path, or from the domain_create() error path before the 
> domain
> @@ -722,6 +739,8 @@ static void _domain_destroy(struct domain *d)
>  
>      XVFREE(d->console);
>  
> +    domain_free_pending_scrub(d);
> +
>      argo_destroy(d);
>  
>      rangeset_domain_destroy(d);
> @@ -1286,6 +1305,8 @@ int domain_kill(struct domain *d)
>          rspin_barrier(&d->domain_lock);
>          argo_destroy(d);
>          vnuma_destroy(d->vnuma);
> +        domain_free_pending_scrub(d);
> +        rspin_unlock(&d->page_alloc_lock);

This is a double unlock, isn't it?


The freeing wants to be in domain_kill() (ish), or _domain_destroy() but
not both.

In this case we can't have anything using pending scrubbing until the
domain is the domlist (i.e. can be the target of other hypercalls), so
this wants to be in domain_relinquish_resources()  (rather than
domain_kill() which we're trying to empty).

In principle we could assert that it's already NULL in _domain_destroy()
which might help catch an error if it gets set early but the domain
destroyed before getting into the domlist, but that seems like a rather
slim case.

~Andrew