[x265] [PATCH] bug: fix for numa node mask with specifying multiple '+'es. Better log prints

[Steve Borho]

On 09/16, pradeep at multicorewareinc.com wrote:
> # HG changeset patch
> # User Pradeep Ramachandran <pradeep at multicorewareinc.com>
> # Date 1442376246 -19800
> #      Wed Sep 16 09:34:06 2015 +0530
> # Node ID 78456c1eb56dcb2c5f3a4882d176b39cfdaf9fbe
> # Parent  365f7ed4d89628d49cd6af8d81d4edc01f73ffad
> bug: fix for numa node mask with specifying multiple '+'es. Better log prints
> 
> diff -r 365f7ed4d896 -r 78456c1eb56d source/common/threadpool.cpp
> --- a/source/common/threadpool.cpp	Tue Sep 08 16:38:01 2015 +0530
> +++ b/source/common/threadpool.cpp	Wed Sep 16 09:34:06 2015 +0530
> @@ -227,7 +227,7 @@
>      enum { MAX_NODE_NUM = 127 };
>      int cpusPerNode[MAX_NODE_NUM + 1];
>      int threadsPerPool[MAX_NODE_NUM + 2];
> -    uint32_t nodeMaskPerPool[MAX_NODE_NUM +2];
> +    uint64_t nodeMaskPerPool[MAX_NODE_NUM +2];
>  
>      memset(cpusPerNode, 0, sizeof(cpusPerNode));
>      memset(threadsPerPool, 0, sizeof(threadsPerPool));
> @@ -281,20 +281,20 @@
>                  for (int j = i; j < numNumaNodes; j++)
>                  {
>                      threadsPerPool[numNumaNodes] += cpusPerNode[j];
> -                    nodeMaskPerPool[numNumaNodes] |= (1U << j);
> +                    nodeMaskPerPool[numNumaNodes] |= ((uint64_t)1 << j);

fwiw, you could use 1UL, but this is just as good

>                  }
>                  break;
>              }
>              else if (*nodeStr == '+')
>              {
>                  threadsPerPool[numNumaNodes] += cpusPerNode[i];
> -                nodeMaskPerPool[numNumaNodes] = (1U << i);
> +                nodeMaskPerPool[numNumaNodes] |= ((uint64_t)1 << i);
>              }
>              else
>              {
>                  int count = atoi(nodeStr);
>                  threadsPerPool[i] = X265_MIN(count, cpusPerNode[i]);
> -                nodeMaskPerPool[i] = (1U << i);
> +                nodeMaskPerPool[i] = ((uint64_t)1 << i);
>              }
>  
>              /* consume current node string, comma, and white-space */
> @@ -309,7 +309,7 @@
>          for (int i = 0; i < numNumaNodes; i++)
>          {
>              threadsPerPool[numNumaNodes]  += cpusPerNode[i];
> -            nodeMaskPerPool[numNumaNodes] |= (1U << i);
> +            nodeMaskPerPool[numNumaNodes] |= ((uint64_t)1 << i);
>          }
>      }
>   
> @@ -357,7 +357,17 @@
>                  return NULL;
>              }
>              if (numNumaNodes > 1)
> -                x265_log(p, X265_LOG_INFO, "Thread pool %d using %d threads with NUMA node mask %lx\n", i, numThreads, nodeMaskPerPool[node]);
> +            {
> +                char nodes_string[100] = "";

I would be happier if the string size was calculated as 64 * strlen(",63") + 1

> +                for (int j = 0; j < 64; j++)
> +                    if ((nodeMaskPerPool[node]>>j) & 0x1)
> +                    {
> +                        char this_node[10] = "" ;
> +                        sprintf (this_node, ",%d", j);
> +                        strcat (nodes_string, this_node);

w/s

this would be clearer as:

if (numNumaNodes > 1)
{
    char nodesstr[64 * strlen(",63") + 1];
    int len = 0;
    for (int j = 0; j < 64; j++)
       if ((nodeMaskPerPool[node] >> j) & 1)
           len += sprintf(nodestr + len, ",%d", j);
    x265_log(p, X265_LOG_INFO, "Thread pool %d using %d threads on numa nodes %s\n", i, numThreads, nodestr + 1);
}


-- 
Steve Borho