From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9C244C25B50 for ; Mon, 23 Jan 2023 22:23:23 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.483299.749379 (Exim 4.92) (envelope-from ) id 1pK5DW-0005wU-Qu; Mon, 23 Jan 2023 22:23:06 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 483299.749379; Mon, 23 Jan 2023 22:23:06 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1pK5DW-0005wN-OL; Mon, 23 Jan 2023 22:23:06 +0000 Received: by outflank-mailman (input) for mailman id 483299; Mon, 23 Jan 2023 22:23:05 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1pK5DV-0005wF-Fu for xen-devel@lists.xenproject.org; Mon, 23 Jan 2023 22:23:05 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1pK5DU-0005Yv-Q2; Mon, 23 Jan 2023 22:23:04 +0000 Received: from gw1.octic.net ([88.97.20.152] helo=[10.0.1.102]) by xenbits.xenproject.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1pK5DU-0001ZS-Ks; Mon, 23 Jan 2023 22:23:04 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:Subject: From:References:Cc:To:MIME-Version:Date:Message-ID; bh=M5CGCxzzAW/bES03HcF9ha3g+oYu/Y8f85jI11wEIAk=; b=ALXbaZxEeXG78HSgHJu9IQ6em+ /pG+51DUuEFzxqjQbAZGK7P8FC6f97vXeG6c7Hpwd6eR8quXpqrFzhVjp6UlKqa2HsUiGm9khF3wS xOPH29gFaAGlBUFpoJx66V80mQ1JTPUbsL6uPs2KFfxYBzUr4fDsvotzX21ht48hDjJ8=; Message-ID: Date: Mon, 23 Jan 2023 22:23:02 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, Hongyan Xia , Andrew Cooper , George Dunlap , Jan Beulich , Wei Liu , Julien Grall References: <20221216114853.8227-1-julien@xen.org> <20221216114853.8227-18-julien@xen.org> From: Julien Grall Subject: Re: [PATCH 17/22] x86/setup: vmap heap nodes when they are outside the direct map In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, On 23/01/2023 22:03, Stefano Stabellini wrote: > On Fri, 16 Dec 2022, Julien Grall wrote: >> From: Hongyan Xia >> >> 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 >> Signed-off-by: Julien Grall >> >> ---- >> >> 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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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