Re: [Xen-devel] [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 16 March 2020 16:53
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Julien 
> Grall <julien@xxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau 
> Monné
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list
> 
> On 10.03.2020 18:49, paul@xxxxxxx wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -257,6 +257,13 @@ void dump_pageframe_info(struct domain *d)
> >                 _p(mfn_x(page_to_mfn(page))),
> >                 page->count_info, page->u.inuse.type_info);
> >      }
> > +
> > +    page_list_for_each ( page, &d->extra_page_list )
> > +    {
> > +        printk("    ExtraPage %p: caf=%08lx, taf=%" PRtype_info "\n",
> > +               _p(mfn_x(page_to_mfn(page))),
> > +               page->count_info, page->u.inuse.type_info);
> > +    }
> >      spin_unlock(&d->page_alloc_lock);
> 
> Another blank line above here would have been nice.
> 

Ok.

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2314,7 +2314,7 @@ int assign_pages(
> >          smp_wmb(); /* Domain pointer must be visible before updating 
> > refcnt. */
> >          pg[i].count_info =
> >              (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> > -        page_list_add_tail(&pg[i], &d->page_list);
> > +        page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
> >      }
> 
> This moves the one extra page we currently have (VMX'es APIC access
> page) to a different list. Without adjustment to domain cleanup,
> how is this page now going to get freed? (This of course then should
> be extended to Arm, even if right now there's no "extra" page there.)
> 

I don't think there is any need to adjust as the current code in will drop the 
allocation ref in vmx_free_vlapic_mapping(), so it doesn't matter that it is 
missed by relinquish_memory().

> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -629,10 +629,8 @@ typedef struct mm_rwlock {
> >      const char        *locker_function; /* func that took it */
> >  } mm_rwlock_t;
> >
> > -#define arch_free_heap_page(d, pg)                                      \
> > -    page_list_del2(pg, is_xen_heap_page(pg) ?                           \
> > -                       &(d)->xenpage_list : &(d)->page_list,            \
> > -                   &(d)->arch.relmem_list)
> > +#define arch_free_heap_page(d, pg) \
> > +    page_list_del2(pg, page_to_list((d), (pg)), &(d)->arch.relmem_list)
> 
> Arguments passed on as is (i.e. not as part of an expression) don't
> need parentheses.
> 

Are you saying it should be:

#define arch_free_heap_page(d, pg) \
    page_list_del2(pg, page_to_list(d, pg), &(d)->arch.relmem_list)

?

> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -583,9 +583,8 @@ static inline unsigned int 
> > get_order_from_pages(unsigned long nr_pages)
> >  void scrub_one_page(struct page_info *);
> >
> >  #ifndef arch_free_heap_page
> > -#define arch_free_heap_page(d, pg)                      \
> > -    page_list_del(pg, is_xen_heap_page(pg) ?            \
> > -                      &(d)->xenpage_list : &(d)->page_list)
> > +#define arch_free_heap_page(d, pg) \
> > +    page_list_del(pg, page_to_list((d), (pg)))
> 
> Same here then.
> 
> > @@ -538,6 +539,17 @@ struct domain
> >  #endif
> >  };
> >
> > +static inline struct page_list_head *page_to_list(
> > +    struct domain *d, const struct page_info *pg)
> > +{
> > +    if ( is_xen_heap_page(pg) )
> > +        return &d->xenpage_list;
> > +    else if ( pg->count_info & PGC_extra )
> 
> Unnecessary "else".
>

Oh yes.

  Paul

 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel