On 22/12/2023 13:58, Julien Grall wrote:
Hi Jan,
On 22/12/2023 13:20, Jan Beulich wrote:
On 22.12.2023 14:07, Oleksii wrote:
On Fri, 2023-12-22 at 09:22 +0100, Jan Beulich wrote:
On 21.12.2023 20:09, Julien Grall wrote:
On 20/12/2023 14:08, Oleksii Kurochko wrote:
<asm/numa.h> is common through some archs so it is moved
to asm-generic.
Signed-off-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
Reviewed-by: Michal Orzel <michal.or...@amd.com>
Acked-by: Jan Beulich <jbeul...@suse.com>
Acked-by: Shawn Anastasio <sanasta...@raptorengineering.com>
I think this patch will need to be rebased on top of the lastest
staging
as this should clash with 51ffb3311895.
No, and I'd like to withdraw my ack here. In this case a stub header
isn't
the right choice imo - the !NUMA case should be handled in common
code. I
would have submitted the patch I have, if only the first_valid_mfn
patch
hadn't been committed already (which I now need to re-base over).
I haven't seen your patch yet (waiting for it), but I assume that
<asm/numa.h> will still be necessary and remain the same across Arm,
PPC, and RISC-V.
What am I missing?
asm/numa.h will be necessary for an arch only if it actually has a way
to have CONFIG_NUMA enabled. Below, for your reference, the patch in
the not-yet-rebased form. As you can see, Arm's (and PPC's) header goes
away. If and when they choose to support NUMA, they may need to regain
one; whether at that point we can sort how an asm-generic/ form of the
header might sensibly look like remains to be seen.
Jan
NUMA: no need for asm/numa.h when !NUMA
There's no point in every architecture carrying the same stubs for the
case when NUMA isn't enabled (or even supported). Move all of that to
xen/numa.h; replace explicit uses of asm/numa.h in common code. Make
inclusion of asm/numa.h dependent upon NUMA=y.
Address the TODO regarding first_valid_mfn by making the variable static
when NUMA=y, thus also addressing a Misrs C:2012 rule 8.4 concern. Drop
the not really applicable "implement NUMA support" comment - in a !NUMA
section this simply makes no sense.
I have to admit, I am not really thrill with the approach you have
taken. As I said before, I don't quite understand the problem of having
first_valid_mfn exposed in all the circumstances.
This is better than trying to do...
--- unstable.orig/xen/common/page_alloc.c
+++ unstable/xen/common/page_alloc.c
@@ -140,7 +140,6 @@
#include <xen/vm_event.h>
#include <asm/flushtlb.h>
-#include <asm/numa.h>
#include <asm/page.h>
#include <public/sysctl.h>
@@ -256,10 +255,14 @@ static PAGE_LIST_HEAD(page_broken_list);
* BOOT-TIME ALLOCATOR
*/
+#ifndef CONFIG_NUMA
/*
* first_valid_mfn is exported because it is use in ARM specific NUMA
* helpers. See comment in arch/arm/include/asm/numa.h.
*/
+#else
+static
+#endif
... this sort ugliness.
Just to add more thoughts. My main concern here is that we likely have
other variables that may only be exposed for a given arch. I would
rather not like if we start sprinkling the code with #ifdef CONFIG_XXX
static #else.
If there is a desire for everyone to start reducing the scope of the
variable. Then we should come up with a way to do what you did above on
the same line rather than multiple one. Maybe:
STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn.
Cheers,
--
Julien Grall
