diff mbox

[2/2] x86/NUMA: cleanup

Message ID 57AB0ED10200007800104953@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Aug. 10, 2016, 9:24 a.m. UTC 
- drop the only left CONFIG_NUMA conditional (this is always true)
- drop struct node_data's node_id field (being always equal to the
  node_data[] array index used)
- don't open code node_{start,end}_pfn() nor node_spanned_pages()
  except when used as lvalues (those could be converted too, but this
  seems a little awkward)
- no longer open code pfn_to_paddr() in an expression being modified
  anyway
- make dump less verbose by logging actual vs intended node IDs only
  when they don't match

Signed-off-by: Jan Beulich <jbeulich@suse.com>
x86/NUMA: cleanup

- drop the only left CONFIG_NUMA conditional (this is always true)
- drop struct node_data's node_id field (being always equal to the
  node_data[] array index used)
- don't open code node_{start,end}_pfn() nor node_spanned_pages()
  except when used as lvalues (those could be converted too, but this
  seems a little awkward)
- no longer open code pfn_to_paddr() in an expression being modified
  anyway
- make dump less verbose by logging actual vs intended node IDs only
  when they don't match

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -179,7 +179,6 @@ void __init setup_node_bootmem(nodeid_t
     start_pfn = start >> PAGE_SHIFT;
     end_pfn = end >> PAGE_SHIFT;
 
-    NODE_DATA(nodeid)->node_id = nodeid;
     NODE_DATA(nodeid)->node_start_pfn = start_pfn;
     NODE_DATA(nodeid)->node_spanned_pages = end_pfn - start_pfn;
 
@@ -386,16 +385,15 @@ static void dump_numa(unsigned char key)
 
     for_each_online_node ( i )
     {
-        paddr_t pa = (paddr_t)(NODE_DATA(i)->node_start_pfn + 1)<< PAGE_SHIFT;
-        printk("idx%d -> NODE%d start->%lu size->%lu free->%lu\n",
-               i, NODE_DATA(i)->node_id,
-               NODE_DATA(i)->node_start_pfn,
-               NODE_DATA(i)->node_spanned_pages,
+        paddr_t pa = pfn_to_paddr(node_start_pfn(i) + 1);
+
+        printk("NODE%u start->%lu size->%lu free->%lu\n",
+               i, node_start_pfn(i), node_spanned_pages(i),
                avail_node_heap_pages(i));
         /* sanity check phys_to_nid() */
-        printk("phys_to_nid(%"PRIpaddr") -> %d should be %d\n", pa,
-               phys_to_nid(pa),
-               NODE_DATA(i)->node_id);
+        if ( phys_to_nid(pa) != i )
+            printk("phys_to_nid(%"PRIpaddr") -> %d should be %u\n",
+                   pa, phys_to_nid(pa), i);
     }
 
     j = cpumask_first(&cpu_online_map);
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1386,21 +1386,21 @@ int memory_add(unsigned long spfn, unsig
             goto destroy_directmap;
     }
 
-    old_node_start = NODE_DATA(node)->node_start_pfn;
-    old_node_span = NODE_DATA(node)->node_spanned_pages;
+    old_node_start = node_start_pfn(node);
+    old_node_span = node_spanned_pages(node);
     orig_online = node_online(node);
 
     if ( !orig_online )
     {
         dprintk(XENLOG_WARNING, "node %x pxm %x is not online\n",node, pxm);
-        NODE_DATA(node)->node_id = node;
         NODE_DATA(node)->node_start_pfn = spfn;
         NODE_DATA(node)->node_spanned_pages =
                 epfn - node_start_pfn(node);
         node_set_online(node);
-    }else
+    }
+    else
     {
-        if (NODE_DATA(node)->node_start_pfn > spfn)
+        if (node_start_pfn(node) > spfn)
             NODE_DATA(node)->node_start_pfn = spfn;
         if (node_end_pfn(node) < epfn)
             NODE_DATA(node)->node_spanned_pages = epfn - node_start_pfn(node);
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -40,7 +40,6 @@ extern void srat_detect_node(int cpu);
 
 extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
 extern nodeid_t apicid_to_node[];
-#ifdef CONFIG_NUMA
 extern void init_cpu_to_node(void);
 
 static inline void clear_node_cpumask(int cpu)
@@ -56,7 +55,6 @@ extern u8 *memnodemap;
 struct node_data {
     unsigned long node_start_pfn;
     unsigned long node_spanned_pages;
-    nodeid_t      node_id;
 };
 
 extern struct node_data node_data[];
@@ -78,11 +76,6 @@ static inline __attribute__((pure)) node
 				 NODE_DATA(nid)->node_spanned_pages)
 
 extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
-#else
-#define init_cpu_to_node() do {} while (0)
-#define clear_node_cpumask(cpu) do {} while (0)
-#define valid_numa_range(start, end, node) 1
-#endif
 
 void srat_parse_regions(u64 addr);
 extern u8 __node_distance(nodeid_t a, nodeid_t b);

Comments

Andrew Cooper Aug. 10, 2016, 10:35 a.m. UTC | #1 
On 10/08/16 10:24, Jan Beulich wrote:
> - drop the only left CONFIG_NUMA conditional (this is always true)

I observe that CONFIG_NUMA_EMU is also unconditionally true, which
offers further cleanup opportunities (albeit it probably a separate patch).

> - drop struct node_data's node_id field (being always equal to the
>   node_data[] array index used)
> - don't open code node_{start,end}_pfn() nor node_spanned_pages()
>   except when used as lvalues (those could be converted too, but this
>   seems a little awkward)
> - no longer open code pfn_to_paddr() in an expression being modified
>   anyway
> - make dump less verbose by logging actual vs intended node IDs only
>   when they don't match
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Jan Beulich Aug. 10, 2016, 11:47 a.m. UTC | #2 
>>> On 10.08.16 at 12:35, <andrew.cooper3@citrix.com> wrote:
> On 10/08/16 10:24, Jan Beulich wrote:
>> - drop the only left CONFIG_NUMA conditional (this is always true)
> 
> I observe that CONFIG_NUMA_EMU is also unconditionally true, which
> offers further cleanup opportunities (albeit it probably a separate patch).

