Re: [Xen-devel] [RFC PATCH v2 03/25] x86: NUMA: Rename and sanitize some common functions

[Vijay Kilari]

Hi Jan,

 Sorry for late reply.

On Fri, Jun 30, 2017 at 7:35 PM, Jan Beulich  wrote:
  03/28/17 5:54 PM >>>
>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -53,15 +53,15 @@ 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
>> + * -EINVAL if memnodmap[] too small (of shift too small)
>> + * OR if node overlap or lost ram (shift too big)
>
> It may not matter too much, but you're making things actually worse to
> the caller, as it now can't distinguish the two failure modes anymore.
> Also, if you already touch it, please also correct the apparent typo
> ("of" quite likely meant to be "or"). But what I consider most problematic
> is that you convert ...

OK. I propose to return different error values for two failure modes.
-ENOMEM for "if memnodmap[] too small" and
-EINVAL for if node overlap or lost ram

But In any case it does not matter much and can drop this change.

> ... what is an error case so far to a success one.
>
>> @@ -116,10 +116,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,
>> +  int numnodes)
>
> Why would you convert the return type to unsigned, but not also that of the
> bogusly signed parameter?

Because memnode_shift type is changed from int to unsigned int.
The return type is changed.

I will change int parameter to unsigned int.
Apart from that I see that variable 'i' in extract_lsb_from_nodes() is int.
This needs to changed to unsigned int.

>
>> @@ -143,27 +143,27 @@ 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, int numnodes,
>> + nodeid_t *nodeids, unsigned int *shift)
>
> I'm not in favor of returning the shift count via pointer when it can easily
> be returned by value.

OK.

>
>>  {
>> -int shift;
>> +*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;
>> +
>> +printk(KERN_DEBUG "NUMA: Using %u for the hash shift.\n", *shift);
>>
>> -if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
>> +if ( populate_memnodemap(nodes, numnodes, *shift, nodeids) )
>>  {
>>  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", *shift);
>> +return -EINVAL;
>
> So you make populate_memnodemap() return proper error values, but then discard
> it and uniformly use -EINVAL here. If you mean the function to simply return a
> success/failure indicator, make it return bool. Otherwise use the error value
> it return (even if right now it can only ever be -EINVAL).

OK. I will drop this change and keep compute_hash_shift() return -1 or
shift value.

Regards
Vijay

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

Re: [Xen-devel] [RFC PATCH v2 03/25] x86: NUMA: Rename and sanitize some common functions

[Jan Beulich]

>>>  03/28/17 5:54 PM >>>
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -53,15 +53,15 @@ 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
> + * -EINVAL if memnodmap[] too small (of shift too small)
> + * OR if node overlap or lost ram (shift too big)

It may not matter too much, but you're making things actually worse to
the caller, as it now can't distinguish the two failure modes anymore.
Also, if you already touch it, please also correct the apparent typo
("of" quite likely meant to be "or"). But what I consider most problematic
is that you convert ...

> @@ -74,7 +74,7 @@ static int __init populate_memnodemap(const struct node 
> *nodes,
>  return 0;

... what is an error case so far to a success one.

> @@ -116,10 +116,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,
> +  int numnodes)

Why would you convert the return type to unsigned, but not also that of the
bogusly signed parameter?

> @@ -143,27 +143,27 @@ 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, int numnodes,
> + nodeid_t *nodeids, unsigned int *shift)

I'm not in favor of returning the shift count via pointer when it can easily
be returned by value.

>  {
> -int shift;
> +*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;
> +
> +printk(KERN_DEBUG "NUMA: Using %u for the hash shift.\n", *shift);
>  
> -if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
> +if ( populate_memnodemap(nodes, numnodes, *shift, nodeids) )
>  {
>  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", *shift);
> +return -EINVAL;

So you make populate_memnodemap() return proper error values, but then discard
it and uniformly use -EINVAL here. If you mean the function to simply return a
success/failure indicator, make it return bool. Otherwise use the error value
it return (even if right now it can only ever be -EINVAL).

Jan

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

[Xen-devel] [RFC PATCH v2 03/25] x86: NUMA: Rename and sanitize some common functions

[vijay . kilari]

From: Vijaya Kumar K 

Following changes are made
 - Rename compute_hash_shift to compute_memnode_shift
   and return error values instead of shift.
 - Changes prototypes of populate_memnodemap()
   and extract_lsb_from_nodes()

Signed-off-by: Vijaya Kumar K 
---
 xen/arch/x86/numa.c| 47 +++---
 xen/arch/x86/srat.c|  7 +++
 xen/include/asm-x86/numa.h |  4 ++--
 3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 8ed31cb..964fc5a 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -53,15 +53,15 @@ 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
+ * -EINVAL if memnodmap[] too small (of shift too small)
+ * OR 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)
+static int __init populate_memnodemap(const struct node *nodes, 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++ )
@@ -74,7 +74,7 @@ static int __init populate_memnodemap(const struct node 
*nodes,
 return 0;
 do {
 if ( memnodemap[spdx >> shift] != NUMA_NO_NODE )
-return -1;
+return -EINVAL;
 
 if ( !nodeids )
 memnodemap[spdx >> shift] = i;
@@ -83,7 +83,7 @@ static int __init populate_memnodemap(const struct node 
*nodes,
 
 spdx += (1UL << shift);
 } while ( spdx < epdx );
-res = 1;
+res = 0;
 }
 
 return res;
@@ -99,7 +99,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);
@@ -116,10 +116,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,
+  int numnodes)
 {
-int i, nodes_used = 0;
+unsigned int i, nodes_used = 0;
 unsigned long spdx, epdx;
 unsigned long bitfield = 0, memtop = 0;
 
@@ -143,27 +143,27 @@ 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, int numnodes,
+ nodeid_t *nodeids, unsigned int *shift)
 {
-int shift;
+*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;
+
+printk(KERN_DEBUG "NUMA: Using %u for the hash shift.\n", *shift);
 
-if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
+if ( populate_memnodemap(nodes, numnodes, *shift, nodeids) )
 {
 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", *shift);
+return -EINVAL;
 }
 
-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)
@@ -235,8 +235,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, _shift) )
 {
 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 800a7c3..2d0c047 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -470,10 +470,9 @@ int __init