[RFC,v2,04/25] x86: NUMA: Add accessors for acpi_numa, numa_off and numa_fake variables

Commit Message

Vijay Kilari March 28, 2017, 3:53 p.m. UTC

From: Vijaya Kumar K <[e-mail]>

Add accessor functions for acpi_numa and numa_off static
variables. Init value of acpi_numa is set 1 instead of 0.
Also return value of srat_disabled is changed to bool.

Signed-off-by: Vijaya Kumar K <[e-mail]>
---
 xen/arch/x86/numa.c        | 43 +++++++++++++++++++++++++++++++------------
 xen/arch/x86/setup.c       |  2 +-
 xen/arch/x86/srat.c        | 12 ++++++------
 xen/include/asm-x86/acpi.h |  1 -
 xen/include/asm-x86/numa.h |  5 +----
 xen/include/xen/numa.h     |  3 +++
 6 files changed, 42 insertions(+), 24 deletions(-)

Comments

Julien Grall April 20, 2017, 3:59 p.m. UTC | #1

Hi Vijay,

On 28/03/17 16:53, [e-mail] wrote:
> From: Vijaya Kumar K <[e-mail]>
>
> Add accessor functions for acpi_numa and numa_off static
> variables. Init value of acpi_numa is set 1 instead of 0.

Please explain why you change the acpi_numa value from 0 to 1.

Also, I am not sure to understand the benefits of those helpers. Why do 
you need them? Why not using the global variable directly, this will 
avoid to call a function just for returning a value...

> Also return value of srat_disabled is changed to bool.
>
> Signed-off-by: Vijaya Kumar K <[e-mail]>
> ---
>  xen/arch/x86/numa.c        | 43 +++++++++++++++++++++++++++++++------------
>  xen/arch/x86/setup.c       |  2 +-
>  xen/arch/x86/srat.c        | 12 ++++++------
>  xen/include/asm-x86/acpi.h |  1 -
>  xen/include/asm-x86/numa.h |  5 +----
>  xen/include/xen/numa.h     |  3 +++
>  6 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> index 964fc5a..6b794a7 100644
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -42,12 +42,27 @@ cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>
>  nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>
> -bool numa_off = 0;
> -s8 acpi_numa = 0;
> +static bool numa_off = 0;
> +static bool acpi_numa = 1;

Please don't mix 0/1 and bool. Instead use false/true.

