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

From: Paul Durrant <xadimgnik@gmail.com>
To: "'Jan Beulich'" <jbeulich@suse.com>
Cc: "'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Julien Grall'" <julien@xen.org>, "'Wei Liu'" <wl@xen.org>,
	"'Andrew Cooper'" <andrew.cooper3@citrix.com>,
	"'Ian Jackson'" <ian.jackson@eu.citrix.com>,
	"'George Dunlap'" <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	"'Roger Pau Monné'" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list
Date: Mon, 16 Mar 2020 18:11:59 -0000	[thread overview]
Message-ID: <003f01d5fbbe$64271aa0$2c754fe0$@xen.org> (raw)
In-Reply-To: <a7ab5e54-6ce1-4e3f-0014-9ecebf90d6fa@suse.com>

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 16 March 2020 16:53
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap
> <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>;
> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: Re: [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list
> 
> On 10.03.2020 18:49, paul@xen.org 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel