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

2017-06-30 Thread Jan Beulich 
>>>  03/28/17 5:55 PM >>>
> --- 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;
>  
> -int srat_disabled(void)
> +bool is_numa_off(void)

numa_enabled() (or less desirably numa_disabled())

> +bool get_acpi_numa(void)

acpi_numa_enabled() then perhaps.

Iirc Julien has already commented on the non-boolean nature of acpi_numa.

> @@ -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;
> +}

I don't see the point of having static accessors for static variables. Even
if the accessor became non-static, I'd expect it to be used only in other
translation units.

Jan

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

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

2017-05-08 Thread Julien Grall 

Hi Vijay,

On 02/05/17 10:47, Vijay Kilari wrote:

On Tue, Apr 25, 2017 at 9:13 PM, Jan Beulich  wrote:

On 25.04.17 at 17:14,  wrote:

On 25/04/17 15:54, Vijay Kilari wrote:

On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall  wrote:


By setting 1, we are enabling acpi_numa by default. If not enabled, the
below
call has check srat_disabled() before proceeding fails.




My understanding is on x86 acpi_numa is disabled by default and will be
enabled if they are able to parse the SRAT. So why are you changing the
behavior for x86?



acpi_numa = 0 means it is enabled by default on x86.



In acpi_scan_nodes:

if (acpi_numa <= 0)
  return -1;

So it does not seem that 0 means enabled.


IMO, In x86
 -1 means disabled
  0 enabled but not numa initialized
  1 enabled and numa initialized.

I clubbed 0 & 1.


 From your description 0 and 1 have different meaning, so I don't see
how you can merge them that easily without any explanation.

Anyway, I will leave x86 maintainers give their opinion here.


I'm pretty certain this needs to remain a tristate.


Ok. I will drop this patch from this series and can be fixed
outside this series.
BTW, any review comments on remaining patches?


I had a looked at the series and decided to stop reviewing it because 
comments are not addressed.


I am not going to review anything until *all* the comments from previous 
version are addressed. I would recommend the other to do the same.


Cheers,

--
Julien Grall

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

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

2017-05-02 Thread Jan Beulich 
>>> On 02.05.17 at 11:47,  wrote:
> BTW, any review comments on remaining patches?

I didn't even manage to get to the start of the flood of RFCs
posted during the last 1.5 months (which priority wise all sit behind
the various non-RFC postings), so I can't predict when I'll get to
yours. Also please remember that the focus right now is to make
4.9 good quality ...

Jan


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

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

2017-05-02 Thread Vijay Kilari 
On Tue, Apr 25, 2017 at 9:13 PM, Jan Beulich  wrote:
 On 25.04.17 at 17:14,  wrote:
>> On 25/04/17 15:54, Vijay Kilari wrote:
>>> On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall  wrote:
>>>
>>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>>> below
>>> call has check srat_disabled() before proceeding fails.
>>
>>
>>
>> My understanding is on x86 acpi_numa is disabled by default and will be
>> enabled if they are able to parse the SRAT. So why are you changing the
>> behavior for x86?
>
>
> acpi_numa = 0 means it is enabled by default on x86.


 In acpi_scan_nodes:

 if (acpi_numa <= 0)
   return -1;

 So it does not seem that 0 means enabled.
>>>
>>> IMO, In x86
>>>  -1 means disabled
>>>   0 enabled but not numa initialized
>>>   1 enabled and numa initialized.
>>>
>>> I clubbed 0 & 1.
>>
>>  From your description 0 and 1 have different meaning, so I don't see
>> how you can merge them that easily without any explanation.
>>
>> Anyway, I will leave x86 maintainers give their opinion here.
>
> I'm pretty certain this needs to remain a tristate.

Ok. I will drop this patch from this series and can be fixed
outside this series.
BTW, any review comments on remaining patches?

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

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

2017-04-25 Thread Jan Beulich 
>>> On 25.04.17 at 17:14,  wrote:
> On 25/04/17 15:54, Vijay Kilari wrote:
>> On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall  wrote:
>>
>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>> below
>> call has check srat_disabled() before proceeding fails.
>
>
>
> My understanding is on x86 acpi_numa is disabled by default and will be
> enabled if they are able to parse the SRAT. So why are you changing the
> behavior for x86?


 acpi_numa = 0 means it is enabled by default on x86.
>>>
>>>
>>> In acpi_scan_nodes:
>>>
>>> if (acpi_numa <= 0)
>>>   return -1;
>>>
>>> So it does not seem that 0 means enabled.
>>
>> IMO, In x86
>>  -1 means disabled
>>   0 enabled but not numa initialized
>>   1 enabled and numa initialized.
>>
>> I clubbed 0 & 1.
> 
>  From your description 0 and 1 have different meaning, so I don't see 
> how you can merge them that easily without any explanation.
> 
> Anyway, I will leave x86 maintainers give their opinion here.

I'm pretty certain this needs to remain a tristate.

