[libvirt] [PATCH] util: Correct the NUMA node range checking
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 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(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index ab46591..500dca7 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -99,7 +99,6 @@ virNumaSetupMemoryPolicy(virNumaTuneDef numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL; if (numatune.memory.placement_mode == @@ -122,16 +121,13 @@ virNumaSetupMemoryPolicy(virNumaTuneDef numatune, return -1; } - 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) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Nodeset is out of range, host cannot support " - "NUMA node bigger than %d"), bit); + _("NUMA node %d is out of range"), bit); return -1; } nodemask_set(&mask, bit); -- 1.8.1.4
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? ACK if you can fix that up.
virReportError(VIR_ERR_INTERNAL_ERROR, - _("Nodeset is out of range, host cannot support " - "NUMA node bigger than %d"), bit); + _("NUMA node %d is out of range"), bit); return -1; } nodemask_set(&mask, bit);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 23/01/14 08:04, Eric Blake wrote:
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?
Good idea, pushed with the fix, along with a bit change in the commit log. Thanks. Osier
participants (2)
-
Eric Blake -
Osier Yang