Re: [PATCH v2 3/3] xen/mm: improve freeing of partially scrubbed pages

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 26, 2026 at 12:50:27PM +0100, Jan Beulich wrote:
> On 26.03.2026 09:51, Roger Pau Monne wrote:
> > When freeing possibly partially scrubbed pages in populate_physmap() the
> > whole page is marked as dirty, but that's not fully accurate.  Since the
> > PGC_need_scrub bit is preserved for the populate_physmap() allocation we
> > can use those when freeing to detect which pages need scrubbing instead of
> > marking the whole page as dirty.
> > 
> > This requires exposing free_heap_pages() globally, and switching
> > populate_physmap() to use it instead of free_domheap_pages().
> > 
> > Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Jan: I'm not sure if that's what you suggested in the review of v1.  I've
> > added your Suggested-by but I can drop it if that's not what you were
> > thinking of.
> 
> You're going quite a bit farther. In my comment I really only meant the one
> new use you add in patch 2 (in which case no changes to the body of
> free_heap_pages() would have been needed, and hence why I thought that it
> could maybe be done right there). Up to you whether to keep the tag.

I see, you meant to change the single usage in case assign_page()
fails.  I think going a bit further is fine, seeing the adjustment to
free_heap_pages() is very minimal?

> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -177,7 +177,7 @@ static void stash_allocation(struct domain *d, struct 
> > page_info *page,
> >       * interface is designed to be used for single-threaded domain 
> > creation.
> >       */
> >      if ( d->pending_scrub || d->is_dying )
> > -        free_domheap_pages(page, order);
> > +        free_heap_pages(page, order, false);
> >      else
> >      {
> >          d->pending_scrub_index = scrub_index;
> > @@ -210,7 +210,7 @@ static struct page_info *get_stashed_allocation(struct 
> > domain *d,
> >              *scrub_index = d->pending_scrub_index;
> >          }
> >          else
> > -            free_domheap_pages(d->pending_scrub, d->pending_scrub_order);
> > +            free_heap_pages(d->pending_scrub, d->pending_scrub_order, 
> > false);
> >  
> >          /*
> >           * The caller now owns the page or it has been freed, clear stashed
> > @@ -391,7 +391,7 @@ static void populate_physmap(struct memop_args *a)
> >  
> >                      if ( assign_page(page, a->extent_order, d, memflags) )
> >                      {
> > -                        free_domheap_pages(page, a->extent_order);
> > +                        free_heap_pages(page, a->extent_order, false);
> >                          goto out;
> >                      }
> >                  }
> 
> Along with all of these there's then also domain_pending_scrub_free().

Yes, indeed.

> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -153,6 +153,12 @@ unsigned long avail_node_heap_pages(unsigned int 
> > nodeid);
> >  } while ( false )
> >  #define FREE_DOMHEAP_PAGE(p) FREE_DOMHEAP_PAGES(p, 0)
> >  
> > +/*
> > + * Most callers should use free_{xen,dom}heap_pages() instead of directly
> > + * calling free_heap_pages().
> > + */
> > +void free_heap_pages(struct page_info *pg, unsigned int order, bool 
> > need_scrub);
> 
> Might we better not put this here, but instead in a private header in common/?

No strong opinion.  It could logically be used outside of common in
principle, hence we might end up moving it anyway.  Would you prefer
me to introduce a common/memory.h header with just this prototype?

Thanks, Roger.