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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, Jan 8, 2024 at 6:08 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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).

Sorry, I may have misinterpreted your comments on the previous version of the
series (I know it was a really long time ago)

https://patchew.org/Xen/20230123154735.74832-1-carlo.nonato@xxxxxxxxxxxxxxx/20230123154735.74832-8-carlo.nonato@xxxxxxxxxxxxxxx/#c843b031-52f7-056d-e8c0-75fe9c426343@xxxxxxxx

Anyway, would the solution here be to have two distinct #define? One for
suppress merging and the other for preserved flags. This would probably also
remove any confusion with the usage of PGC_extra.

Thanks.

> Jan