Jan


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

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

2017-04-25 Thread Julien Grall 



On 25/04/17 15:54, Vijay Kilari wrote:

On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall  wrote:


By setting 1, we are enabling acpi_numa by default. If not enabled, the
below
call has check srat_disabled() before proceeding fails.




My understanding is on x86 acpi_numa is disabled by default and will be
enabled if they are able to parse the SRAT. So why are you changing the
behavior for x86?



acpi_numa = 0 means it is enabled by default on x86.



In acpi_scan_nodes:

if (acpi_numa <= 0)
  return -1;

So it does not seem that 0 means enabled.


IMO, In x86
 -1 means disabled
  0 enabled but not numa initialized
  1 enabled and numa initialized.

I clubbed 0 & 1.


From your description 0 and 1 have different meaning, so I don't see 
how you can merge them that easily without any explanation.


Anyway, I will leave x86 maintainers give their opinion here.



I was referring to below code in x86 where in acpi_numa = 0 means
srat_disabled() returns false. Which means acpi_numa is enabled implicitly.

int srat_disabled(void)
{
  return numa_off || acpi_numa < 0;
}

When I changed acpi_numa to bool, it is initialized to 1 and changed
below code.

bool srat_disabled(void)
{
return numa_off || acpi_numa == 0;
}

Also this srat_disabed() is used in acpi_numa_memory_affinity_init which is
called from acpi_numa_init() before calling acpi_scan_nodes().




--
Julien Grall

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

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

2017-04-25 Thread Vijay Kilari 
On Tue, Apr 25, 2017 at 5:58 PM, Julien Grall  wrote:
>
>
> On 25/04/17 13:20, Vijay Kilari wrote:
>>
>> On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall 
>> wrote:
>>>
>>> Hello Vijay,
>>>
>>> On 25/04/17 07:54, Vijay Kilari wrote:


 On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall 
 wrote:
>
>
> Hi Vijay,
>
> On 28/03/17 16:53, vijay.kil...@gmail.com wrote:
>>
>>
>>
>> From: Vijaya Kumar K 
>>
>> 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.
>>>
>>>
>>>
>>> Are you sure? With a quick grep I spot it sounds like acpi_numa can have
>>> the
>>> value 0, -1, 1.
>>>
>>
>> Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use
>> of
>> having 3 values to say enable or disable.
>
>
> Then explain why in the commit message and don't let people discover. If you
> have not done it, I would recommend to read:
>
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
>
>>

 By setting 1, we are enabling acpi_numa by default. If not enabled, the
 below
 call has check srat_disabled() before proceeding fails.
>>>
>>>
>>>
>>> My understanding is on x86 acpi_numa is disabled by default and will be
>>> enabled if they are able to parse the SRAT. So why are you changing the
>>> behavior for x86?
>>
>>
>> acpi_numa = 0 means it is enabled by default on x86.
>
>
> In acpi_scan_nodes:
>
> if (acpi_numa <= 0)
>   return -1;
>
> So it does not seem that 0 means enabled.

IMO, In x86
 -1 means disabled
  0 enabled but not numa initialized
  1 enabled and numa initialized.

I clubbed 0 & 1.

I was referring to below code in x86 where in acpi_numa = 0 means
srat_disabled() returns false. Which means acpi_numa is enabled implicitly.

int srat_disabled(void)
{
  return numa_off || acpi_numa < 0;
}

When I changed acpi_numa to bool, it is initialized to 1 and changed
below code.

bool srat_disabled(void)
{
return numa_off || acpi_numa == 0;
}

Also this srat_disabed() is used in acpi_numa_memory_affinity_init which is
called from acpi_numa_init() before calling acpi_scan_nodes().

>
>>
>>>

 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
>>>
>>>
>>>
>>> The variables were global so they could already be used anywhere.
>>>
>>> Furthermore, those helpers are not even inlined, so everytime you want to
>>> access read acpi_numa you have to do a branch then a memory access.
>>>
>>> So what is the point to do that?
>>
>>
>> I agree with making inline. But I don't think there is any harm in making
>> them
>> static and adding helpers.
>
>
> But why? Why don't you keep the code as it is? You modify code without any
> justification and not for the better.
>
> Cheers,
>
> --
> Julien Grall

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

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

2017-04-25 Thread Julien Grall 



On 25/04/17 13:20, Vijay Kilari wrote:

On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall  wrote:

Hello Vijay,

On 25/04/17 07:54, Vijay Kilari wrote:


On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall 
wrote:


Hi Vijay,

On 28/03/17 16:53, vijay.kil...@gmail.com wrote:



From: Vijaya Kumar K 

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.



Are you sure? With a quick grep I spot it sounds like acpi_numa can have the
value 0, -1, 1.



Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use of
having 3 values to say enable or disable.


Then explain why in the commit message and don't let people discover. If 
you have not done it, I would recommend to read:


https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches





By setting 1, we are enabling acpi_numa by default. If not enabled, the
below
call has check srat_disabled() before proceeding fails.



