Hi,
On 23/01/2023 22:03, Stefano Stabellini wrote:
On Fri, 16 Dec 2022, Julien Grall wrote:
From: Hongyan Xia <hongy...@amazon.com>
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 <hongy...@amazon.com>
Signed-off-by: Julien Grall <jgr...@amazon.com>
----
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
