Re: [PATCH 17/22] x86/setup: vmap heap nodes when they are outside the direct map

[Julien Grall <julien@xxxxxxx>]

Hi,

On 23/01/2023 22:03, Stefano Stabellini wrote:
> On Fri, 16 Dec 2022, Julien Grall wrote:
> > From: Hongyan Xia <hongyxia@xxxxxxxxxx>
> >
> > When we do not have a direct map, archs_mfn_in_direct_map() will always
> > return false, thus init_node_heap() will allocate xenheap pages from an
> > existing node for the metadata of a new node. This means that the
> > metadata of a new node is in a different node, slowing down heap
> > allocation.
> >
> > Since we now have early vmap, vmap the metadata locally in the new node.
> >
> > Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ----
> >
> >      Changes from Hongyan's version:
> >          * arch_mfn_in_direct_map() was renamed to
> >            arch_mfns_in_direct_map()
> >          * Use vmap_contig_pages() rather than __vmap(...).
> >          * Add missing include (xen/vmap.h) so it compiles on Arm
> > ---
> >   xen/common/page_alloc.c | 42 +++++++++++++++++++++++++++++++----------
> >   1 file changed, 32 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 0c4af5a71407..581c15d74dfb 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -136,6 +136,7 @@
> >   #include <xen/sched.h>
> >   #include <xen/softirq.h>
> >   #include <xen/spinlock.h>
> > +#include <xen/vmap.h>
> >   
> >   #include <asm/flushtlb.h>
> >   #include <asm/numa.h>
> > @@ -597,22 +598,43 @@ static unsigned long init_node_heap(int node, unsigned 
> > long mfn,
> >           needed = 0;
> >       }
> >       else if ( *use_tail && nr >= needed &&
> > -              arch_mfns_in_directmap(mfn + nr - needed, needed) &&
> >                 (!xenheap_bits ||
> >                  !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
> >       {
> > -        _heap[node] = mfn_to_virt(mfn + nr - needed);
> > -        avail[node] = mfn_to_virt(mfn + nr - 1) +
> > -                      PAGE_SIZE - sizeof(**avail) * NR_ZONES;
> > -    }
> > -    else if ( nr >= needed &&
> > -              arch_mfns_in_directmap(mfn, needed) &&
> > +        if ( arch_mfns_in_directmap(mfn + nr - needed, needed) )
> > +        {
> > +            _heap[node] = mfn_to_virt(mfn + nr - needed);
> > +            avail[node] = mfn_to_virt(mfn + nr - 1) +
> > +                          PAGE_SIZE - sizeof(**avail) * NR_ZONES;
> > +        }
> > +        else
> > +        {
> > +            mfn_t needed_start = _mfn(mfn + nr - needed);
> > +
> > +            _heap[node] = vmap_contig_pages(needed_start, needed);
> > +            BUG_ON(!_heap[node]);
> I see a BUG_ON here but init_node_heap is not __init.
FWIW, this is not the first introducing the first BUG_ON() in this function.

 Asking because
> BUG_ON is only a good idea during init time. Should init_node_heap be
> __init (not necessarely in this patch, but still)?
AFAIK, there are two uses outside of __init:
  1) Free the init sections
  2) Memory hotplug

In the first case, we will likely need to panic() in case of an error. 
For ther second case, I am not entirely sure.
But there would be a fair bit of plumbing and thinking (how do you deal 
with the case where part of the memory were already added?).
Anyway, I don't think I am making the function worse, so I would rather 
no open that can of worms (yet).
Cheers,

--
Julien Grall