Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

From: "Durrant, Paul" <pdurrant@amazon.co.uk>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign
Date: Tue, 28 Jan 2020 17:01:03 +0000	[thread overview]
Message-ID: <29425ac0b17d4772a162a097448cfee4@EX13D32EUC003.ant.amazon.com> (raw)
In-Reply-To: <9376dca1-1bdd-ac08-d84a-e8ac101436d2@suse.com>

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 28 January 2020 15:23
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign
> 
> On 24.01.2020 16:31, Paul Durrant wrote:
> > Currently it is unsafe to assign a domheap page allocated with
> > MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
> > be incremented, but will be decrement when the page is freed (since
> > free_domheap_pages() has no way of telling that the increment was
> skipped).
> >
> > This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
> > which is then used to mark domheap pages allocated with
> MEMF_no_refcount.
> > This then allows free_domheap_pages() to skip decrementing tot_pages
> when
> > appropriate and hence makes the pages safe to assign.
> >
> > NOTE: The patch sets MEMF_no_refcount directly in alloc_domheap_pages()
> >       rather than in assign_pages() because the latter is called with
> >       MEMF_no_refcount by memory_exchange() as an optimization, to avoid
> >       too many calls to domain_adjust_tot_pages() (which acquires and
> >       releases the global 'heap_lock').
> 
> I don't think there were any optimization thoughts with this. The
> MEMF_no_refcount use is because otherwise for a domain with
> tot_pages == max_pages the assignment would fail.
> 

That would not be the case if the calls to steal_page() further up didn't pass MEMF_no_refcount (which would be the correct thing to do if not passing it to assign_pages(). I had originally considered doing that because I think it allows the somewhat complex error path after assign_pages() to be dropped. But avoiding thrashing the global lock seemed a good reason to leave memory_exchange() the way it is.

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain
> *d, long pages)
> >  {
> >      long dom_before, dom_after, dom_claimed, sys_before, sys_after;
> >
> > +    if ( !pages )
> > +        goto out;
> 
> Unrelated change? Are there, in fact, any callers passing in 0?
> Oh, further down you add one which may do so, but then perhaps
> better to make the caller not call here (as is done e.g. in
> memory_exchange())?

I think it's preferable for domain_adjust_tot_pages() to handle zero gracefully.

> 
> > @@ -2331,11 +2331,20 @@ struct page_info *alloc_domheap_pages(
> >                                    memflags, d)) == NULL)) )
> >           return NULL;
> >
> > -    if ( d && !(memflags & MEMF_no_owner) &&
> > -         assign_pages(d, pg, order, memflags) )
> > +    if ( d && !(memflags & MEMF_no_owner) )
> >      {
> > -        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > -        return NULL;
> > +        if ( assign_pages(d, pg, order, memflags) )
> > +        {
> > +            free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > +            return NULL;
> > +        }
> > +        if ( memflags & MEMF_no_refcount )
> > +        {
> > +            unsigned long i;
> > +
> > +            for ( i = 0; i < (1 << order); i++ )
> > +                pg[i].count_info |= PGC_no_refcount;
> > +        }
> 
> I would seem to me that this needs doing the other way around:
> First set PGC_no_refcount, then assign_pages(). After all, the
> moment assign_pages() drops its lock, the domain could also
> decide to get rid of (some of) the pages again.

True. Yes, this needs to be swapped.

> For this (and
> also to slightly simplify things in free_domheap_pages())
> perhaps it would be better not to add that ASSERT() to
> free_heap_pages(). The function shouldn't really be concerned
> of any refcounting, and hence could as well be ignorant to
> PGC_no_refcount being set on a page.
> 

Not sure I understand here. What would you like to see free_heap_pages() assert?

> > @@ -2368,24 +2377,32 @@ void free_domheap_pages(struct page_info *pg,
> unsigned int order)
> >
> >          if ( likely(d) && likely(d != dom_cow) )
> >          {
> > +            long pages = 0;
> > +
> >              /* NB. May recursively lock from relinquish_memory(). */
> >              spin_lock_recursive(&d->page_alloc_lock);
> >
> >              for ( i = 0; i < (1 << order); i++ )
> >              {
> > +                unsigned long count_info = pg[i].count_info;
> > +
> >                  if ( pg[i].u.inuse.type_info & PGT_count_mask )
> >                  {
> >                      printk(XENLOG_ERR
> >                             "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx
> t=%#x\n",
> >                             i, mfn_x(page_to_mfn(pg + i)),
> > -                           pg[i].count_info, pg[i].v.free.order,
> > +                           count_info, pg[i].v.free.order,
> >                             pg[i].u.free.val, pg[i].tlbflush_timestamp);
> >                      BUG();
> >                  }
> >                  arch_free_heap_page(d, &pg[i]);
> > +                if ( count_info & PGC_no_refcount )
> > +                    pg[i].count_info &= ~PGC_no_refcount;
> > +                else
> > +                    pages--;
> 
> Not only to reduce code churn, may I recommend to avoid introducing
> the local variable? There's no strict rule preventing
> arch_free_heap_page() from possibly playing with the field you
> latch up front.

Ok.

  Paul

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