Re: [Xen-devel] [RFC PATCH v2 10/25] x86: NUMA: Move numa code and make it generic

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, May 8, 2017 at 10:11 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Vijay,
>
>
> On 28/03/17 16:53, vijay.kilari@xxxxxxxxx wrote:
>>
>> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
>> index 3bdab9a..33c6806 100644
>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -10,286 +10,13 @@
>>  #include <xen/ctype.h>
>>  #include <xen/nodemask.h>
>>  #include <xen/numa.h>
>> -#include <xen/keyhandler.h>
>>  #include <xen/time.h>
>>  #include <xen/smp.h>
>>  #include <xen/pfn.h>
>>  #include <asm/acpi.h>
>> -#include <xen/sched.h>
>> -#include <xen/softirq.h>
>> -
>> -static int numa_setup(char *s);
>> -custom_param("numa", numa_setup);
>> -
>> -struct node_data node_data[MAX_NUMNODES];
>> -
>> -/* Mapping from pdx to node id */
>> -unsigned int memnode_shift;
>> -static typeof(*memnodemap) _memnodemap[64];
>> -unsigned long memnodemapsize;
>> -uint8_t *memnodemap;
>> -
>> -nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
>> -    [0 ... NR_CPUS-1] = NUMA_NO_NODE
>> -};
>> -/*
>> - * Keep BIOS's CPU2node information, should not be used for memory
>> allocaion
>> - */
>> -nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
>> -    [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
>> -};
>
>
> Why this is moved in this patch from here to x86/srat.c?

This is x86 specific. I will make a separate patch for this
move.

>
> [...]
>
>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>> index 7cf4771..2cc87a3 100644
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -27,6 +27,13 @@ static nodemask_t __initdata memory_nodes_parsed;
>>  static nodemask_t __initdata processor_nodes_parsed;
>>  static struct node __initdata nodes[MAX_NUMNODES];
>>
>> +/*
>> + * Keep BIOS's CPU2node information, should not be used for memory
>> allocaion
>> + */
>> +nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
>> +    [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
>> +};
>> +
>
>
> This does not belong to this patch...
Ok
>
>>  struct pxm2node {
>>         unsigned int pxm;
>>         nodeid_t node;
>
>
> [...]
>
>
>> diff --git a/xen/common/numa.c b/xen/common/numa.c
>> new file mode 100644
>> index 0000000..207ebd8
>> --- /dev/null
>> +++ b/xen/common/numa.c
>> @@ -0,0 +1,488 @@
>> +/*
>> + * Common NUMA handling functions for x86 and arm.
>> + * Original code extracted from arch/x86/numa.c
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms and conditions of the GNU General Public
>> + * License, version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/mm.h>
>> +#include <xen/string.h>
>> +#include <xen/init.h>
>> +#include <xen/ctype.h>
>> +#include <xen/nodemask.h>
>> +#include <xen/numa.h>
>> +#include <xen/keyhandler.h>
>> +#include <xen/time.h>
>> +#include <xen/smp.h>
>> +#include <xen/pfn.h>
>> +#include <asm/acpi.h>
>> +#include <xen/sched.h>
>> +#include <xen/softirq.h>
>
>
> Whilst you are moving this in a newfile, please order the includes.

I understand that you don't like any code changes in code movement
patch.

>
> [...]
>
>> +static unsigned int __init extract_lsb_from_nodes(const struct node
>> *nodes,
>> +                                                  int numnodes)
>> +{
>> +    unsigned int i, nodes_used = 0;
>> +    unsigned long spdx, epdx;
>> +    unsigned long bitfield = 0, memtop = 0;
>> +
>> +    for ( i = 0; i < numnodes; i++ )
>> +    {
>> +        spdx = paddr_to_pdx(nodes[i].start);
>> +        epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
>> +        if ( spdx >= epdx )
>> +            continue;
>> +        bitfield |= spdx;
>> +        nodes_used++;
>> +        if ( epdx > memtop )
>> +            memtop = epdx;
>> +    }
>> +    if ( nodes_used <= 1 )
>> +        i = BITS_PER_LONG - 1;
>> +    else
>> +        i = find_first_bit(&bitfield, sizeof(unsigned long) * 8);
>> +
>
>
> It is interesting to see that newline was added in the process of moving the
> code.

OK.
>
>> +    memnodemapsize = (memtop >> i) + 1;
>
>
> [....]
>
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 922fbd8..eed40af 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -14,6 +14,21 @@
>>
>>  #define MAX_NUMNODES    (1 << NODES_SHIFT)
>>
>> +struct node {
>> +    paddr_t start;
>> +    paddr_t end;
>> +};
>> +
>> +extern int compute_memnode_shift(struct node *nodes, int numnodes,
>> +                                 nodeid_t *nodeids, unsigned int *shift);
>> +extern void numa_init_array(void);
>> +extern bool_t srat_disabled(void);
>> +extern void numa_set_node(int cpu, nodeid_t node);
>> +extern nodeid_t acpi_setup_node(unsigned int pxm);
>> +extern void srat_detect_node(int cpu);
>> +extern void setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t
>> end);
>> +extern void init_cpu_to_node(void);
>
>
> Can you please be consistent with this file and drop the unecessary
> "extern".

I see all the externs are not required here. I will drop

>
>> +
>>  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
>>
>>  #define domain_to_node(d) \
>> @@ -23,4 +38,7 @@
>>  bool is_numa_off(void);
>>  bool get_acpi_numa(void);
>>  void set_acpi_numa(bool val);
>> +int get_numa_fake(void);
>> +extern int numa_emulation(uint64_t start_pfn, uint64_t end_pfn);
>> +extern void numa_dummy_init(uint64_t start_pfn, uint64_t end_pfn);
>
>
> Ditto.
>
>
>>  #endif /* _XEN_NUMA_H */
>>
>
> --
> Julien Grall

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