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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jan 19, 2026 at 02:00:49PM +0100, Jan Beulich wrote:
> On 15.01.2026 12:18, Roger Pau Monne wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -722,6 +722,15 @@ static void _domain_destroy(struct domain *d)
> >  
> >      XVFREE(d->console);
> >  
> > +    if ( d->pending_scrub )
> > +    {
> > +        BUG_ON(d->creation_finished);
> > +        free_domheap_pages(d->pending_scrub, d->pending_scrub_order);
> > +        d->pending_scrub = NULL;
> > +        d->pending_scrub_order = 0;
> > +        d->pending_scrub_index = 0;
> > +    }
> 
> Because of the other zeroing wanted (it's not strictly needed, is it?),
> it may be a little awkward to use FREE_DOMHEAP_PAGES() here. Yet I would
> still have recommended to avoid its open-coding, if only we had such a
> wrapper already.

I don't mind introducing a FREE_DOMHEAP_PAGES() wrapper in this same
patch, if you are OK with it.

> Would this better be done earlier, in domain_kill(), to avoid needlessly
> holding back memory that isn't going to be used by this domain anymore?
> Would require the spinlock be acquired to guard against a racing
> stash_allocation(), I suppose. In fact freeing right in
> domain_unpause_by_systemcontroller() might be yet better (albeit without
> eliminating the need to do it here or in domain_kill()).

Even with a lock taken moving to domain_kill() would be racy.  A rogue
toolstack could keep trying to issue populate_physmap hypercalls which
would fail in the assign_pages() call, but it could still leave
pending pages in d->pending_scrub, as the assign_pages() call happens
strictly after the scrubbing is done.

> > @@ -1678,6 +1687,14 @@ int domain_unpause_by_systemcontroller(struct domain *d)
> >       */
> >      if ( new == 0 && !d->creation_finished )
> >      {
> > +        if ( d->pending_scrub )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "%pd: cannot be started with pending dirty pages, destroying\n",
> 
> s/dirty/unscrubbed/ to avoid ambiguity with "dirty" as in "needing writeback"?
> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -159,6 +159,74 @@ static void increase_reservation(struct memop_args *a)
> >      a->nr_done = i;
> >  }
> >  
> > +/*
> > + * Temporary storage for a domain assigned page that's not been fully scrubbed.
> > + * Stored pages must be domheap ones.
> > + *
> > + * The stashed page can be freed at any time by Xen, the caller must pass the
> > + * order and NUMA node requirement to the fetch function to ensure the
> > + * currently stashed page matches it's requirements.
> > + */
> > +static void stash_allocation(struct domain *d, struct page_info *page,
> > +                             unsigned int order, unsigned int scrub_index)
> > +{
> > +    BUG_ON(d->creation_finished);
> 
> Is this valid here and ...
> 
> > +    rspin_lock(&d->page_alloc_lock);
> > +
> > +    /*
> > +     * Drop any stashed allocation to accommodated the current one.  This
> > +     * interface is designed to be used for single-threaded domain creation.
> > +     */
> > +    if ( d->pending_scrub )
> > +        free_domheap_pages(d->pending_scrub, d->pending_scrub_order);
> > +
> > +    d->pending_scrub_index = scrub_index;
> > +    d->pending_scrub_order = order;
> > +    d->pending_scrub = page;
> > +
> > +    rspin_unlock(&d->page_alloc_lock);
> > +}
> > +
> > +static struct page_info *get_stashed_allocation(struct domain *d,
> > +                                                unsigned int order,
> > +                                                nodeid_t node,
> > +                                                unsigned int *scrub_index)
> > +{
> > +    struct page_info *page = NULL;
> > +
> > +    BUG_ON(d->creation_finished && d->pending_scrub);
> 
> ... here? A badly behaved toolstack could do a populate in parallel with
> the initial unpause, couldn't it?

Oh, I think I've forgot to refresh before sending the patch . I have
those as ASSERTs in my local copy anyway.  But yes, you are right,
populate_physmap() is not done while holding the domctl lock, so it
can race with an unpause.  I will remove the ASSERTs.

> > +    rspin_lock(&d->page_alloc_lock);
> > +
> > +    /*
> > +     * If there's a pending page to scrub check it satisfies the current
> > +     * request.  If it doesn't keep it stashed and return NULL.
> > +     */
> > +    if ( !d->pending_scrub || d->pending_scrub_order != order ||
> > +         (node != NUMA_NO_NODE && node != page_to_nid(d->pending_scrub)) )
> 
> Ah, and MEMF_exact_node is handled in the caller.
> 
> > +        goto done;
> > +    else
> > +    {
> > +        page = d->pending_scrub;
> > +        *scrub_index = d->pending_scrub_index;
> > +    }
> > +
> > +    /*
> > +     * The caller now owns the page, clear stashed information.  Prevent
> > +     * concurrent usages of get_stashed_allocation() from returning the same
> > +     * page to different contexts.
> > +     */
> > +    d->pending_scrub_index = 0;
> > +    d->pending_scrub_order = 0;
> > +    d->pending_scrub = NULL;
> > +
> > + done:
> > +    rspin_unlock(&d->page_alloc_lock);
> > +
> > +    return page;
> > +}
> 
> Hmm, you free the earlier allocation only in stash_allocation(), i.e. that
> memory isn't available to fulfill the present request. (I do understand
> that the freeing there can't be dropped, to deal with possible races
> caused by the toolstack.)

Since we expect populate_physmap(9 to be executed sequentially by the
toolstack I would argue it's fine to hold onto that memory.  Otherwise
I could possibly free in get_stashed_allocation() when the request
doesn't match what's stashed.  I opted for freeing later in
stash_allocation() to maybe give time for the other parallel caller to
finish the scrubbing.

> The use of "goto" here also looks a little odd, as it would be easy to get
> away without. Or else I'd like to ask that the "else" be dropped.

Hm, OK, let me use an unlock + return and also drop the else then.  I
think that's clearer.

> > @@ -286,6 +365,30 @@ static void populate_physmap(struct memop_args *a)
> >                      goto out;
> >                  }
> >  
> > +                if ( !d->creation_finished )
> > +                {
> > +                    unsigned int dirty_cnt = 0, j;
> 
> Declaring (another) j here is going to upset Eclair, I fear, as ...
> 
> > +                    /* Check if there's anything to scrub. */
> > +                    for ( j = scrub_start; j < (1U << a->extent_order); j++ )
> > +                    {
> > +                        if ( !test_and_clear_bit(_PGC_need_scrub,
> > +                                                 &page[j].count_info) )
> > +                            continue;
> > +
> > +                        scrub_one_page(&page[j], true);
> > +
> > +                        if ( (j + 1) != (1U << a->extent_order) &&
> > +                             !(++dirty_cnt & 0xff) &&
> > +                             hypercall_preempt_check() )
> > +                        {
> > +                            a->preempted = 1;
> > +                            stash_allocation(d, page, a->extent_order, ++j);
> 
> Better j + 1, as j's value isn't supposed to be used any further?
> 
> > +                            goto out;
> > +                        }
> > +                    }
> > +                }
> > +
> >                  if ( unlikely(a->memflags & MEMF_no_tlbflush) )
> >                  {
> >                      for ( j = 0; j < (1U << a->extent_order); j++ )
> 
> ... for this to work there must already be one available from an outer scope.

Indeed, I've removed that outrageous j variable.

Thanks, Roger.