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

[Julien Grall <julien.grall@arm.com>]

Hi Vijay,

On 28/03/17 16:53, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> 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 <Vijaya.Kumar@cavium.com>
> ---
>  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 */
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel