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

[Julien Grall <julien.grall@xxxxxxx>]

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.
[...]

> @@ -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.
This is also more than cosmetics and I think the reviewed-by from Wei 
should have been carried.
>  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