| Message ID | 1490716413-19796-10-git-send-email-vijay.kilari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vijay, On 28/03/17 16:53, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > 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 <Vijaya.Kumar@cavium.com> > --- > 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,
On Mon, May 8, 2017 at 9:28 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Vijay, > > On 28/03/17 16:53, vijay.kilari@gmail.com wrote: >> >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> >> 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 <Vijaya.Kumar@cavium.com> >> --- >> 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
On 05/09/2017 08:14 AM, Vijay Kilari wrote: > On Mon, May 8, 2017 at 9:28 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Vijay, >> >> On 28/03/17 16:53, vijay.kilari@gmail.com wrote: >>> >>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >>> >>> 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 <Vijaya.Kumar@cavium.com> >>> --- >>> 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,
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) {