| Message ID | 1500378106-2620-8-git-send-email-vijay.kilari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vijay, On 18/07/17 12:41, 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. Though CONFIG_NUMA is enabled by default, > manually disabling this option is possible and compilation > should go through. Hence kept the these changes under > !CONFIG_NUMA. This is still no true. It is not possible to disable CONFIG_NUMA from the Kconfig unless you hack it (just tried it)... As I said on v2, if you always enable NUMA why should we add code in Xen that get rotten? Either you allow NUMA to be disabled by the user or you drop this code. > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > --- > v3: - Dropped NODE_SHIFT define > --- > xen/include/asm-arm/numa.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h > index 53f99af..7f00a36 100644 > --- a/xen/include/asm-arm/numa.h > +++ b/xen/include/asm-arm/numa.h > @@ -3,6 +3,7 @@ > > typedef uint8_t nodeid_t; > > +#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 +17,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 Tue, Jul 18, 2017 at 11:36 PM, Julien Grall <julien.grall@arm.com> wrote: > Hi Vijay, > > On 18/07/17 12:41, 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. Though CONFIG_NUMA is enabled by default, >> manually disabling this option is possible and compilation >> should go through. Hence kept the these changes under >> !CONFIG_NUMA. > > > This is still no true. It is not possible to disable CONFIG_NUMA from the > Kconfig unless you hack it (just tried it)... > > As I said on v2, if you always enable NUMA why should we add code in Xen > that get rotten? Either you allow NUMA to be disabled by the user or you > drop this code. The reason is: The next patch #8, which does the code movement moves the generic code to common header file xen/numa.h. If we don't put these *existing* defines in asm-arm/numa.h under #ifndef CONFIG_NUMA, the compilation fails for ARM. Is it ok to removes these defines under separate patch after enabling NUMA config at the end of patch series? Let me know if you have any better approach. > >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> --- >> v3: - Dropped NODE_SHIFT define >> --- >> xen/include/asm-arm/numa.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h >> index 53f99af..7f00a36 100644 >> --- a/xen/include/asm-arm/numa.h >> +++ b/xen/include/asm-arm/numa.h >> @@ -3,6 +3,7 @@ >> >> typedef uint8_t nodeid_t; >> >> +#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 +17,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
Hi Vijay, On 20/07/17 10:31, Vijay Kilari wrote: > On Tue, Jul 18, 2017 at 11:36 PM, Julien Grall <julien.grall@arm.com> wrote: >> Hi Vijay, >> >> On 18/07/17 12:41, 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. Though CONFIG_NUMA is enabled by default, >>> manually disabling this option is possible and compilation >>> should go through. Hence kept the these changes under >>> !CONFIG_NUMA. >> >> >> This is still no true. It is not possible to disable CONFIG_NUMA from the >> Kconfig unless you hack it (just tried it)... >> >> As I said on v2, if you always enable NUMA why should we add code in Xen >> that get rotten? Either you allow NUMA to be disabled by the user or you >> drop this code. > > The reason is: The next patch #8, which does the code movement moves > the generic code to common header file xen/numa.h. > If we don't put these *existing* defines in asm-arm/numa.h under > #ifndef CONFIG_NUMA, > the compilation fails for ARM. > > Is it ok to removes these defines under separate patch after enabling > NUMA config > at the end of patch series? > > Let me know if you have any better approach. The question here is more, do we want to allow the user disabling NUMA (even if it has to be guarded with EXPERT)? You seem to choose the approach where NUMA is here by default and can't be disabled. 1) If we decide to let the user configuring NUMA, then this patch is valid as it is. 2) If not, then you need a patch drop this code at the end and have a word in this commit message explaining this is temporary... When the question above is answered, you need to do either 1) or 2) according to the answer. Cheers,
diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h index 53f99af..7f00a36 100644 --- a/xen/include/asm-arm/numa.h +++ b/xen/include/asm-arm/numa.h @@ -3,6 +3,7 @@ typedef uint8_t nodeid_t; +#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 +17,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) {