Re: [Xen-devel] [RFC PATCH v2 09/25] ARM: NUMA: Add existing ARM numa code under CONFIG_NUMA

2017-05-09 Thread Julien Grall 

On 05/09/2017 08:14 AM, Vijay Kilari wrote:

On Mon, May 8, 2017 at 9:28 PM, Julien Grall  wrote:

Hi Vijay,

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


From: Vijaya Kumar K 

Right now CONFIG_NUMA is not enabled for ARM and
existing code in asm-arm/numa.h is for !CONFIG_NUMA.
Hence put this code under #ifndef CONFIG_NUMA.

This help to make this changes work when CONFIG_NUMA
is not enabled.



But you always turn NUMA on by default (see patch #24) and there is no
possibility to turn off NUMA.


Yes at the end of the series we enable NUMA by default.
But the the intermittent patches of this patch series fails to compile.


So for helping this series, you add code that will get rotten???

I don't like this idea at all, we should avoid to add code in Xen that 
will not be used.






Also define NODES_SHIFT macro for ARM to value 2.
This limits number of NUMA nodes supported to 4.
There is not hard restrictions on this value set to 2.



Again, why only 2 when x86 is supporting 6?

Furthermore, this is not related to this patch itself and should be part of
separate patch.

Lastly, why don't you move that to a Kconfig allowing the user to configure
the number of Nodes?


ok





Signed-off-by: Vijaya Kumar K 
---
 xen/include/asm-arm/numa.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
index 53f99af..924bfc0 100644
--- a/xen/include/asm-arm/numa.h
+++ b/xen/include/asm-arm/numa.h
@@ -3,6 +3,10 @@

 typedef uint8_t nodeid_t;

+/* Limit number of NUMA nodes supported to 4 */
+#define NODES_SHIFT 2



Why this is not covered by CONFIG_NUMA?


The below define is used in generic code irrespective of CONFIG_NUMA

#define MAX_NUMNODES(1 << NODES_SHIFT)


As you may have noticed NODES_SHIFT currently does not exist on ARM and 
we are still able to compile the generic code. So why do you need to do 
it unconditionally?


If you look at the code, xen/numa.h will define NODES_SHIFT to 0 if it 
has not previously defined. So I still don't see any reason on what you 
are doing.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH v2 09/25] ARM: NUMA: Add existing ARM numa code under CONFIG_NUMA

