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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Tue, Nov 5, 2024 at 4:59 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 25.10.2024 11:50, Carlo Nonato wrote:
> > PGC_static and PGC_extra 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.
> >
> > To make preserved flags even more meaningful, they are kept also when
> > switching state in mark_page_free().
> > Enforce the removal of PGC_extra before freeing new pages as this is
> > considered an error and can cause ASSERT violations.
>
> What does "new" here mean?

I believe it means "assigned to domains". So yes "new" it's definitely wrong.

> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > ---
> > v9:
> > - add PGC_broken to PGC_preserved
>
> Which then also wants reflecting in the description.

Yep.

> > @@ -2485,6 +2485,14 @@ struct page_info *alloc_domheap_pages(
> >          }
> >          if ( assign_page(pg, order, d, memflags) )
> >          {
> > +            if ( memflags & MEMF_no_refcount )
> > +            {
> > +                unsigned long i;
> > +
> > +                for ( i = 0; i < (1UL << order); i++ )
> > +                    pg[i].count_info &= ~PGC_extra;
> > +            }
>
> The description doesn't cover this, only ...

PGC_extra must be cleared before freeing pages. Since here we're going to free
pages, PGC_extra must be removed.

> > @@ -2539,6 +2547,7 @@ void free_domheap_pages(struct page_info *pg, 
> > unsigned int order)
> >                  {
> >                      ASSERT(d->extra_pages);
> >                      d->extra_pages--;
> > +                    pg[i].count_info &= ~PGC_extra;
> >                  }
> >              }
>
> ... this is mentioned there. And it's not clear to me why it would need doing
> in both places.
>
> Jan

- Carlo