On 06/16/2015 05:38 AM, Kothapally Madhu Pavan wrote:

Currently we are parsing all the possible cpus to get the nodeinfo. This fix will perform a check for present cpus before parsing.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- src/nodeinfo.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2fafe2d..0134aba 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -57,6 +57,7 @@ #define VIR_FROM_THIS VIR_FROM_NONE

VIR_LOG_INIT("nodeinfo"); +virBitmapPtr nodeGetPresentCPUBitmap(void);

^^^ Didn't notice this the first time - if you had #include "nodeinfo.h", then this is unnecessary

#if defined(__FreeBSD__) || defined(__APPLE__) static int @@ -418,6 +419,7 @@ virNodeParseNode(const char *node, int processors = 0; DIR *cpudir = NULL; struct dirent *cpudirent = NULL; + virBitmapPtr present_cpumap = NULL; int sock_max = 0; cpu_set_t sock_map; int sock; @@ -438,12 +440,18 @@ virNodeParseNode(const char *node, goto cleanup; }

+ present_cpumap = nodeGetPresentCPUBitmap(); + /* enumerate sockets in the node */ CPU_ZERO(&sock_map); while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;

+ if (present_cpumap) + if (!(virBitmapIsBitSet(present_cpumap, cpu)))

if (present_cpumap && !(virBitmapIsSet(present_cpumap, cpu)))

+ continue; + if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0) goto cleanup;

Expanding a bit below here one finds if (!online) continue;

@@ -477,6 +485,10 @@ virNodeParseNode(const char *node, if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue;

+ if (present_cpumap) + if (!(virBitmapIsBitSet(present_cpumap, cpu)))

if (present_cpumap && !(virBitmapIsSet(present_cpumap, cpu)))

+ continue; + if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0) goto cleanup;

Expanding a bit below here one finds if (!online) { (*offline)++; continue; } What's not clear to me is what you're trying to accomplish by avoiding a CPU in this second loop since it appears from my reading that there is a desire to know the offline count - that is the set of CPU's you're perhaps trying to avoid by non including present CPU's? Perhaps better rephrased.... The nodeGetPresentCPUBitmap() returns a bitmap of "/sys/devices/system/cpu/present". Does the directory structure in /sys/devices/system/node/nodeN/cpuM" still exist for CPU's that aren't within the 'present' bitmap? If so, then wouldn't this second loop need to be able to mark/count those offline cpu's? Perhaps would be nice to have a test that could be run before and after the patch to ensure no "functionality" is lost here. Obviously my "knowledge" of this cpu/node filesystem is lacking, but what's being done in this code seems to me to perhaps want to know about those not present cpu's. Since both loops continue back to the top once a CPU directory either doesn't have the "online" file or it contains a 0 in the file, so other than perhaps a slight optimization in the first loop to not look at the online file, what is the value gained for this patch? What problem are you trying to solve. John

@@ -537,6 +549,7 @@ virNodeParseNode(const char *node, ret = -1; } VIR_FREE(core_maps); + virBitmapFree(present_cpumap);

return ret; }

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list