2017-05-09 Thread Vijay Kilari 
On Mon, May 8, 2017 at 9:28 PM, Julien Grall  wrote:
> Hi Vijay,
>
> On 28/03/17 16:53, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K 
>>
>> Right now CONFIG_NUMA is not enabled for ARM and
>> existing code in asm-arm/numa.h is for !CONFIG_NUMA.
>> Hence put this code under #ifndef CONFIG_NUMA.
>>
>> This help to make this changes work when CONFIG_NUMA
>> is not enabled.
>
>
> But you always turn NUMA on by default (see patch #24) and there is no
> possibility to turn off NUMA.

Yes at the end of the series we enable NUMA by default.
But the the intermittent patches of this patch series fails to compile.

>
>>
>> Also define NODES_SHIFT macro for ARM to value 2.
>> This limits number of NUMA nodes supported to 4.
>> There is not hard restrictions on this value set to 2.
>
>
> Again, why only 2 when x86 is supporting 6?
>
> Furthermore, this is not related to this patch itself and should be part of
> separate patch.
>
> Lastly, why don't you move that to a Kconfig allowing the user to configure
> the number of Nodes?

ok

>
>>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>>  xen/include/asm-arm/numa.h | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
>> index 53f99af..924bfc0 100644
>> --- a/xen/include/asm-arm/numa.h
>> +++ b/xen/include/asm-arm/numa.h
>> @@ -3,6 +3,10 @@
>>
>>  typedef uint8_t nodeid_t;
>>
>> +/* Limit number of NUMA nodes supported to 4 */
>> +#define NODES_SHIFT 2
>
>
> Why this is not covered by CONFIG_NUMA?

The below define is used in generic code irrespective of CONFIG_NUMA

#define MAX_NUMNODES(1 << NODES_SHIFT)

>
>> +
>> +#ifndef CONFIG_NUMA
>>  /* Fake one node for now. See also node_online_map. */
>>  #define cpu_to_node(cpu) 0
>>  #define node_to_cpumask(node)   (cpu_online_map)
>> @@ -16,6 +20,7 @@ static inline __attribute__((pure)) nodeid_t
>> phys_to_nid(paddr_t addr)
>>  #define node_spanned_pages(nid) (total_pages)
>>  #define node_start_pfn(nid) (pdx_to_pfn(frametable_base_pdx))
>>  #define __node_distance(a, b) (20)
>> +#endif /* CONFIG_NUMA */
>>
>>  static inline unsigned int arch_get_dma_bitsize(void)
>>  {
>>
>
> Cheers,
>
> --
> Julien Grall

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

Re: [Xen-devel] [RFC PATCH v2 09/25] ARM: NUMA: Add existing ARM numa code under CONFIG_NUMA

2017-05-08 Thread Julien Grall 

Hi Vijay,

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

From: Vijaya Kumar K 

Right now CONFIG_NUMA is not enabled for ARM and
existing code in asm-arm/numa.h is for !CONFIG_NUMA.
Hence put this code under #ifndef CONFIG_NUMA.

This help to make this changes work when CONFIG_NUMA
is not enabled.


But you always turn NUMA on by default (see patch #24) and there is no 
possibility to turn off NUMA.




Also define NODES_SHIFT macro for ARM to value 2.
This limits number of NUMA nodes supported to 4.
There is not hard restrictions on this value set to 2.


Again, why only 2 when x86 is supporting 6?

Furthermore, this is not related to this patch itself and should be part 
of separate patch.


Lastly, why don't you move that to a Kconfig allowing the user to 
configure the number of Nodes?




Signed-off-by: Vijaya Kumar K 
---
 xen/include/asm-arm/numa.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
index 53f99af..924bfc0 100644
--- a/xen/include/asm-arm/numa.h
+++ b/xen/include/asm-arm/numa.h
@@ -3,6 +3,10 @@

 typedef uint8_t nodeid_t;

+/* Limit number of NUMA nodes supported to 4 */
+#define NODES_SHIFT 2


Why this is not covered by CONFIG_NUMA?


+
+#ifndef CONFIG_NUMA
 /* Fake one node for now. See also node_online_map. */
 #define cpu_to_node(cpu) 0
 #define node_to_cpumask(node)   (cpu_online_map)
@@ -16,6 +20,7 @@ static inline __attribute__((pure)) nodeid_t 
phys_to_nid(paddr_t addr)
 #define node_spanned_pages(nid) (total_pages)
 #define node_start_pfn(nid) (pdx_to_pfn(frametable_base_pdx))
 #define __node_distance(a, b) (20)
+#endif /* CONFIG_NUMA */

 static inline unsigned int arch_get_dma_bitsize(void)
 {



Cheers,

--
Julien Grall

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

[Xen-devel] [RFC PATCH v2 09/25] ARM: NUMA: Add existing ARM numa code under CONFIG_NUMA

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

Right now CONFIG_NUMA is not enabled for ARM and
existing code in asm-arm/numa.h is for !CONFIG_NUMA.
Hence put this code under #ifndef CONFIG_NUMA.

This help to make this changes work when CONFIG_NUMA
is not enabled.

Also define NODES_SHIFT macro for ARM to value 2.
This limits number of NUMA nodes supported to 4.
There is not hard restrictions on this value set to 2.

Signed-off-by: Vijaya Kumar K 
---
 xen/include/asm-arm/numa.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
index 53f99af..924bfc0 100644
--- a/xen/include/asm-arm/numa.h
+++ b/xen/include/asm-arm/numa.h
@@ -3,6 +3,10 @@
 
 typedef uint8_t nodeid_t;
 
+/* Limit number of NUMA nodes supported to 4 */
+#define NODES_SHIFT 2
+
+#ifndef CONFIG_NUMA
 /* Fake one node for now. See also node_online_map. */
 #define cpu_to_node(cpu) 0
 #define node_to_cpumask(node)   (cpu_online_map)
@@ -16,6 +20,7 @@ static inline __attribute__((pure)) nodeid_t 
phys_to_nid(paddr_t addr)
 #define node_spanned_pages(nid) (total_pages)
 #define node_start_pfn(nid) (pdx_to_pfn(frametable_base_pdx))
 #define __node_distance(a, b) (20)
+#endif /* CONFIG_NUMA */
 
 static inline unsigned int arch_get_dma_bitsize(void)
 {
-- 
2.7.4


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