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

From: Jan Beulich <jbeulich@suse.com>
To: Paul Durrant <pdurrant@amazon.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,
	"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 16:23:02 +0100	[thread overview]
Message-ID: <9376dca1-1bdd-ac08-d84a-e8ac101436d2@suse.com> (raw)
In-Reply-To: <20200124153103.18321-6-pdurrant@amazon.com>

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.

> --- 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())?

> @@ -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. 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.

> @@ -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.

Jan

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