[libvirt] [PATCH 1/1] virsh nodecpustats erroneously returns stats for offline cpus on Linux

[Pradipta Kumar Banerjee]

On 12/19/2013 02:02 PM, Bing Bu Cao wrote:
> On 12/19/2013 12:40 PM, Pradipta Kr. Banerjee wrote:
>> From: "Pradipta Kr. Banerjee" <bpradip at in.ibm.com>
>>
>> virsh nodecpustats erroneously returns stats for offline cpus on Linux
>>
>> To retrieve node cpu statistics on Linux system, the
>> linuxNodeGetCPUstats function performs a minimal match of the input
>> cpuid with the cpuid read from /proc/cpustat. On systems with larger
>> cpus this can result in erroneous data.
>> For eg if /proc/stat has similar data
>>   cpu  3645648 485 361475 3691194868 176595 40645 69850 0 1526172 0
>>   cpu0 494045 62 42763 138516619 16107 2339 2431 0 248345 0
>>   cpu4 402646 59 34745 138617562 17738 1413 1196 0 193948 0
>>   cpu8 318662 32 32119 138715131 7903 927 982 0 115429 0
>>   cpu12 247004 33 30465 138791438 5251 759 913 0 51938 0
>>
>> and cpu 1,2 are offline, then
>>
>> 'virsh nodecpustats 1' displays data for cpu12
>>
>> whereas virsh nodecpustats 2 correctly throws the following error
>>
>> "error: Unable to get node cpu stats
>> error: Invalid cpuNum in linuxNodeGetCPUStats"
>>
>> This patch fixes the above mentioned problem
>>
>> Signed-off-by: Pradipta Kr. Banerjee <bpradip at in.ibm.com>
>> ---
>>   src/nodeinfo.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
>> index 1838547..c9e5ff1 100644
>> --- a/src/nodeinfo.c
>> +++ b/src/nodeinfo.c
>> @@ -614,6 +614,8 @@ linuxNodeGetCPUStats(FILE *procstat,
>>       unsigned long long usr, ni, sys, idle, iowait;
>>       unsigned long long irq, softirq, steal, guest, guest_nice;
>>       char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)];
>> +    char cpu_header_read[3 + INT_BUFSIZE_BOUND(cpuNum)];
>> +    char cpu_header_fmt[8];
>>
>>       if ((*nparams) == 0) {
>>           /* Current number of cpu stats supported by linux */
>> @@ -631,8 +633,11 @@ linuxNodeGetCPUStats(FILE *procstat,
>>
>>       if (cpuNum == VIR_NODE_CPU_STATS_ALL_CPUS) {
>>           strcpy(cpu_header, "cpu");
>> +        snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds", 3);
>>       } else {
>>           snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum);
>> +        snprintf(cpu_header_fmt, sizeof(cpu_header_fmt), "%%%ds",
>> +                 (int)(3 + INT_BUFSIZE_BOUND(cpuNum)));
>>       }
>>
>>       while (fgets(line, sizeof(line), procstat) != NULL) {
>> @@ -649,6 +654,14 @@ linuxNodeGetCPUStats(FILE *procstat,
>>                   continue;
>>               }
>>
>> +            /*
>> +             * Process with stats gathering only if the cpuid from /proc/stat
>> +             * matches exactly with the input cpuid
>> +            */
>> +            sscanf(buf, cpu_header_fmt, cpu_header_read);
>> +            if (STRNEQ(cpu_header, cpu_header_read))
>> +                continue;
>> +
>>               for (i = 0; i < *nparams; i++) {
>>                   virNodeCPUStatsPtr param = &params[i];
> 
> What about this?
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 1838547..aa1ad81 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -637,8 +637,9 @@ linuxNodeGetCPUStats(FILE *procstat,
> 
>       while (fgets(line, sizeof(line), procstat) != NULL) {
>           char *buf = line;
> +        char **buf_header = virStringSplit(buf, " ", 2);
> 
> -        if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */
> +        if (STREQ(buf_header[0], cpu_header)) { /* aka logical CPU time */
>               size_t i;
> 
>               if (sscanf(buf,
> @@ -697,6 +698,7 @@ linuxNodeGetCPUStats(FILE *procstat,
>               ret = 0;
>               goto cleanup;
>           }
> +        virStringFreeList(buf_header);
>       }
> 
>       virReportInvalidArg(cpuNum,
> 
> 
This is definitely better and lesser amount of code..

Thanks
> 
>>
>> -- 
>> 1.8.3.1
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>>
>>
> 


-- 
Regards,
Pradipta