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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2026 17:01, Roger Pau Monne wrote:
> When freeing possibly partially scrubbed pages in populate_physmap() and
> domain_pending_scrub_free() 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() and domain_pending_scrub_free() to use it instead of
> free_domheap_pages().
> 
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

This is okay as is, i.e.:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

However, a few remarks below.

> ---
>  xen/common/domain.c     |  4 +++-
>  xen/common/memory.c     |  8 +++++---
>  xen/common/page_alloc.c | 16 +++++++++++++---
>  xen/common/page_alloc.h | 14 ++++++++++++++
>  4 files changed, 35 insertions(+), 7 deletions(-)
>  create mode 100644 xen/common/page_alloc.h

I'm on the edge of requesting page-alloc.h as the name here. I can see though
how the name you picked better fits page_alloc.c.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1529,13 +1529,13 @@ static bool mark_page_free(struct page_info *pg, 
> mfn_t mfn)
>  static void free_color_heap_page(struct page_info *pg, bool need_scrub);
>  
>  /* Free 2^@order set of pages. */
> -static void free_heap_pages(
> -    struct page_info *pg, unsigned int order, bool need_scrub)
> +void free_heap_pages(struct page_info *pg, unsigned int order, bool 
> need_scrub)
>  {
>      unsigned long mask;
>      mfn_t mfn = page_to_mfn(pg);
>      unsigned int i, node = mfn_to_nid(mfn);
>      unsigned int zone = page_to_zone(pg);
> +    unsigned int first_dirty = INVALID_DIRTY_IDX, dirty_cnt = 0;
>      bool pg_offlined = false;
>  
>      ASSERT(order <= MAX_ORDER);
> @@ -1552,6 +1552,13 @@ static void free_heap_pages(
>              pg[i].count_info |= PGC_need_scrub;
>              poison_one_page(&pg[i]);
>          }
> +        else if ( test_bit(_PGC_need_scrub, &pg[i].count_info) )
> +        {
> +            /* The caller might have returned pages pending scrub. */
> +            if ( first_dirty == INVALID_DIRTY_IDX )
> +                first_dirty = i;
> +            dirty_cnt++;
> +        }

Would we perhaps want another "else" here, invoking check_one_page()?

> --- /dev/null
> +++ b/xen/common/page_alloc.h
> @@ -0,0 +1,14 @@
> +#ifndef PAGE_ALLOC_H
> +#define PAGE_ALLOC_H
> +
> +void free_heap_pages(struct page_info *pg, unsigned int order, bool 
> need_scrub);
> +
> +/* Free an allocation, and zero the pointer to it. */
> +#define FREE_HEAP_PAGES(p, o, s) do { \
> +    void *_ptr_ = (p);                \
> +    (p) = NULL;                       \
> +    free_heap_pages(_ptr_, o, s);     \
> +} while ( false )
> +#define FREE_HEAP_PAGE(p, s) FREE_HEAP_PAGES(p, 0, s)

I'm not quite sure whether in this case we need the order-0 shorthand. I'm
inclined to think that either we want to go further:

#define FREE_HEAP_PAGE(p) FREE_HEAP_PAGES(p, 0, true)

Or that we want to omit the shorthand, until a clear need appears.

Jan