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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> <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 ...

> @@ -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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel