diff mbox

[RFC,v3,04/24] x86: NUMA: Rename and sanitize memnode shift code

Message ID 1500378106-2620-5-git-send-email-vijay.kilari@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vijay Kilari July 18, 2017, 11:41 a.m. UTC 
From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

memnode_shift variable is changed from int to unsigned int.
With this change, compute_memnode_shift() returns error value
instead of returning shift value. The memnode_shift is updated inside
compute_memnode_shift().

Also, following changes are made
  - Rename compute_hash_shift to compute_memnode_shift
  - Update int to unsigned int for params in extract_lsb_from_nodes()
  - Return values of populate_memnodemap() is changed

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
v3:
  - Update int to unsigned int for params in extract_lsb_from_nodes()
  - Return values of populate_memnodemap() is changed
---
 xen/arch/x86/numa.c        | 53 ++++++++++++++++++++++++----------------------
 xen/arch/x86/srat.c        |  7 +++---
 xen/include/asm-x86/numa.h |  6 +++---
 3 files changed, 34 insertions(+), 32 deletions(-)

Comments

Wei Liu July 18, 2017, 3:29 p.m. UTC | #1 
On Tue, Jul 18, 2017 at 05:11:26PM +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> memnode_shift variable is changed from int to unsigned int.
> With this change, compute_memnode_shift() returns error value
> instead of returning shift value. The memnode_shift is updated inside
> compute_memnode_shift().
> 
> Also, following changes are made
>   - Rename compute_hash_shift to compute_memnode_shift
>   - Update int to unsigned int for params in extract_lsb_from_nodes()
>   - Return values of populate_memnodemap() is changed
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Julien Grall July 19, 2017, 5:12 p.m. UTC | #2 
Hi Vijay,

On 18/07/17 12:41, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> memnode_shift variable is changed from int to unsigned int.
> With this change, compute_memnode_shift() returns error value
> instead of returning shift value. The memnode_shift is updated inside
> compute_memnode_shift().
>
> Also, following changes are made
>   - Rename compute_hash_shift to compute_memnode_shift
>   - Update int to unsigned int for params in extract_lsb_from_nodes()
>   - Return values of populate_memnodemap() is changed

I am not sure to understand the rationale behind changing the return 
value of populate_memnodemap. Likely this mean a bit more description in 
the commit message.

Cheers,
Vijay Kilari July 20, 2017, 6:56 a.m. UTC | #3 
On Wed, Jul 19, 2017 at 10:42 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>
>>
>> memnode_shift variable is changed from int to unsigned int.
>> With this change, compute_memnode_shift() returns error value
>> instead of returning shift value. The memnode_shift is updated inside
>> compute_memnode_shift().
>>
>> Also, following changes are made
>>   - Rename compute_hash_shift to compute_memnode_shift
>>   - Update int to unsigned int for params in extract_lsb_from_nodes()
>>   - Return values of populate_memnodemap() is changed
>
>
> I am not sure to understand the rationale behind changing the return value
> of populate_memnodemap. Likely this mean a bit more description in the
> commit message.

There is no much rationale behind it. As a part of cleanup, I have made
meaningful return values. Anyway, I will update commit message.

>
> Cheers,
>
> --
> Julien Grall
diff mbox

Patch

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index aa4a7c1..2ea2ec0 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -24,7 +24,7 @@  custom_param("numa", numa_setup);
 struct node_data node_data[MAX_NUMNODES];
 
 /* Mapping from pdx to node id */
