Re: [PATCH v5 08/13] xen/page_alloc: introduce preserved page flags macro

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.01.2024 10:51, Carlo Nonato wrote:
> PGC_static and PGC_extra are flags that needs to be preserved when assigning
> a page. Define a new macro that groups those flags and use it instead of
> or'ing every time.
> 
> The new macro is used also in free_heap_pages() allowing future commits to
> extended it with other flags that must stop merging, as it now works for
> PGC_static. PGC_extra is no harm here since it's only ever being set on
> allocated pages.

Is it? I can't see where free_domheap_pages() would clear it before calling
free_heap_pages(). Or wait, that may happen in mark_page_free(), but then
PGC_static would be cleared there, too. I must be missing something.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -158,6 +158,8 @@
>  #define PGC_static 0
>  #endif
>  
> +#define preserved_flags (PGC_extra | PGC_static)

I think this wants to (a) have a PGC_ prefix and (b) as a #define be all
capitals.

> @@ -1504,7 +1506,7 @@ static void free_heap_pages(
>              /* Merge with predecessor block? */
>              if ( !mfn_valid(page_to_mfn(predecessor)) ||
>                   !page_state_is(predecessor, free) ||
> -                 (predecessor->count_info & PGC_static) ||
> +                 (predecessor->count_info & preserved_flags) ||
>                   (PFN_ORDER(predecessor) != order) ||
>                   (page_to_nid(predecessor) != node) )
>                  break;
> @@ -1528,7 +1530,7 @@ static void free_heap_pages(
>              /* Merge with successor block? */
>              if ( !mfn_valid(page_to_mfn(successor)) ||
>                   !page_state_is(successor, free) ||
> -                 (successor->count_info & PGC_static) ||
> +                 (successor->count_info & preserved_flags) ||
>                   (PFN_ORDER(successor) != order) ||
>                   (page_to_nid(successor) != node) )
>                  break;

Irrespective of the comment at the top, this looks like an abuse of the
new constant: There's nothing inherently making preserved flags also
suppress merging (assuming it was properly checked that both sided have
the same flags set/clear).

Jan