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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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().

> --- 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/?

Jan