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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Vijay,
>
> On 28/03/17 16:53, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> 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 <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>>  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

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