So I thought, but then wasn't sure eliminating just the conditionals
it would actually be the right route - the alternative would be to
instead just drop that NUMA emulation code altogether, as no-one
should be using it.

Jan
Dario Faggioli Aug. 16, 2016, 10:02 a.m. UTC | #3 
On Wed, 2016-08-10 at 05:47 -0600, Jan Beulich wrote:
> > On 10.08.16 at 12:35, <andrew.cooper3@citrix.com> wrote:
> > I observe that CONFIG_NUMA_EMU is also unconditionally true, which
> > offers further cleanup opportunities (albeit it probably a separate
> > patch).
> So I thought, but then wasn't sure eliminating just the conditionals
> it would actually be the right route - the alternative would be to
> instead just drop that NUMA emulation code altogether, as no-one
> should be using it.
> 
Last time I tried to use it, it indeed was *not* useful at all... and
looking at it, it seems that nothing changed since then.

AFAICT, it's something imported from Linux quite a few time back, but
while Linux evolved its own NUMA_EMU support to something that may
actually be useful, ours stayed original.

So, for what the opinion of someone that tried to use it is worth, I
agree that we either also improve it (e.g., by trying syncing with the
Linux one), or kill it.

Regards,
Dairo
diff mbox

Patch

--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -179,7 +179,6 @@  void __init setup_node_bootmem(nodeid_t
     start_pfn = start >> PAGE_SHIFT;
     end_pfn = end >> PAGE_SHIFT;
 
-    NODE_DATA(nodeid)->node_id = nodeid;
     NODE_DATA(nodeid)->node_start_pfn = start_pfn;
     NODE_DATA(nodeid)->node_spanned_pages = end_pfn - start_pfn;
 
@@ -386,16 +385,15 @@  static void dump_numa(unsigned char key)
 
     for_each_online_node ( i )
     {
-        paddr_t pa = (paddr_t)(NODE_DATA(i)->node_start_pfn + 1)<< PAGE_SHIFT;
-        printk("idx%d -> NODE%d start->%lu size->%lu free->%lu\n",
-               i, NODE_DATA(i)->node_id,
-               NODE_DATA(i)->node_start_pfn,
-               NODE_DATA(i)->node_spanned_pages,
+        paddr_t pa = pfn_to_paddr(node_start_pfn(i) + 1);
+
+        printk("NODE%u start->%lu size->%lu free->%lu\n",
+               i, node_start_pfn(i), node_spanned_pages(i),
                avail_node_heap_pages(i));
         /* sanity check phys_to_nid() */
-        printk("phys_to_nid(%"PRIpaddr") -> %d should be %d\n", pa,
-               phys_to_nid(pa),
-               NODE_DATA(i)->node_id);
+        if ( phys_to_nid(pa) != i )
+            printk("phys_to_nid(%"PRIpaddr") -> %d should be %u\n",
+                   pa, phys_to_nid(pa), i);
     }
 
     j = cpumask_first(&cpu_online_map);
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1386,21 +1386,21 @@  int memory_add(unsigned long spfn, unsig
             goto destroy_directmap;
     }
 
-    old_node_start = NODE_DATA(node)->node_start_pfn;
-    old_node_span = NODE_DATA(node)->node_spanned_pages;
+    old_node_start = node_start_pfn(node);
+    old_node_span = node_spanned_pages(node);
     orig_online = node_online(node);
 
     if ( !orig_online )
     {
         dprintk(XENLOG_WARNING, "node %x pxm %x is not online\n",node, pxm);
-        NODE_DATA(node)->node_id = node;
         NODE_DATA(node)->node_start_pfn = spfn;
         NODE_DATA(node)->node_spanned_pages =
                 epfn - node_start_pfn(node);
         node_set_online(node);
-    }else
+    }
+    else
     {
-        if (NODE_DATA(node)->node_start_pfn > spfn)
+        if (node_start_pfn(node) > spfn)
             NODE_DATA(node)->node_start_pfn = spfn;
         if (node_end_pfn(node) < epfn)
             NODE_DATA(node)->node_spanned_pages = epfn - node_start_pfn(node);
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -40,7 +40,6 @@  extern void srat_detect_node(int cpu);
 
 extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
 extern nodeid_t apicid_to_node[];
-#ifdef CONFIG_NUMA
 extern void init_cpu_to_node(void);
 
 static inline void clear_node_cpumask(int cpu)
@@ -56,7 +55,6 @@  extern u8 *memnodemap;
 struct node_data {
     unsigned long node_start_pfn;
     unsigned long node_spanned_pages;
-    nodeid_t      node_id;
 };
 
 extern struct node_data node_data[];
@@ -78,11 +76,6 @@  static inline __attribute__((pure)) node
 				 NODE_DATA(nid)->node_spanned_pages)
 
 extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
-#else
-#define init_cpu_to_node() do {} while (0)
-#define clear_node_cpumask(cpu) do {} while (0)
-#define valid_numa_range(start, end, node) 1
-#endif
 
 void srat_parse_regions(u64 addr);
 extern u8 __node_distance(nodeid_t a, nodeid_t b);