>
> -int srat_disabled(void)
> +bool is_numa_off(void)
>  {
> -    return numa_off || acpi_numa < 0;
> +    return numa_off;
> +}
> +
> +bool get_acpi_numa(void)
> +{
> +    return acpi_numa;
> +}
> +
> +void set_acpi_numa(bool_t val)
> +{
> +    acpi_numa = val;
> +}
> +
> +bool srat_disabled(void)
> +{
> +    return numa_off || acpi_numa == 0;
>  }
>
>  /*
> @@ -202,13 +217,17 @@ void __init numa_init_array(void)
>
>  #ifdef CONFIG_NUMA_EMU
>  static int __initdata numa_fake = 0;
> +static int get_numa_fake(void)
> +{
> +    return numa_fake;
> +}
>
>  /* Numa emulation */
>  static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
>  {
>      int i;
>      struct node nodes[MAX_NUMNODES];
> -    uint64_t sz = ((end_pfn - start_pfn) << PAGE_SHIFT) / numa_fake;
> +    uint64_t sz = ((end_pfn - start_pfn) << PAGE_SHIFT) / get_numa_fake();
>
>      /* Kludge needed for the hash function */
>      if ( hweight64(sz) > 1 )
> @@ -223,10 +242,10 @@ static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
>      }
>
>      memset(&nodes,0,sizeof(nodes));
> -    for ( i = 0; i < numa_fake; i++ )
> +    for ( i = 0; i < get_numa_fake(); i++ )
>      {
>          nodes[i].start = (start_pfn << PAGE_SHIFT) + i * sz;
> -        if ( i == numa_fake - 1 )
> +        if ( i == get_numa_fake() - 1 )
>              sz = (end_pfn << PAGE_SHIFT) - nodes[i].start;
>          nodes[i].end = nodes[i].start + sz;
>          printk(KERN_INFO
> @@ -235,7 +254,7 @@ static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
>                 (nodes[i].end - nodes[i].start) >> 20);
>          node_set_online(i);
>      }
> -    if ( compute_memnode_shift(nodes, numa_fake, NULL, &memnode_shift) )
> +    if ( compute_memnode_shift(nodes, get_numa_fake(), NULL, &memnode_shift) )
>      {
>          memnode_shift = 0;
>          printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
> @@ -254,18 +273,18 @@ void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn)
>      int i;
>
>  #ifdef CONFIG_NUMA_EMU
> -    if ( numa_fake && !numa_emulation(start_pfn, end_pfn) )
> +    if ( get_numa_fake() && !numa_emulation(start_pfn, end_pfn) )
>          return;
>  #endif
>
>  #ifdef CONFIG_ACPI_NUMA
> -    if ( !numa_off && !acpi_scan_nodes((uint64_t)start_pfn << PAGE_SHIFT,
> +    if ( !is_numa_off() && !acpi_scan_nodes((uint64_t)start_pfn << PAGE_SHIFT,
>           (uint64_t)end_pfn << PAGE_SHIFT) )
>          return;
>  #endif
>
>      printk(KERN_INFO "%s\n",
> -           numa_off ? "NUMA turned off" : "No NUMA configuration found");
> +           is_numa_off() ? "NUMA turned off" : "No NUMA configuration found");
>
>      printk(KERN_INFO "Faking a node at %016"PRIx64"-%016"PRIx64"\n",
>             (uint64_t)start_pfn << PAGE_SHIFT,
> @@ -312,7 +331,7 @@ static int __init numa_setup(char *opt)
>      if ( !strncmp(opt,"noacpi",6) )
>      {
>          numa_off = 0;
> -        acpi_numa = -1;
> +        acpi_numa = 0;
>      }
>  #endif
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 1cd290e..4410e53 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -241,7 +241,7 @@ void srat_detect_node(int cpu)
>      node_set_online(node);
>      numa_set_node(cpu, node);
>
> -    if ( opt_cpu_info && acpi_numa > 0 )
> +    if ( opt_cpu_info && get_acpi_numa() )
>          printk("CPU %d APIC %d -> Node %d\n", cpu, apicid, node);
>  }
>
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 2d0c047..ccacbcd 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -153,7 +153,7 @@ static void __init bad_srat(void)
>  {
>  	int i;
>  	printk(KERN_ERR "SRAT: SRAT not used.\n");
> -	acpi_numa = -1;
> +	set_acpi_numa(0);
>  	for (i = 0; i < MAX_LOCAL_APIC; i++)
>  		apicid_to_node[i] = NUMA_NO_NODE;
>  	for (i = 0; i < ARRAY_SIZE(pxm2node); i++)
> @@ -232,7 +232,7 @@ acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *pa)
>
>  	apicid_to_node[pa->apic_id] = node;
>  	node_set(node, processor_nodes_parsed);
> -	acpi_numa = 1;
> +	set_acpi_numa(1);
>  	printk(KERN_INFO "SRAT: PXM %u -> APIC %08x -> Node %u\n",
>  	       pxm, pa->apic_id, node);
>  }
> @@ -265,7 +265,7 @@ acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *pa)
>  	}
>  	apicid_to_node[pa->apic_id] = node;
>  	node_set(node, processor_nodes_parsed);
> -	acpi_numa = 1;
> +	set_acpi_numa(1);
>  	printk(KERN_INFO "SRAT: PXM %u -> APIC %02x -> Node %u\n",
>  	       pxm, pa->apic_id, node);
>  }
> @@ -418,7 +418,7 @@ static int __init srat_parse_region(struct acpi_subtable_header *header,
>  	    (ma->flags & ACPI_SRAT_MEM_NON_VOLATILE))
>  		return 0;
>
> -	if (numa_off)
> +	if (is_numa_off())
>  		printk(KERN_INFO "SRAT: %013"PRIx64"-%013"PRIx64"\n",
>  		       ma->base_address, ma->base_address + ma->length - 1);
>
> @@ -433,7 +433,7 @@ void __init srat_parse_regions(uint64_t addr)
>  	uint64_t mask;
>  	unsigned int i;
>
> -	if (acpi_disabled || acpi_numa < 0 ||
> +	if (acpi_disabled || (get_acpi_numa() == 0) ||
>  	    acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
>  		return;
>
> @@ -462,7 +462,7 @@ int __init acpi_scan_nodes(uint64_t start, uint64_t end)
>  	for (i = 0; i < MAX_NUMNODES; i++)
>  		cutoff_node(i, start, end);
>
> -	if (acpi_numa <= 0)
> +	if (get_acpi_numa() == 0)
>  		return -1;
>
>  	if (!nodes_cover_memory()) {
> diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
> index a766688..9298d42 100644
> --- a/xen/include/asm-x86/acpi.h
> +++ b/xen/include/asm-x86/acpi.h
> @@ -103,7 +103,6 @@ extern void acpi_reserve_bootmem(void);
>
>  #define ARCH_HAS_POWER_INIT	1
>
> -extern s8 acpi_numa;
>  extern int acpi_scan_nodes(u64 start, u64 end);
>  #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
>
> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> index bb22bff..ae5768b 100644
> --- a/xen/include/asm-x86/numa.h
> +++ b/xen/include/asm-x86/numa.h
> @@ -30,10 +30,7 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
>
>  extern void numa_add_cpu(int cpu);
>  extern void numa_init_array(void);
> -extern bool_t numa_off;
> -
> -
> -extern int srat_disabled(void);
> +extern bool srat_disabled(void);
>  extern void numa_set_node(int cpu, nodeid_t node);
>  extern nodeid_t setup_node(unsigned int pxm);
>  extern void srat_detect_node(int cpu);
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index 7aef1a8..7f6d090 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -18,4 +18,7 @@
>    (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
>     ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
>
> +bool is_numa_off(void);
> +bool get_acpi_numa(void);
> +void set_acpi_numa(bool val);

I am not sure to understand why you add those helpers directly here in 
xen/numa.h. IHMO, This should belong to the moving code patches.

>  #endif /* _XEN_NUMA_H */
>

Vijay Kilari April 25, 2017, 6:54 a.m. UTC | #2

On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <[e-mail]> wrote:
> Hi Vijay,
>
> On 28/03/17 16:53, [e-mail] wrote:
>>
>> From: Vijaya Kumar K <[e-mail]>
>>
>> Add accessor functions for acpi_numa and numa_off static
>> variables. Init value of acpi_numa is set 1 instead of 0.
>
>
> Please explain why you change the acpi_numa value from 0 to 1.

previously acpi_numa was s8 and are using 0 and -1 values. I have made
it bool and set
the initial value to 1.

By setting 1, we are enabling acpi_numa by default. If not enabled, the below
call has check srat_disabled() before proceeding fails.

acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
{
....

    if ( srat_disabled() )
        return;

}

>
> Also, I am not sure to understand the benefits of those helpers. Why do you
> need them? Why not using the global variable directly, this will avoid to
> call a function just for returning a value...

These helpers are used by both common code and arch specific code later.
Hence made these global variables as static and added helpers

>
>> Also return value of srat_disabled is changed to bool.
>>
>> Signed-off-by: Vijaya Kumar K <[e-mail]>
>> ---
>>  xen/arch/x86/numa.c        | 43
>> +++++++++++++++++++++++++++++++------------
>>  xen/arch/x86/setup.c       |  2 +-
>>  xen/arch/x86/srat.c        | 12 ++++++------
>>  xen/include/asm-x86/acpi.h |  1 -
>>  xen/include/asm-x86/numa.h |  5 +----
>>  xen/include/xen/numa.h     |  3 +++
>>  6 files changed, 42 insertions(+), 24 deletions(-)
>>
>> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
>> index 964fc5a..6b794a7 100644
>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -42,12 +42,27 @@ cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>>
>>  nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>>
>> -bool numa_off = 0;
>> -s8 acpi_numa = 0;
>> +static bool numa_off = 0;
>> +static bool acpi_numa = 1;
>
>
> Please don't mix 0/1 and bool. Instead use false/true.

OK.
>
>
>>
>> -int srat_disabled(void)
>> +bool is_numa_off(void)
>>  {
>> -    return numa_off || acpi_numa < 0;
>> +    return numa_off;
>> +}
>> +
>> +bool get_acpi_numa(void)
>> +{
>> +    return acpi_numa;
>> +}
>> +
>> +void set_acpi_numa(bool_t val)
>> +{
>> +    acpi_numa = val;
>> +}
>> +
>> +bool srat_disabled(void)
>> +{
>> +    return numa_off || acpi_numa == 0;
>>  }
>>
>>  /*
>> @@ -202,13 +217,17 @@ void __init numa_init_array(void)
>>
>>  #ifdef CONFIG_NUMA_EMU
>>  static int __initdata numa_fake = 0;
>> +static int get_numa_fake(void)
>> +{
>> +    return numa_fake;
>> +}
>>
>>  /* Numa emulation */
>>  static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
>>  {
>>      int i;
>>      struct node nodes[MAX_NUMNODES];
>> -    uint64_t sz = ((end_pfn - start_pfn) << PAGE_SHIFT) / numa_fake;
>> +    uint64_t sz = ((end_pfn - start_pfn) << PAGE_SHIFT) /
>> get_numa_fake();
>>
>>      /* Kludge needed for the hash function */
>>      if ( hweight64(sz) > 1 )
>> @@ -223,10 +242,10 @@ static int __init numa_emulation(uint64_t start_pfn,
>> uint64_t end_pfn)
>>      }
>>
>>      memset(&nodes,0,sizeof(nodes));
>> -    for ( i = 0; i < numa_fake; i++ )
>> +    for ( i = 0; i < get_numa_fake(); i++ )
>>      {
>>          nodes[i].start = (start_pfn << PAGE_SHIFT) + i * sz;
>> -        if ( i == numa_fake - 1 )
>> +        if ( i == get_numa_fake() - 1 )
>>              sz = (end_pfn << PAGE_SHIFT) - nodes[i].start;
>>          nodes[i].end = nodes[i].start + sz;
>>          printk(KERN_INFO
>> @@ -235,7 +254,7 @@ static int __init numa_emulation(uint64_t start_pfn,
>> uint64_t end_pfn)
>>                 (nodes[i].end - nodes[i].start) >> 20);
>>          node_set_online(i);
>>      }
>> -    if ( compute_memnode_shift(nodes, numa_fake, NULL, &memnode_shift) )
>> +    if ( compute_memnode_shift(nodes, get_numa_fake(), NULL,
>> &memnode_shift) )
>>      {
>>          memnode_shift = 0;
>>          printk(KERN_ERR "No NUMA hash function found. Emulation
>> disabled.\n");
>> @@ -254,18 +273,18 @@ void __init numa_initmem_init(unsigned long
>> start_pfn, unsigned long end_pfn)
>>      int i;
>>
>>  #ifdef CONFIG_NUMA_EMU
>> -    if ( numa_fake && !numa_emulation(start_pfn, end_pfn) )
>> +    if ( get_numa_fake() && !numa_emulation(start_pfn, end_pfn) )
>>          return;
>>  #endif
>>
>>  #ifdef CONFIG_ACPI_NUMA
>> -    if ( !numa_off && !acpi_scan_nodes((uint64_t)start_pfn << PAGE_SHIFT,
>> +    if ( !is_numa_off() && !acpi_scan_nodes((uint64_t)start_pfn <<
>> PAGE_SHIFT,
>>           (uint64_t)end_pfn << PAGE_SHIFT) )
>>          return;
>>  #endif
>>
>>      printk(KERN_INFO "%s\n",
>> -           numa_off ? "NUMA turned off" : "No NUMA configuration found");
>> +           is_numa_off() ? "NUMA turned off" : "No NUMA configuration
>> found");
>>
>>      printk(KERN_INFO "Faking a node at %016"PRIx64"-%016"PRIx64"\n",
>>             (uint64_t)start_pfn << PAGE_SHIFT,
>> @@ -312,7 +331,7 @@ static int __init numa_setup(char *opt)
>>      if ( !strncmp(opt,"noacpi",6) )
>>      {
>>          numa_off = 0;
>> -        acpi_numa = -1;
>> +        acpi_numa = 0;
>>      }
>>  #endif
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 1cd290e..4410e53 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -241,7 +241,7 @@ void srat_detect_node(int cpu)
>>      node_set_online(node);
>>      numa_set_node(cpu, node);
>>
>> -    if ( opt_cpu_info && acpi_numa > 0 )
>> +    if ( opt_cpu_info && get_acpi_numa() )
>>          printk("CPU %d APIC %d -> Node %d\n", cpu, apicid, node);
>>  }
>>
>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>> index 2d0c047..ccacbcd 100644
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -153,7 +153,7 @@ static void __init bad_srat(void)
>>  {
>>         int i;
>>         printk(KERN_ERR "SRAT: SRAT not used.\n");
>> -       acpi_numa = -1;
>> +       set_acpi_numa(0);
>>         for (i = 0; i < MAX_LOCAL_APIC; i++)
>>                 apicid_to_node[i] = NUMA_NO_NODE;
>>         for (i = 0; i < ARRAY_SIZE(pxm2node); i++)
>> @@ -232,7 +232,7 @@ acpi_numa_x2apic_affinity_init(const struct
>> acpi_srat_x2apic_cpu_affinity *pa)
>>
>>         apicid_to_node[pa->apic_id] = node;
>>         node_set(node, processor_nodes_parsed);
>> -       acpi_numa = 1;
>> +       set_acpi_numa(1);
>>         printk(KERN_INFO "SRAT: PXM %u -> APIC %08x -> Node %u\n",
>>                pxm, pa->apic_id, node);
>>  }
>> @@ -265,7 +265,7 @@ acpi_numa_processor_affinity_init(const struct
>> acpi_srat_cpu_affinity *pa)
>>         }
>>         apicid_to_node[pa->apic_id] = node;
>>         node_set(node, processor_nodes_parsed);
>> -       acpi_numa = 1;
>> +       set_acpi_numa(1);
>>         printk(KERN_INFO "SRAT: PXM %u -> APIC %02x -> Node %u\n",
>>                pxm, pa->apic_id, node);
>>  }
>> @@ -418,7 +418,7 @@ static int __init srat_parse_region(struct
>> acpi_subtable_header *header,
>>             (ma->flags & ACPI_SRAT_MEM_NON_VOLATILE))
>>                 return 0;
>>
>> -       if (numa_off)
>> +       if (is_numa_off())
>>                 printk(KERN_INFO "SRAT: %013"PRIx64"-%013"PRIx64"\n",
>>                        ma->base_address, ma->base_address + ma->length -
>> 1);
>>
>> @@ -433,7 +433,7 @@ void __init srat_parse_regions(uint64_t addr)
>>         uint64_t mask;
>>         unsigned int i;
>>
>> -       if (acpi_disabled || acpi_numa < 0 ||
>> +       if (acpi_disabled || (get_acpi_numa() == 0) ||
>>             acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
>>                 return;
>>
>> @@ -462,7 +462,7 @@ int __init acpi_scan_nodes(uint64_t start, uint64_t
>> end)
>>         for (i = 0; i < MAX_NUMNODES; i++)
>>                 cutoff_node(i, start, end);
>>
>> -       if (acpi_numa <= 0)
>> +       if (get_acpi_numa() == 0)
>>                 return -1;
>>
>>         if (!nodes_cover_memory()) {
>> diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
>> index a766688..9298d42 100644
>> --- a/xen/include/asm-x86/acpi.h
>> +++ b/xen/include/asm-x86/acpi.h
>> @@ -103,7 +103,6 @@ extern void acpi_reserve_bootmem(void);
>>
>>  #define ARCH_HAS_POWER_INIT    1
>>
>> -extern s8 acpi_numa;
>>  extern int acpi_scan_nodes(u64 start, u64 end);
>>  #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
>>
>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>> index bb22bff..ae5768b 100644
>> --- a/xen/include/asm-x86/numa.h
>> +++ b/xen/include/asm-x86/numa.h
>> @@ -30,10 +30,7 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
>>
>>  extern void numa_add_cpu(int cpu);
>>  extern void numa_init_array(void);
>> -extern bool_t numa_off;
>> -
>> -
>> -extern int srat_disabled(void);
>> +extern bool srat_disabled(void);
>>  extern void numa_set_node(int cpu, nodeid_t node);
>>  extern nodeid_t setup_node(unsigned int pxm);
>>  extern void srat_detect_node(int cpu);
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 7aef1a8..7f6d090 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -18,4 +18,7 @@
>>    (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
>>     ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
>>
>> +bool is_numa_off(void);
>> +bool get_acpi_numa(void);
>> +void set_acpi_numa(bool val);
>
>
> I am not sure to understand why you add those helpers directly here in
> xen/numa.h. IHMO, This should belong to the moving code patches.

To have code moving patches doing only code movement, I have exported
in the patch it is defined.

>
>
>>  #endif /* _XEN_NUMA_H */
>>
>
> --
> Julien Grall

Julien Grall April 25, 2017, 12:04 p.m. UTC | #3

Hello Vijay,

On 25/04/17 07:54, Vijay Kilari wrote:
> On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <[e-mail]> wrote:
>> Hi Vijay,
>>
>> On 28/03/17 16:53, [e-mail] wrote:
>>>
>>> From: Vijaya Kumar K <[e-mail]>
>>>
>>> Add accessor functions for acpi_numa and numa_off static
>>> variables. Init value of acpi_numa is set 1 instead of 0.
>>
>>
>> Please explain why you change the acpi_numa value from 0 to 1.
>
> previously acpi_numa was s8 and are using 0 and -1 values. I have made
> it bool and set
> the initial value to 1.

Are you sure? With a quick grep I spot it sounds like acpi_numa can have 
the value 0, -1, 1.

>
> By setting 1, we are enabling acpi_numa by default. If not enabled, the below
> call has check srat_disabled() before proceeding fails.

My understanding is on x86 acpi_numa is disabled by default and will be 
enabled if they are able to parse the SRAT. So why are you changing the 
behavior for x86?

>
> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
> {
> ....
>
>     if ( srat_disabled() )
>         return;
>
> }
>
>>
>> Also, I am not sure to understand the benefits of those helpers. Why do you
>> need them? Why not using the global variable directly, this will avoid to
>> call a function just for returning a value...
>
> These helpers are used by both common code and arch specific code later.
> Hence made these global variables as static and added helpers

The variables were global so they could already be used anywhere.

Furthermore, those helpers are not even inlined, so everytime you want 
to access read acpi_numa you have to do a branch then a memory access.

So what is the point to do that?

>>> diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
>>> index a766688..9298d42 100644
>>> --- a/xen/include/asm-x86/acpi.h
>>> +++ b/xen/include/asm-x86/acpi.h
>>> @@ -103,7 +103,6 @@ extern void acpi_reserve_bootmem(void);
>>>
>>>  #define ARCH_HAS_POWER_INIT    1
>>>
>>> -extern s8 acpi_numa;
>>>  extern int acpi_scan_nodes(u64 start, u64 end);
>>>  #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
>>>
>>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>>> index bb22bff..ae5768b 100644
>>> --- a/xen/include/asm-x86/numa.h
>>> +++ b/xen/include/asm-x86/numa.h
>>> @@ -30,10 +30,7 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
>>>
>>>  extern void numa_add_cpu(int cpu);
>>>  extern void numa_init_array(void);
>>> -extern bool_t numa_off;
>>> -
>>> -
>>> -extern int srat_disabled(void);
>>> +extern bool srat_disabled(void);
>>>  extern void numa_set_node(int cpu, nodeid_t node);
>>>  extern nodeid_t setup_node(unsigned int pxm);
>>>  extern void srat_detect_node(int cpu);
>>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>>> index 7aef1a8..7f6d090 100644
>>> --- a/xen/include/xen/numa.h
>>> +++ b/xen/include/xen/numa.h
>>> @@ -18,4 +18,7 @@
>>>    (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
>>>     ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
>>>
>>> +bool is_numa_off(void);
>>> +bool get_acpi_numa(void);
>>> +void set_acpi_numa(bool val);
>>
>>
>> I am not sure to understand why you add those helpers directly here in
>> xen/numa.h. IHMO, This should belong to the moving code patches.
>
> To have code moving patches doing only code movement, I have exported
> in the patch it is defined.

Sorry but what are prototypes if not code?

The point of moving the prototypes in the patch which move the actual 
declarations is helping the reviewers to check if you correctly moved 
everything.

>
>>
>>
>>>  #endif /* _XEN_NUMA_H */
>>>
>>
>> --
>> Julien Grall

Vijay Kilari April 25, 2017, 12:20 p.m. UTC | #4

On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall <[e-mail]> wrote:
> Hello Vijay,
>
> On 25/04/17 07:54, Vijay Kilari wrote:
>>
>> On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <[e-mail]>
>> wrote:
>>>
>>> Hi Vijay,
>>>
>>> On 28/03/17 16:53, [e-mail] wrote:
>>>>
>>>>
>>>> From: Vijaya Kumar K <[e-mail]>
>>>>
>>>> Add accessor functions for acpi_numa and numa_off static
>>>> variables. Init value of acpi_numa is set 1 instead of 0.
>>>
>>>
>>>
>>> Please explain why you change the acpi_numa value from 0 to 1.
>>
>>
>> previously acpi_numa was s8 and are using 0 and -1 values. I have made
>> it bool and set
>> the initial value to 1.
>
>
> Are you sure? With a quick grep I spot it sounds like acpi_numa can have the
> value 0, -1, 1.
>

Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use of
having 3 values to say enable or disable.

>>
>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>> below
>> call has check srat_disabled() before proceeding fails.
>
>
> My understanding is on x86 acpi_numa is disabled by default and will be
> enabled if they are able to parse the SRAT. So why are you changing the
> behavior for x86?

acpi_numa = 0 means it is enabled by default on x86.

>
>>
>> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>> {
>> ....
>>
>>     if ( srat_disabled() )
>>         return;
>>
>> }
>>
>>>
>>> Also, I am not sure to understand the benefits of those helpers. Why do
>>> you
>>> need them? Why not using the global variable directly, this will avoid to
>>> call a function just for returning a value...
>>
>>
>> These helpers are used by both common code and arch specific code later.
>> Hence made these global variables as static and added helpers
>
>
> The variables were global so they could already be used anywhere.
>
> Furthermore, those helpers are not even inlined, so everytime you want to
> access read acpi_numa you have to do a branch then a memory access.
>
> So what is the point to do that?

I agree with making inline. But I don't think there is any harm in making them
static and adding helpers.

>
>
>>>> diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
>>>> index a766688..9298d42 100644
>>>> --- a/xen/include/asm-x86/acpi.h
>>>> +++ b/xen/include/asm-x86/acpi.h
>>>> @@ -103,7 +103,6 @@ extern void acpi_reserve_bootmem(void);
>>>>
>>>>  #define ARCH_HAS_POWER_INIT    1
>>>>
>>>> -extern s8 acpi_numa;
>>>>  extern int acpi_scan_nodes(u64 start, u64 end);
>>>>  #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
>>>>
>>>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>>>> index bb22bff..ae5768b 100644
>>>> --- a/xen/include/asm-x86/numa.h
>>>> +++ b/xen/include/asm-x86/numa.h
>>>> @@ -30,10 +30,7 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
>>>>
>>>>  extern void numa_add_cpu(int cpu);
>>>>  extern void numa_init_array(void);
>>>> -extern bool_t numa_off;
>>>> -
>>>> -
>>>> -extern int srat_disabled(void);
>>>> +extern bool srat_disabled(void);
>>>>  extern void numa_set_node(int cpu, nodeid_t node);
>>>>  extern nodeid_t setup_node(unsigned int pxm);
>>>>  extern void srat_detect_node(int cpu);
>>>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>>>> index 7aef1a8..7f6d090 100644
>>>> --- a/xen/include/xen/numa.h
>>>> +++ b/xen/include/xen/numa.h
>>>> @@ -18,4 +18,7 @@
>>>>    (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
>>>>     ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
>>>>
>>>> +bool is_numa_off(void);
>>>> +bool get_acpi_numa(void);
>>>> +void set_acpi_numa(bool val);
>>>
>>>
>>>
>>> I am not sure to understand why you add those helpers directly here in
>>> xen/numa.h. IHMO, This should belong to the moving code patches.
>>
>>
>> To have code moving patches doing only code movement, I have exported
>> in the patch it is defined.
>
>
> Sorry but what are prototypes if not code?
>
> The point of moving the prototypes in the patch which move the actual
> declarations is helping the reviewers to check if you correctly moved
> everything.

I am ok if it helps in review.

>
>
>>
>>>
>>>
>>>>  #endif /* _XEN_NUMA_H */
>>>>
>>>
>>> --
>>> Julien Grall
>
>
> --
> Julien Grall

Julien Grall April 25, 2017, 12:28 p.m. UTC | #5

On 25/04/17 13:20, Vijay Kilari wrote:
> On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall <[e-mail]> wrote:
>> Hello Vijay,
>>
>> On 25/04/17 07:54, Vijay Kilari wrote:
>>>
>>> On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <[e-mail]>
>>> wrote:
>>>>
>>>> Hi Vijay,
>>>>
>>>> On 28/03/17 16:53, [e-mail] wrote:
>>>>>
>>>>>
>>>>> From: Vijaya Kumar K <[e-mail]>
>>>>>
>>>>> Add accessor functions for acpi_numa and numa_off static
>>>>> variables. Init value of acpi_numa is set 1 instead of 0.
>>>>
>>>>
>>>>
>>>> Please explain why you change the acpi_numa value from 0 to 1.
>>>
>>>
>>> previously acpi_numa was s8 and are using 0 and -1 values. I have made
>>> it bool and set
>>> the initial value to 1.
>>
>>
>> Are you sure? With a quick grep I spot it sounds like acpi_numa can have the
>> value 0, -1, 1.
>>
>
> Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use of
> having 3 values to say enable or disable.

Then explain why in the commit message and don't let people discover. If 
you have not done it, I would recommend to read:

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

>
>>>
>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>> below
>>> call has check srat_disabled() before proceeding fails.
>>
>>
>> My understanding is on x86 acpi_numa is disabled by default and will be
>> enabled if they are able to parse the SRAT. So why are you changing the
>> behavior for x86?
>
> acpi_numa = 0 means it is enabled by default on x86.

In acpi_scan_nodes:

if (acpi_numa <= 0)
   return -1;

So it does not seem that 0 means enabled.

>
>>
>>>
>>> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>>> {
>>> ....
>>>
>>>     if ( srat_disabled() )
>>>         return;
>>>
>>> }
>>>
>>>>
>>>> Also, I am not sure to understand the benefits of those helpers. Why do
>>>> you
>>>> need them? Why not using the global variable directly, this will avoid to
>>>> call a function just for returning a value...
>>>
>>>
>>> These helpers are used by both common code and arch specific code later.
>>> Hence made these global variables as static and added helpers
>>
>>
>> The variables were global so they could already be used anywhere.
>>
>> Furthermore, those helpers are not even inlined, so everytime you want to
>> access read acpi_numa you have to do a branch then a memory access.
>>
>> So what is the point to do that?
>
> I agree with making inline. But I don't think there is any harm in making them
> static and adding helpers.

But why? Why don't you keep the code as it is? You modify code without 
any justification and not for the better.

Cheers,

Vijay Kilari April 25, 2017, 2:54 p.m. UTC | #6

On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall <[e-mail]> wrote:
>
>
> On 25/04/17 13:20, Vijay Kilari wrote:
>>
>> On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall <[e-mail]>
>> wrote:
>>>
>>> Hello Vijay,
>>>
>>> On 25/04/17 07:54, Vijay Kilari wrote:
>>>>
>>>>
>>>> On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <[e-mail]>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi Vijay,
>>>>>
>>>>> On 28/03/17 16:53, [e-mail] wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> From: Vijaya Kumar K <[e-mail]>
>>>>>>
>>>>>> Add accessor functions for acpi_numa and numa_off static
>>>>>> variables. Init value of acpi_numa is set 1 instead of 0.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Please explain why you change the acpi_numa value from 0 to 1.
>>>>
>>>>
>>>>
>>>> previously acpi_numa was s8 and are using 0 and -1 values. I have made
>>>> it bool and set
>>>> the initial value to 1.
>>>
>>>
>>>
>>> Are you sure? With a quick grep I spot it sounds like acpi_numa can have
>>> the
>>> value 0, -1, 1.
>>>
>>
>> Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use
>> of
>> having 3 values to say enable or disable.
>
>
> Then explain why in the commit message and don't let people discover. If you
> have not done it, I would recommend to read:
>
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
>
>>
>>>>
>>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>>> below
>>>> call has check srat_disabled() before proceeding fails.
>>>
>>>
>>>
>>> My understanding is on x86 acpi_numa is disabled by default and will be
>>> enabled if they are able to parse the SRAT. So why are you changing the
>>> behavior for x86?
>>
>>
>> acpi_numa = 0 means it is enabled by default on x86.
>
>
> In acpi_scan_nodes:
>
> if (acpi_numa <= 0)
>   return -1;
>
> So it does not seem that 0 means enabled.

IMO, In x86
         -1 means disabled
          0 enabled but not numa initialized
          1 enabled and numa initialized.

I clubbed 0 & 1.

I was referring to below code in x86 where in acpi_numa = 0 means
srat_disabled() returns false. Which means acpi_numa is enabled implicitly.

int srat_disabled(void)
{
      return numa_off || acpi_numa < 0;
}

When I changed acpi_numa to bool, it is initialized to 1 and changed
below code.

bool srat_disabled(void)
{
    return numa_off || acpi_numa == 0;
}

Also this srat_disabed() is used in acpi_numa_memory_affinity_init which is
called from acpi_numa_init() before calling acpi_scan_nodes().

>
>>
>>>
>>>>
>>>> acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>>>> {
>>>> ....
>>>>
>>>>     if ( srat_disabled() )
>>>>         return;
>>>>
>>>> }
>>>>
>>>>>
>>>>> Also, I am not sure to understand the benefits of those helpers. Why do
>>>>> you
>>>>> need them? Why not using the global variable directly, this will avoid
>>>>> to
>>>>> call a function just for returning a value...
>>>>
>>>>
>>>>
>>>> These helpers are used by both common code and arch specific code later.
>>>> Hence made these global variables as static and added helpers
>>>
>>>
>>>
>>> The variables were global so they could already be used anywhere.
>>>
>>> Furthermore, those helpers are not even inlined, so everytime you want to
>>> access read acpi_numa you have to do a branch then a memory access.
>>>
>>> So what is the point to do that?
>>
>>
>> I agree with making inline. But I don't think there is any harm in making
>> them
>> static and adding helpers.
>
>
> But why? Why don't you keep the code as it is? You modify code without any
> justification and not for the better.
>
> Cheers,
>
> --
> Julien Grall

Julien Grall April 25, 2017, 3:14 p.m. UTC | #7

On 25/04/17 15:54, Vijay Kilari wrote:
> On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall <[e-mail]> wrote:
>>>>>
>>>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>>>> below
>>>>> call has check srat_disabled() before proceeding fails.
>>>>
>>>>
>>>>
>>>> My understanding is on x86 acpi_numa is disabled by default and will be
>>>> enabled if they are able to parse the SRAT. So why are you changing the
>>>> behavior for x86?
>>>
>>>
>>> acpi_numa = 0 means it is enabled by default on x86.
>>
>>
>> In acpi_scan_nodes:
>>
>> if (acpi_numa <= 0)
>>   return -1;
>>
>> So it does not seem that 0 means enabled.
>
> IMO, In x86
>          -1 means disabled
>           0 enabled but not numa initialized
>           1 enabled and numa initialized.
>
> I clubbed 0 & 1.

 From your description 0 and 1 have different meaning, so I don't see 
how you can merge them that easily without any explanation.

Anyway, I will leave x86 maintainers give their opinion here.

>
> I was referring to below code in x86 where in acpi_numa = 0 means
> srat_disabled() returns false. Which means acpi_numa is enabled implicitly.
>
> int srat_disabled(void)
> {
>       return numa_off || acpi_numa < 0;
> }
>
> When I changed acpi_numa to bool, it is initialized to 1 and changed
> below code.
>
> bool srat_disabled(void)
> {
>     return numa_off || acpi_numa == 0;
> }
>
> Also this srat_disabed() is used in acpi_numa_memory_affinity_init which is
> called from acpi_numa_init() before calling acpi_scan_nodes().

Jan Beulich April 25, 2017, 3:43 p.m. UTC | #8

>>> On 25.04.17 at 17:14, <[e-mail]> wrote:
> On 25/04/17 15:54, Vijay Kilari wrote:
>> On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall <[e-mail]> wrote:
>>>>>>
>>>>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>>>>> below
>>>>>> call has check srat_disabled() before proceeding fails.
>>>>>
>>>>>
>>>>>
>>>>> My understanding is on x86 acpi_numa is disabled by default and will be
>>>>> enabled if they are able to parse the SRAT. So why are you changing the
>>>>> behavior for x86?
>>>>
>>>>
>>>> acpi_numa = 0 means it is enabled by default on x86.
>>>
>>>
>>> In acpi_scan_nodes:
>>>
>>> if (acpi_numa <= 0)
>>>   return -1;
>>>
>>> So it does not seem that 0 means enabled.
>>
>> IMO, In x86
>>          -1 means disabled
>>           0 enabled but not numa initialized
>>           1 enabled and numa initialized.
>>
>> I clubbed 0 & 1.
> 
>  From your description 0 and 1 have different meaning, so I don't see 
> how you can merge them that easily without any explanation.
> 
> Anyway, I will leave x86 maintainers give their opinion here.

I'm pretty certain this needs to remain a tristate.

Jan

Vijay Kilari May 2, 2017, 9:47 a.m. UTC | #9

On Tue, Apr 25, 2017 at 9:13 PM, Jan Beulich <[e-mail]> wrote:
>>>> On 25.04.17 at 17:14, <[e-mail]> wrote:
>> On 25/04/17 15:54, Vijay Kilari wrote:
>>> On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall <[e-mail]> wrote:
>>>>>>>
>>>>>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>>>>>> below
>>>>>>> call has check srat_disabled() before proceeding fails.
>>>>>>
>>>>>>
>>>>>>
>>>>>> My understanding is on x86 acpi_numa is disabled by default and will be
>>>>>> enabled if they are able to parse the SRAT. So why are you changing the
>>>>>> behavior for x86?
>>>>>
>>>>>
>>>>> acpi_numa = 0 means it is enabled by default on x86.
>>>>
>>>>
>>>> In acpi_scan_nodes:
>>>>
>>>> if (acpi_numa <= 0)
>>>>   return -1;
>>>>
>>>> So it does not seem that 0 means enabled.
>>>
>>> IMO, In x86
>>>          -1 means disabled
>>>           0 enabled but not numa initialized
>>>           1 enabled and numa initialized.
>>>
>>> I clubbed 0 & 1.
>>
>>  From your description 0 and 1 have different meaning, so I don't see
>> how you can merge them that easily without any explanation.
>>
>> Anyway, I will leave x86 maintainers give their opinion here.
>
> I'm pretty certain this needs to remain a tristate.

Ok. I will drop this patch from this series and can be fixed
outside this series.
BTW, any review comments on remaining patches?

Jan Beulich May 2, 2017, 9:54 a.m. UTC | #10

>>> On 02.05.17 at 11:47, <[e-mail]> wrote:
> BTW, any review comments on remaining patches?

I didn't even manage to get to the start of the flood of RFCs
posted during the last 1.5 months (which priority wise all sit behind
the various non-RFC postings), so I can't predict when I'll get to
yours. Also please remember that the focus right now is to make
4.9 good quality ...

Jan

Julien Grall May 8, 2017, 5:38 p.m. UTC | #11

Hi Vijay,

On 02/05/17 10:47, Vijay Kilari wrote:
> On Tue, Apr 25, 2017 at 9:13 PM, Jan Beulich <[e-mail]> wrote:
>>>>> On 25.04.17 at 17:14, <[e-mail]> wrote:
>>> On 25/04/17 15:54, Vijay Kilari wrote:
>>>> On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>>>>>>> below
>>>>>>>> call has check srat_disabled() before proceeding fails.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> My understanding is on x86 acpi_numa is disabled by default and will be
>>>>>>> enabled if they are able to parse the SRAT. So why are you changing the
>>>>>>> behavior for x86?
>>>>>>
>>>>>>
>>>>>> acpi_numa = 0 means it is enabled by default on x86.
>>>>>
>>>>>
>>>>> In acpi_scan_nodes:
>>>>>
>>>>> if (acpi_numa <= 0)
>>>>>   return -1;
>>>>>
>>>>> So it does not seem that 0 means enabled.
>>>>
>>>> IMO, In x86
>>>>          -1 means disabled
>>>>           0 enabled but not numa initialized
>>>>           1 enabled and numa initialized.
>>>>
>>>> I clubbed 0 & 1.
>>>
>>>  From your description 0 and 1 have different meaning, so I don't see
>>> how you can merge them that easily without any explanation.
>>>
>>> Anyway, I will leave x86 maintainers give their opinion here.
>>
>> I'm pretty certain this needs to remain a tristate.
>
> Ok. I will drop this patch from this series and can be fixed
> outside this series.
> BTW, any review comments on remaining patches?

I had a looked at the series and decided to stop reviewing it because 
comments are not addressed.

I am not going to review anything until *all* the comments from previous 
version are addressed. I would recommend the other to do the same.

Cheers,

Jan Beulich June 30, 2017, 2:07 p.m. UTC | #12

>>> <[e-mail]> 03/28/17 5:55 PM >>>
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -42,12 +42,27 @@ cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>  
>  nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
>  
> -bool numa_off = 0;
> -s8 acpi_numa = 0;
> +static bool numa_off = 0;
> +static bool acpi_numa = 1;
>  
> -int srat_disabled(void)
> +bool is_numa_off(void)

numa_enabled() (or less desirably numa_disabled())

> +bool get_acpi_numa(void)

acpi_numa_enabled() then perhaps.

Iirc Julien has already commented on the non-boolean nature of acpi_numa.

> @@ -202,13 +217,17 @@ void __init numa_init_array(void)
>  
>  #ifdef CONFIG_NUMA_EMU
>  static int __initdata numa_fake = 0;
> +static int get_numa_fake(void)
> +{
> +    return numa_fake;
> +}

I don't see the point of having static accessors for static variables. Even
if the accessor became non-static, I'd expect it to be used only in other
translation units.

Jan

Patch

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 964fc5a..6b794a7 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -42,12 +42,27 @@  cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
 
 nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
 
-bool numa_off = 0;
-s8 acpi_numa = 0;
+static bool numa_off = 0;
+static bool acpi_numa = 1;
 
-int srat_disabled(void)
+bool is_numa_off(void)
 {
-    return numa_off || acpi_numa < 0;
+    return numa_off;
+}
+
+bool get_acpi_numa(void)
+{
+    return acpi_numa;
+}
+
+void set_acpi_numa(bool_t val)
+{
+    acpi_numa = val;
+}
+
+bool srat_disabled(void)
+{
+    return numa_off || acpi_numa == 0;
 }
 
 /*
@@ -202,13 +217,17 @@  void __init numa_init_array(void)
 
 #ifdef CONFIG_NUMA_EMU
 static int __initdata numa_fake = 0;
+static int get_numa_fake(void)
+{
+    return numa_fake;
+}
 
 /* Numa emulation */
 static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
 {
     int i;
     struct node nodes[MAX_NUMNODES];
-    uint64_t sz = ((end_pfn - start_pfn) << PAGE_SHIFT) / numa_fake;
+    uint64_t sz = ((end_pfn - start_pfn) << PAGE_SHIFT) / get_numa_fake();
 
     /* Kludge needed for the hash function */
     if ( hweight64(sz) > 1 )
@@ -223,10 +242,10 @@  static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
     }
 
     memset(&nodes,0,sizeof(nodes));
-    for ( i = 0; i < numa_fake; i++ )
+    for ( i = 0; i < get_numa_fake(); i++ )
     {
         nodes[i].start = (start_pfn << PAGE_SHIFT) + i * sz;
-        if ( i == numa_fake - 1 )
+        if ( i == get_numa_fake() - 1 )
             sz = (end_pfn << PAGE_SHIFT) - nodes[i].start;
         nodes[i].end = nodes[i].start + sz;
         printk(KERN_INFO
@@ -235,7 +254,7 @@  static int __init numa_emulation(uint64_t start_pfn, uint64_t end_pfn)
                (nodes[i].end - nodes[i].start) >> 20);
         node_set_online(i);
     }
-    if ( compute_memnode_shift(nodes, numa_fake, NULL, &memnode_shift) )
+    if ( compute_memnode_shift(nodes, get_numa_fake(), NULL, &memnode_shift) )
     {
         memnode_shift = 0;
         printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
@@ -254,18 +273,18 @@  void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn)
     int i;
 
 #ifdef CONFIG_NUMA_EMU
-    if ( numa_fake && !numa_emulation(start_pfn, end_pfn) )
+    if ( get_numa_fake() && !numa_emulation(start_pfn, end_pfn) )
         return;
 #endif
 
 #ifdef CONFIG_ACPI_NUMA
-    if ( !numa_off && !acpi_scan_nodes((uint64_t)start_pfn << PAGE_SHIFT,
+    if ( !is_numa_off() && !acpi_scan_nodes((uint64_t)start_pfn << PAGE_SHIFT,
          (uint64_t)end_pfn << PAGE_SHIFT) )
         return;
 #endif
 
     printk(KERN_INFO "%s\n",
-           numa_off ? "NUMA turned off" : "No NUMA configuration found");
+           is_numa_off() ? "NUMA turned off" : "No NUMA configuration found");
 
     printk(KERN_INFO "Faking a node at %016"PRIx64"-%016"PRIx64"\n",
            (uint64_t)start_pfn << PAGE_SHIFT,
@@ -312,7 +331,7 @@  static int __init numa_setup(char *opt)
     if ( !strncmp(opt,"noacpi",6) )
     {
         numa_off = 0;
-        acpi_numa = -1;
+        acpi_numa = 0;
     }
 #endif
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1cd290e..4410e53 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -241,7 +241,7 @@  void srat_detect_node(int cpu)
     node_set_online(node);
     numa_set_node(cpu, node);
 
-    if ( opt_cpu_info && acpi_numa > 0 )
+    if ( opt_cpu_info && get_acpi_numa() )
         printk("CPU %d APIC %d -> Node %d\n", cpu, apicid, node);
 }
 
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 2d0c047..ccacbcd 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -153,7 +153,7 @@  static void __init bad_srat(void)
 {
 	int i;
 	printk(KERN_ERR "SRAT: SRAT not used.\n");
-	acpi_numa = -1;
+	set_acpi_numa(0);
 	for (i = 0; i < MAX_LOCAL_APIC; i++)
 		apicid_to_node[i] = NUMA_NO_NODE;
 	for (i = 0; i < ARRAY_SIZE(pxm2node); i++)
@@ -232,7 +232,7 @@  acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *pa)
 
 	apicid_to_node[pa->apic_id] = node;
 	node_set(node, processor_nodes_parsed);
-	acpi_numa = 1;
+	set_acpi_numa(1);
 	printk(KERN_INFO "SRAT: PXM %u -> APIC %08x -> Node %u\n",
 	       pxm, pa->apic_id, node);
 }
@@ -265,7 +265,7 @@  acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *pa)
 	}
 	apicid_to_node[pa->apic_id] = node;
 	node_set(node, processor_nodes_parsed);
-	acpi_numa = 1;
+	set_acpi_numa(1);
 	printk(KERN_INFO "SRAT: PXM %u -> APIC %02x -> Node %u\n",
 	       pxm, pa->apic_id, node);
 }
@@ -418,7 +418,7 @@  static int __init srat_parse_region(struct acpi_subtable_header *header,
 	    (ma->flags & ACPI_SRAT_MEM_NON_VOLATILE))
 		return 0;
 
-	if (numa_off)
+	if (is_numa_off())
 		printk(KERN_INFO "SRAT: %013"PRIx64"-%013"PRIx64"\n",
 		       ma->base_address, ma->base_address + ma->length - 1);
 
@@ -433,7 +433,7 @@  void __init srat_parse_regions(uint64_t addr)
 	uint64_t mask;
 	unsigned int i;
 
-	if (acpi_disabled || acpi_numa < 0 ||
+	if (acpi_disabled || (get_acpi_numa() == 0) ||
 	    acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
 		return;
 
@@ -462,7 +462,7 @@  int __init acpi_scan_nodes(uint64_t start, uint64_t end)
 	for (i = 0; i < MAX_NUMNODES; i++)
 		cutoff_node(i, start, end);
 
-	if (acpi_numa <= 0)
+	if (get_acpi_numa() == 0)
 		return -1;
 
 	if (!nodes_cover_memory()) {
diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
index a766688..9298d42 100644
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -103,7 +103,6 @@  extern void acpi_reserve_bootmem(void);
 
 #define ARCH_HAS_POWER_INIT	1
 
-extern s8 acpi_numa;
 extern int acpi_scan_nodes(u64 start, u64 end);
 #define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
 
diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index bb22bff..ae5768b 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -30,10 +30,7 @@  extern nodeid_t pxm_to_node(unsigned int pxm);
 
 extern void numa_add_cpu(int cpu);
 extern void numa_init_array(void);
-extern bool_t numa_off;
-
-
-extern int srat_disabled(void);
+extern bool srat_disabled(void);
 extern void numa_set_node(int cpu, nodeid_t node);
 extern nodeid_t setup_node(unsigned int pxm);
 extern void srat_detect_node(int cpu);
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 7aef1a8..7f6d090 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -18,4 +18,7 @@ 
   (((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
    ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
 
+bool is_numa_off(void);
+bool get_acpi_numa(void);
+void set_acpi_numa(bool val);
 #endif /* _XEN_NUMA_H */