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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, May 6, 2024 at 2:22 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 02.05.2024 18:55, Carlo Nonato wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -159,6 +159,7 @@
> >  #endif
> >
> >  #define PGC_no_buddy_merge PGC_static
> > +#define PGC_preserved (PGC_extra | PGC_static)
>
> Seeing this again and its use ...
>
> > @@ -1426,11 +1427,11 @@ static bool mark_page_free(struct page_info *pg, 
> > mfn_t mfn)
> >      {
> >      case PGC_state_inuse:
> >          BUG_ON(pg->count_info & PGC_broken);
> > -        pg->count_info = PGC_state_free;
> > +        pg->count_info = PGC_state_free | (pg->count_info & PGC_preserved);
> >          break;
> >
> >      case PGC_state_offlining:
> > -        pg->count_info = (pg->count_info & PGC_broken) |
> > +        pg->count_info = (pg->count_info & (PGC_broken | PGC_preserved)) |
> >                           PGC_state_offlined;
> >          pg_offlined = true;
> >          break;
>
> ... here: Shouldn't PGC_broken also be included in PGC_preserved?

I hope there are no other problems like the one with PGC_extra.

> > @@ -2484,6 +2485,11 @@ struct page_info *alloc_domheap_pages(
> >          }
> >          if ( assign_page(pg, order, d, memflags) )
> >          {
> > +            unsigned long i;
> > +
> > +            for ( i = 0; i < (1UL << order); i++ )
> > +                pg[i].count_info &= ~PGC_extra;
>
> For larger order this loop is non-trivial and may have a fair effect on
> caches. Looking at the code just outside of upper patch context, is this
> loop needed at all when MEMF_no_refcount is clear in memflags?

Nope. I will wrap it in a if.

Thanks.

> Jan