Re: [libvirt] [PATCH] util: Correct the NUMA node range checking

On 01/22/2014 04:45 AM, Osier Yang wrote: > There are 2 issues here: First we shouldn't add "1" to the return > value of numa_max_node(), since the semanteme of the error message s/semanteme/semantics/ > was changed, it's not saying about the number of total NUMA nodes > anymore. Second, the value of "bit" is the position of the first > bit which exceeds either numa_max_node() or NUMA_NUM_NODES, it can > be any number in the range, so saying "bigger than $bit" is quite > confused now. For example, assuming there is a NUMA machine which > has 10 NUMA nodes, and one specifies the "nodeset" as "0,5,88", > the error message will be like: > > Nodeset is out of range, host cannot support NUMA node bigger than 88 > > It sounds like all NUMA node number less than 88 is fine, but > actually the maximum NUMA node number the machine supports is 9. > > This patch fixes the issues by removing the addition with "1" and > simplifies the error message as "NUMA node $bit is out of range". > --- > src/util/virnuma.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > - maxnode = numa_max_node() + 1; > - > /* Convert nodemask to NUMA bitmask. */ > nodemask_zero(&mask); > bit = -1; > while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { > - if (bit > maxnode || bit > NUMA_NUM_NODES) { > + if (bit > numa_max_node() || bit > NUMA_NUM_NODES) { This calls numa_max_node() in a loop, where we used to call it just once. Since this is third-party code, do we know how efficient it is? It may be smarter to cache the results once than to call every iteration of the loop, to avoid O(n*2) behavior on large hosts. For that matter, can't we just set the max node to the smaller of numa_max_node() and NUMA_NUM_NODES up front, and avoid the || in the loop?