Re: [PATCH 05/12] xen: introduce reserve_heap_pages

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 17 Apr 2020, Jan Beulich wrote:
> On 15.04.2020 03:02, Stefano Stabellini wrote:
> > Introduce a function named reserve_heap_pages (similar to
> > alloc_heap_pages) that allocates a requested memory range. Call
> > __alloc_heap_pages for the implementation.
> > 
> > Change __alloc_heap_pages so that the original page doesn't get
> > modified, giving back unneeded memory top to bottom rather than bottom
> > to top.
> 
> While it may be less of a problem within a zone, doing so is
> against our general "return high pages first" policy.

Is this something you'd be OK with anyway?

If not, do you have a suggestion on how to do it better? I couldn't find
a nice way to do it without code duplication, or a big nasty 'if' in the
middle of the function.


> > @@ -1073,7 +1073,42 @@ static struct page_info *alloc_heap_pages(
> >          return NULL;
> >      }
> >  
> > -    __alloc_heap_pages(&pg, order, memflags, d);
> > +    __alloc_heap_pages(pg, order, memflags, d);
> > +    return pg;
> > +}
> > +
> > +static struct page_info *reserve_heap_pages(struct domain *d,
> > +                                            paddr_t start,
> > +                                            unsigned int order,
> > +                                            unsigned int memflags)
> > +{
> > +    nodeid_t node;
> > +    unsigned int zone;
> > +    struct page_info *pg;
> > +
> > +    if ( unlikely(order > MAX_ORDER) )
> > +        return NULL;
> > +
> > +    spin_lock(&heap_lock);
> > +
> > +    /*
> > +     * Claimed memory is considered unavailable unless the request
> > +     * is made by a domain with sufficient unclaimed pages.
> > +     */
> > +    if ( (outstanding_claims + (1UL << order) > total_avail_pages) &&
> > +          ((memflags & MEMF_no_refcount) ||
> > +           !d || d->outstanding_pages < (1UL << order)) )
> > +    {
> > +        spin_unlock(&heap_lock);
> > +        return NULL;
> > +    }
> 
> Where would such a claim come from? Given the purpose I'd assume
> the function (as well as reserve_domheap_pages()) can actually be
> __init. With that I'd then also be okay with them getting built
> unconditionally, i.e. even on x86.

Yes, you are right, I'll make the function __init and also remove this
check on claimed memory.


> > +    pg = maddr_to_page(start);
> > +    node = phys_to_nid(start);
> > +    zone = page_to_zone(pg);
> > +    page_list_del(pg, &heap(node, zone, order));
> > +
> > +    __alloc_heap_pages(pg, order, memflags, d);
> 
> I agree with Julien in not seeing how this can be safe / correct.

I haven't seen any issues so far in my testing -- I imagine it is
because there aren't many memory allocations after setup_mm() and before
create_domUs()  (which on ARM is called just before
domain_unpause_by_systemcontroller at the end of start_xen.)


I gave a quick look at David's series. Is the idea that I should add a
patch to do the following:

- avoiding adding these ranges to xenheap in setup_mm, wait for later
  (a bit like reserved_mem regions)

- in construct_domU, add the range to xenheap and reserve it with 
reserve_heap_pages

Is that right?