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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Jan,

 Sorry for late reply.

On Fri, Jun 30, 2017 at 7:35 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> <vijay.kilari@xxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel