Re: [PATCH v4 2/6] xen/x86: move generically usable NUMA code from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2022 12:32, Wei Chen wrote:
> On 2022/9/8 17:09, Jan Beulich wrote:
>> On 02.09.2022 05:31, Wei Chen wrote:
>>> --- /dev/null
>>> +++ b/xen/common/numa.c
>>> @@ -0,0 +1,442 @@
>>> +/*
>>> + * Generic VM initialization for NUMA setups.
>>> + * Copyright 2002,2003 Andi Kleen, SuSE Labs.
>>> + * Adapted for Xen: Ryan Harper <ryanh@xxxxxxxxxx>
>>> + */
>>> +
>>> +#include <xen/init.h>
>>> +#include <xen/keyhandler.h>
>>> +#include <xen/mm.h>
>>> +#include <xen/nodemask.h>
>>> +#include <xen/numa.h>
>>> +#include <xen/param.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/softirq.h>
>>> +
>>> +struct node_data __ro_after_init node_data[MAX_NUMNODES];
>>> +
>>> +/* Mapping from pdx to node id */
>>> +unsigned int __ro_after_init memnode_shift;
>>> +unsigned long __ro_after_init memnodemapsize;
>>> +uint8_t *__ro_after_init memnodemap;
>>> +static uint8_t __ro_after_init _memnodemap[64];
>>
>> These last two want to use nodeid_t instead of uint8_t. Originally
>> the latter used typeof(*memnodemap) for (I think - iirc it was me who
>> made it that way) this reason: That way correcting memnodemap's type
>> would have propagated without the need for further adjustments.
>>
> 
> Thanks for this info, should I need to restore it to use
> "typeof(*memnodemap)" in next version ?

That would be more in line with the original code, but it's not
strictly necessary once nodeid_t if properly used for these variables.
I'd leave it up to you as long as you switch to nodeid_t.

>>> +nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
>>> +    [0 ... NR_CPUS-1] = NUMA_NO_NODE
>>> +};
>>> +
>>> +cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>>> +
>>> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>>> +
>>> +bool __read_mostly numa_off;
>>
>> The v3 review discussing this possibly becoming __ro_after_init didn't
>> really finish (you didn't reply to my latest request there), but you
>> also didn't change the attribute. Please clarify.
>>
> 
> I think I had answered your question by:
>  >> I think yes, it will be used in numa_disabled and numa_disabled will
>  >> be called in cpu_add."
> 
> And you replied me with:
>  > In the original code I cannot spot such a path - can you please point
>  > out how exactly you see numa_disabled() reachable from cpu_add()? I'm
>  > clearly overlooking something ..."
> 
> But there is a time difference here, your reply was sent after I sent 
> v3, maybe you didn't notice it

Which suggests you might better have waited with sending v3 until the
discussion had settled.

> About the new question:
> cpu_add will call srat_disabled, srat_disabled will access numa_off.
> srat_disabled is a function without __init.

But the request wasn't to make the variable __initdata. That would be
wrong of course. Since srat_disabled() only reads numa_off,
__ro_after_init does look usable to me.

Jan