Re: [Xen-devel] [RFC PATCH v2 11/25] x86: NUMA: Move common code from srat.c

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

 and

On Mon, May 8, 2017 at 10:36 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>
>>
>> Move code from xen/arch/x86/srat.c to xen/common/numa.c
>> so that it can be used by other archs.
>> Few generic static functions in x86/srat.c are made
>> non-static common/numa.c
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/srat.c        | 152
>> ++-------------------------------------------
>>  xen/common/numa.c          | 146
>> +++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-x86/acpi.h |   3 -
>>  xen/include/asm-x86/numa.h |   2 -
>>  xen/include/xen/numa.h     |  14 +++++
>>  5 files changed, 164 insertions(+), 153 deletions(-)
>>
>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>> index 2cc87a3..55947bb 100644
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -23,9 +23,8 @@
>>
>>  static struct acpi_table_slit *__read_mostly acpi_slit;
>>
>> -static nodemask_t __initdata memory_nodes_parsed;
>> -static nodemask_t __initdata processor_nodes_parsed;
>> -static struct node __initdata nodes[MAX_NUMNODES];
>> +extern nodemask_t processor_nodes_parsed;
>> +extern nodemask_t memory_nodes_parsed;
>
>
> On v1, Jan clearly NAK to changes like this. Declarations belong in header
> files. It is a different variable compare to v1, but I would have expected
> you to apply what he said everywhere...

Ok I will move these to header files.

One more change that I made is moved from static to global.
because creating accessor functions around these nodesmask_t is tricky because
the macros (defined in nodemask.h) does not take pointer parameters.

I will add comment.

>
> [...]
>
>> diff --git a/xen/common/numa.c b/xen/common/numa.c
>> index 207ebd8..1789bba 100644
>> --- a/xen/common/numa.c
>> +++ b/xen/common/numa.c
>> @@ -32,6 +32,8 @@
>>  static int numa_setup(char *s);
>>  custom_param("numa", numa_setup);
>>
>> +nodemask_t __initdata memory_nodes_parsed;
>> +nodemask_t __initdata processor_nodes_parsed;
>>  struct node_data node_data[MAX_NUMNODES];
>>
>>  /* Mapping from pdx to node id */
>> @@ -47,6 +49,10 @@ cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>>
>>  static bool numa_off = 0;
>>  static bool acpi_numa = 1;
>> +static int num_node_memblks;
>> +static struct node node_memblk_range[NR_NODE_MEMBLKS];
>> +static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>> +static struct node __initdata nodes[MAX_NUMNODES];
>
>
> It would make sense to keep those variables together with
> {memory,processor}_nodes_parsed.

ok
>
> [...]
>
>> +int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < get_num_node_memblks(); i++) {
>
>
> common/numa.c is using Xen coding style whilst arch/x86/srat.c is using
> Linux coding style.
>
> You decided to validly switch to soft tab, making quite difficult to check
> if this patch is only code movement. But you did not go far enough and fix
> the coding style of the code moved.
>
> Please do it properly and not half of it. For simplicity I would be OK that
> it is done in this patch. But this needs to be clearly written in the commit
> message.

I will add in commit message about coding style changes to destination file
compared to source file.

>
>> +        struct node *nd = get_node_memblk_range(i);
>> +
>> +        if (nd->start <= start && nd->end > end &&
>> +            get_memblk_nodeid(i) == node)
>> +            return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>
>
> [...]
>
>
>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>> index 421e8b7..7cff220 100644
>> --- a/xen/include/asm-x86/numa.h
>> +++ b/xen/include/asm-x86/numa.h
>> @@ -47,8 +47,6 @@ static inline __attribute__((pure)) nodeid_t
>> phys_to_nid(paddr_t addr)
>>  #define node_end_pfn(nid)       (NODE_DATA(nid)->node_start_pfn + \
>>                                   NODE_DATA(nid)->node_spanned_pages)
>>
>> -extern int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
>> -
>>  void srat_parse_regions(uint64_t addr);
>>  extern uint8_t __node_distance(nodeid_t a, nodeid_t b);
>>  unsigned int arch_get_dma_bitsize(void);
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index eed40af..ee53526 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -13,6 +13,7 @@
>>  #define NUMA_NO_DISTANCE 0xFF
>>
>>  #define MAX_NUMNODES    (1 << NODES_SHIFT)
>> +#define NR_NODE_MEMBLKS (MAX_NUMNODES * 2)
>>
>>  struct node {
>>      paddr_t start;
>> @@ -28,6 +29,19 @@ 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);
>> +extern int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
>> +extern int conflicting_memblks(paddr_t start, paddr_t end);
>> +extern void cutoff_node(int i, paddr_t start, paddr_t end);
>> +extern struct node *get_numa_node(int id);
>> +extern nodeid_t get_memblk_nodeid(int memblk);
>> +extern nodeid_t *get_memblk_nodeid_map(void);
>> +extern struct node *get_node_memblk_range(int memblk);
>> +extern struct node *get_memblk(int memblk);
>> +extern int numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t
>> size);
>> +extern int get_num_node_memblks(void);
>> +extern int arch_sanitize_nodes_memory(void);
>> +extern void numa_failed(void);
>> +extern int numa_scan_nodes(uint64_t start, uint64_t end);
>
>
> See my comment on the previous patch.

I understand that we can drop this extern
>
>>
>>  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
>>
>>
>
> Cheers,
>
> --
> Julien Grall

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