My understanding is on x86 acpi_numa is disabled by default and will be
enabled if they are able to parse the SRAT. So why are you changing the
behavior for x86?


acpi_numa = 0 means it is enabled by default on x86.


In acpi_scan_nodes:

if (acpi_numa <= 0)
  return -1;

So it does not seem that 0 means enabled.







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



The variables were global so they could already be used anywhere.

Furthermore, those helpers are not even inlined, so everytime you want to
access read acpi_numa you have to do a branch then a memory access.

So what is the point to do that?


I agree with making inline. But I don't think there is any harm in making them
static and adding helpers.


But why? Why don't you keep the code as it is? You modify code without 
any justification and not for the better.


Cheers,

--
Julien Grall

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

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

2017-04-25 Thread Vijay Kilari 
On Tue, Apr 25, 2017 at 5:34 PM, Julien Grall  wrote:
> Hello Vijay,
>
> On 25/04/17 07:54, Vijay Kilari wrote:
>>
>> On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall 
>> wrote:
>>>
>>> Hi Vijay,
>>>
>>> On 28/03/17 16:53, vijay.kil...@gmail.com wrote:


 From: Vijaya Kumar K 

 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.
>
>
> Are you sure? With a quick grep I spot it sounds like acpi_numa can have the
> value 0, -1, 1.
>

Hmm.. But I don't see use of having 0, -1 and 1. But I don't see any use of
having 3 values to say enable or disable.

>>
>> By setting 1, we are enabling acpi_numa by default. If not enabled, the
>> below
>> call has check srat_disabled() before proceeding fails.
>
>
> My understanding is on x86 acpi_numa is disabled by default and will be
> enabled if they are able to parse the SRAT. So why are you changing the
> behavior for x86?

acpi_numa = 0 means it is enabled by default on x86.

>
>>
>> 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
>
>
> The variables were global so they could already be used anywhere.
>
> Furthermore, those helpers are not even inlined, so everytime you want to
> access read acpi_numa you have to do a branch then a memory access.
>
> So what is the point to do that?

I agree with making inline. But I don't think there is any harm in making them
static and adding helpers.

>
>
 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_INIT1

 -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.
>
>
> Sorry but what are prototypes if not code?
>
> The point of moving the prototypes in the patch which move the actual
> declarations is helping the reviewers to check if you correctly moved
> everything.

I am ok if it helps in review.

>
>
>>
>>>
>>>
  #endif /* _XEN_NUMA_H */

>>>
>>> --
>>> Julien Grall
>
>
> --
> Julien Grall

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

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

2017-04-25 Thread Julien Grall 

Hello Vijay,

On 25/04/17 07:54, Vijay Kilari wrote:

On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall  wrote:

Hi Vijay,

On 28/03/17 16:53, vijay.kil...@gmail.com wrote:


From: Vijaya Kumar K 

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.


Are you sure? With a quick grep I spot it sounds like acpi_numa can have 
the value 0, -1, 1.




By setting 1, we are enabling acpi_numa by default. If not enabled, the below
call has check srat_disabled() before proceeding fails.


My understanding is on x86 acpi_numa is disabled by default and will be 
enabled if they are able to parse the SRAT. So why are you changing the 
behavior for x86?




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


The variables were global so they could already be used anywhere.

Furthermore, those helpers are not even inlined, so everytime you want 
to access read acpi_numa you have to do a branch then a memory access.


So what is the point to do that?


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_INIT1

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


Sorry but what are prototypes if not code?

The point of moving the prototypes in the patch which move the actual 
declarations is helping the reviewers to check if you correctly moved 
everything.








 #endif /* _XEN_NUMA_H */



--
Julien Grall


--
Julien Grall

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

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

2017-04-25 Thread Vijay Kilari 
On Thu, Apr 20, 2017 at 9:29 PM, Julien Grall  wrote:
> Hi Vijay,
>
> On 28/03/17 16:53, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K 
>>
>> 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 
>> ---
>>  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(,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, _shift) )
>> +if ( compute_memnode_shift(nodes, get_numa_fake(), NULL,
>> _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 @@

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

2017-04-20 Thread Julien Grall 

Hi Vijay,

On 28/03/17 16:53, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K 

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.

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



Also return value of srat_disabled is changed to bool.

Signed-off-by: Vijaya Kumar K 
---
 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.



-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(,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, _shift) )
+if ( compute_memnode_shift(nodes, get_numa_fake(), NULL, _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] =

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

2017-03-28 Thread vijay . kilari 
From: Vijaya Kumar K 

Add accessor functions for acpi_numa and numa_off static
variables. Init value of acpi_numa is set 1 instead of 0.
Also return value of srat_disabled is changed to bool.

Signed-off-by: Vijaya Kumar K 
---
 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;
 
-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(,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, _shift) )
+if ( compute_memnode_shift(nodes, get_numa_fake(), NULL, _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",