Re: [PATCH v10 08/12] xen/page_alloc: introduce preserved page flags macro

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Nov 28, 2024 at 12:05 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 19.11.2024 15:13, Carlo Nonato wrote:
> > PGC_static, PGC_extra and PGC_broken need 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 domain pages as this is
> > considered an error and can cause ASSERT violations.
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > ---
> > v10:
> > - fixed commit message
> > v9:
> > - add PGC_broken to PGC_preserved
> > - clear PGC_extra in alloc_domheap_pages() only if MEMF_no_refcount is set
> > v8:
> > - fixed PGC_extra ASSERT fail in alloc_domheap_pages() by removing PGC_extra
> >   before freeing
> > v7:
> > - PGC_preserved used also in mark_page_free()
> > v6:
> > - preserved_flags renamed to PGC_preserved
> > - PGC_preserved is used only in assign_pages()
> > v5:
> > - new patch
> > ---
> >  xen/common/page_alloc.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 7b911b5ed9..34cd473150 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -160,6 +160,7 @@
> >  #endif
> >
> >  #define PGC_no_buddy_merge PGC_static
> > +#define PGC_preserved (PGC_extra | PGC_static | PGC_broken)
> >
> >  #ifndef PGT_TYPE_INFO_INITIALIZER
> >  #define PGT_TYPE_INFO_INITIALIZER 0
> > @@ -1427,12 +1428,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;
>
> PGC_extra doesn't want preserving here. Since it's mark_page_free(), and
> since PGC_extra is removed before freeing, the state change is apparently
> fine. But an assertion may want adding, for documentation purposes if
> nothing else.
>
> Alternatively it may make sense to indeed exclude PGC_extra here. In fact
> PGC_static doesn't need using here either, as unprepare_staticmem_pages()
> will explicitly set it again anyway.
>
> Hence I wonder whether the change here really is necessary (one will then
> be needed in the next patch aiui, when PGC_colored is introduced). Which
> would then eliminate the need for the final two hunks of the patch, I
> think.
>
> >      case PGC_state_offlining:
> > -        pg->count_info = (pg->count_info & PGC_broken) |
> > -                         PGC_state_offlined;
> > +        pg->count_info = (pg->count_info & PGC_preserved) | 
> > PGC_state_offlined;
> >          pg_offlined = true;
> >          break;
>
> I'm similarly unconvinced that anything other than PGC_broken (and
> subsequently perhaps PGC_colored) would need preserving here.

Yes, we (re)checked the code and also believe that the introduction of
PGC_preserved is generating more confusion (and code) then the actual logical
help it provides.

We'll remove this patch and integrate PGC_colored explicitly in the flags to
be preserved. This avoid the clumsy logic of preserving something (extra)
when it's not needed and then handling the special case to remove it
explicitly.
Basically my goal is to go back to v4 where this patch didn't exist.

> Jan

Thanks.
- Carlo