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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2026 16:53, Roger Pau Monné wrote:
> 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?

Oh, yes, sure. I was merely trying to address your remark.

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

It would help if others could voice an opinion. To me exposing this
supposedly internal (to the page allocator) function feels a little
risky. Yet of course any undue use would likely be spotted and objected
to during review.

Jan