Re: [Xen-devel] [RFC PATCH v3 02/24] x86: NUMA: Clean up: Fix coding styles and drop unused code

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Jul 19, 2017 at 9:53 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Vijay,
>
> On 18/07/17 12:41, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> Fix coding style, trailing spaces, tabs in NUMA code.
>> Also drop unused macros and functions.
>> There is no functional change.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>> v3: - Change commit message
>>     - Changed VIRTUAL_BUG_ON to ASSERT
>
>
> Looking at the commit message you don't mention any renaming...
>
>>     - Dropped useless inner paranthesis for some macros
>
>
> [...]
>
>> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
>> index 3cf26c2..c0de57b 100644
>> --- a/xen/include/asm-x86/numa.h
>> +++ b/xen/include/asm-x86/numa.h
>> @@ -1,8 +1,11 @@
>> -#ifndef _ASM_X8664_NUMA_H
>> +#ifndef _ASM_X8664_NUMA_H
>>  #define _ASM_X8664_NUMA_H 1
>>
>>  #include <xen/cpumask.h>
>>
>> +#define MAX_NUMNODES    NR_NODES
>> +#define NR_NODE_MEMBLKS (MAX_NUMNODES * 2)
>
>
> I don't understand why this suddenly appears in the code when you moved away
> in patch #1 in xen/numa.h.

Particularly MAX_NUMNODES required by this header file with this
patch changes for compilation.
Though I can include xen/numa.h here but xen/numa.h is including
asm/numa.h back.

I will add separate patch for this defines movement and drop from
this patch.

>
> [...]
>
>
>> @@ -57,21 +55,23 @@ struct node_data {
>>
>>  extern struct node_data node_data[];
>>
>> -static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr)
>> -{
>> -       nodeid_t nid;
>> -       VIRTUAL_BUG_ON((paddr_to_pdx(addr) >> memnode_shift) >=
>> memnodemapsize);
>> -       nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift];
>> -       VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]);
>> -       return nid;
>> -}
>> -
>> -#define NODE_DATA(nid)         (&(node_data[nid]))
>> -
>> -#define node_start_pfn(nid)    (NODE_DATA(nid)->node_start_pfn)
>> -#define node_spanned_pages(nid)
>> (NODE_DATA(nid)->node_spanned_pages)
>> -#define node_end_pfn(nid)       (NODE_DATA(nid)->node_start_pfn + \
>> -                                NODE_DATA(nid)->node_spanned_pages)
>> +static inline __attribute_pure__ nodeid_t phys_to_nid(paddr_t addr)
>> +{
>> +   nodeid_t nid;
>> +
>> +   ASSERT((paddr_to_pdx(addr) >> memnode_shift) < memnodemapsize);
>> +   nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift];
>> +   ASSERT(nid <= MAX_NUMNODES || !node_data[nid].node_start_pfn);
>> +
>> +   return nid;
>> +}
>> +
>> +#define NODE_DATA(nid)          (&(node_data[nid]))
>
>
> I understand Jan asked to remove the inner parentheses here. And you didn't
> do it. However ...
>
>> +
>> +#define node_start_pfn(nid)     NODE_DATA(nid)->node_start_pfn
>> +#define node_spanned_pages(nid) NODE_DATA(nid)->node_spanned_pages
>> +#define node_end_pfn(nid)       NODE_DATA(nid)->node_start_pfn + \
>> +                                 NODE_DATA(nid)->node_spanned_pages
>
>
> ... here it is totally wrong to remove the parenthesis. Imagine you do:
>
> node_end_pfn(nid) * 2
>
> This will now turned into
>
> NODE_DATA(nid)->node_start_pfn + NODE_DATA(nid)->node_spanned_pages * 2
>
> The parenthesis is not correct anymore and will result to wrong computation.
> You should keep the outer parenthesis *everywhere* for safety and remove
> only the inner one in NODE_DATA.

OK.

>
> This is also more than cosmetics and I think the reviewed-by from Wei should
> have been carried.

OK.

>
>>
>>  extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
>>
>> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
>> index 6bba29e..3bb4afc 100644
>> --- a/xen/include/xen/numa.h
>> +++ b/xen/include/xen/numa.h
>> @@ -6,9 +6,6 @@
>>  #define NUMA_NO_NODE     0xFF
>>  #define NUMA_NO_DISTANCE 0xFF
>>
>> -#define MAX_NUMNODES    NR_NODES
>> -#define NR_NODE_MEMBLKS (MAX_NUMNODES * 2)
>> -
>
>
> See my comment above.
>
>>  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
>>
>>  #define domain_to_node(d) \
>>
>
> Cheers,
>
> --
> Julien Grall

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