-int memnode_shift;
+unsigned int memnode_shift;
 
 /*
  * In case of numa init failure or numa off,
@@ -59,15 +59,16 @@  int srat_disabled(void)
 /*
  * Given a shift value, try to populate memnodemap[]
  * Returns :
- * 1 if OK
- * 0 if memnodmap[] too small (of shift too small)
- * -1 if node overlap or lost ram (shift too big)
+ * 0 if OK
+ * -ENOSPC if memnodmap[] too small (or shift too small)
+ * -EINVAL if node overlap or lost ram (shift too big)
  */
 static int __init populate_memnodemap(const struct node *nodes,
-                                      int numnodes, int shift, nodeid_t *nodeids)
+                                      unsigned int numnodes, unsigned int shift,
+                                      nodeid_t *nodeids)
 {
     unsigned long spdx, epdx;
-    int i, res = -1;
+    int i, res = -EINVAL;
 
     memset(memnodemap, NUMA_NO_NODE, memnodemapsize * sizeof(*memnodemap));
     for ( i = 0; i < numnodes; i++ )
@@ -77,10 +78,10 @@  static int __init populate_memnodemap(const struct node *nodes,
         if ( spdx >= epdx )
             continue;
         if ( (epdx >> shift) >= memnodemapsize )
-            return 0;
+            return -ENOSPC;
         do {
             if ( memnodemap[spdx >> shift] != NUMA_NO_NODE )
-                return -1;
+                return -EINVAL;
 
             if ( !nodeids )
                 memnodemap[spdx >> shift] = i;
@@ -89,7 +90,7 @@  static int __init populate_memnodemap(const struct node *nodes,
 
             spdx += (1UL << shift);
         } while ( spdx < epdx );
-        res = 1;
+        res = 0;
     }
 
     return res;
@@ -105,7 +106,7 @@  static int __init allocate_cachealigned_memnodemap(void)
         printk(KERN_ERR
                "NUMA: Unable to allocate Memory to Node hash map\n");
         memnodemapsize = 0;
-        return -1;
+        return -ENOMEM;
     }
 
     memnodemap = mfn_to_virt(mfn);
@@ -122,10 +123,10 @@  static int __init allocate_cachealigned_memnodemap(void)
  * The LSB of all start and end addresses in the node map is the value of the
  * maximum possible shift.
  */
-static int __init extract_lsb_from_nodes(const struct node *nodes,
-                                         int numnodes)
+static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
+                                                  unsigned int numnodes)
 {
-    int i, nodes_used = 0;
+    unsigned int i, nodes_used = 0;
     unsigned long spdx, epdx;
     unsigned long bitfield = 0, memtop = 0;
 
@@ -149,27 +150,30 @@  static int __init extract_lsb_from_nodes(const struct node *nodes,
     return i;
 }
 
-int __init compute_hash_shift(struct node *nodes, int numnodes,
-                              nodeid_t *nodeids)
+int __init compute_memnode_shift(struct node *nodes, unsigned int numnodes,
+                                 nodeid_t *nodeids)
 {
-    int shift;
+    int ret;
+
+    memnode_shift = extract_lsb_from_nodes(nodes, numnodes);
 
-    shift = extract_lsb_from_nodes(nodes, numnodes);
     if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
         memnodemap = _memnodemap;
     else if ( allocate_cachealigned_memnodemap() )
-        return -1;
-    printk(KERN_DEBUG "NUMA: Using %d for the hash shift.\n", shift);
+        return -ENOMEM;
 
-    if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
+    printk(KERN_DEBUG "NUMA: Using %u for the hash shift.\n", memnode_shift);
+
+    ret = populate_memnodemap(nodes, numnodes, memnode_shift, nodeids);
+    if ( ret )
     {
         printk(KERN_INFO "Your memory is not aligned you need to "
                "rebuild your hypervisor with a bigger NODEMAPSIZE "
-               "shift=%d\n", shift);
-        return -1;
+               "shift=%u\n", memnode_shift);
+        return ret;
     }
 
-    return shift;
+    return 0;
 }
 /* initialize NODE_DATA given nodeid and start/end */
 void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end)
@@ -242,8 +246,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);
     }
-    memnode_shift = compute_hash_shift(nodes, numa_fake, NULL);
-    if ( memnode_shift < 0 )
+    if ( compute_memnode_shift(nodes, numa_fake, NULL) )
     {
         memnode_shift = 0;
         printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 209ffc7..535c9d7 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -470,10 +470,9 @@  int __init acpi_scan_nodes(paddr_t start, paddr_t end)
 		return -1;
 	}
 
-	memnode_shift = compute_hash_shift(node_memblk_range, num_node_memblks,
-				memblk_nodeid);
-
-	if (memnode_shift < 0) {
+	if (compute_memnode_shift(node_memblk_range, num_node_memblks,
+				  memblk_nodeid)) {
+		memnode_shift = 0;
 		printk(KERN_ERR
 		     "SRAT: No NUMA node hash function found. Contact maintainer\n");
 		bad_srat();
diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index 5e8474f..1bac25c 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -23,8 +23,8 @@  struct node {
     paddr_t end;
 };
 
-extern int compute_hash_shift(struct node *nodes, int numnodes,
-                              nodeid_t *nodeids);
+extern int compute_memnode_shift(struct node *nodes, unsigned int numnodes,
+                                 nodeid_t *nodeids);
 extern nodeid_t pxm_to_node(unsigned int pxm);
 
 #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT))
@@ -44,7 +44,7 @@  extern nodeid_t apicid_to_node[];
 extern void init_cpu_to_node(void);
 
 /* Simple perfect hash to map pdx to node numbers */
-extern int memnode_shift;
+extern unsigned int memnode_shift;
 extern unsigned long memnodemapsize;
 extern uint8_t *memnodemap;