[PATCH v3 0/1] qga: add command 'guest-get-cpustats'

[PATCH v3 0/1] qga: add command 'guest-get-cpustats'

[zhenwei pi]

v2 -> v3:
- Rename 'GuestOsType' to 'GuestCpuStatsType'.
- Add 'linux' into polluted_words, rename 'linuxos' to 'linux'. Remove
  'windows' from 'GuestCpuStatsType', because currently we don't use it.

v1 -> v2:
- Konstantin & Marc-André pointed out that the structure 'GuestCpuStats'
  is too *linux style*, so re-define it to 'GuestLinuxCpuStats', and use
  an union type of 'GuestCpuStats'.

- Modify comment info from 'man proc', also add linux version infomation.

- Test sscanf return value by '(i == EOF)' (To Marc-André: name is declared
  as 'char name[64];', so we can't test '!name').

- Suggested by Marc-André, use 'int clk_tck = sysconf(_SC_CLK_TCK);'
  instead of hard code.

v1:
- Implement guest agent command 'guest-get-cpustats'

Zhenwei Pi (1):
  qga: add command 'guest-get-cpustats'

 qga/commands-posix.c   | 89 ++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c   |  6 +++
 qga/qapi-schema.json   | 81 ++++++++++++++++++++++++++++++++++++++
 scripts/qapi/common.py |  2 +-
 4 files changed, 177 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH v3 1/1] qga: add command 'guest-get-cpustats'

[zhenwei pi]

A vCPU thread always reaches 100% utilization when:
- guest uses idle=poll
- disable HLT vm-exit
- enable MWAIT

Add new guest agent command 'guest-get-cpustats' to get guest CPU
statistics, we can know the guest workload and how busy the CPU is.

To avoid compiling error like:
qga/qga-qapi-types.h:948:28: error: expected member name or ';'
 after declaration specifiers
        GuestLinuxCpuStats linux;
        ~~~~~~~~~~~~~~~~~~ ^
<built-in>:336:15: note: expanded from here

Also add 'linux' into polluted_words.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 qga/commands-posix.c   | 89 ++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c   |  6 +++
 qga/qapi-schema.json   | 81 ++++++++++++++++++++++++++++++++++++++
 scripts/qapi/common.py |  2 +-
 4 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0469dc409d..f18530d85f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2893,6 +2893,90 @@ GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
     return guest_get_diskstats(errp);
 }
 
+GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
+{
+    GuestCpuStatsList *head = NULL, **tail = &head;
+    const char *cpustats = "/proc/stat";
+    int clk_tck = sysconf(_SC_CLK_TCK);
+    FILE *fp;
+    size_t n;
+    char *line = NULL;
+
+    fp = fopen(cpustats, "r");
+    if (fp  == NULL) {
+        error_setg_errno(errp, errno, "open(\"%s\")", cpustats);
+        return NULL;
+    }
+
+    while (getline(&line, &n, fp) != -1) {
+        GuestCpuStats *cpustat = NULL;
+        GuestLinuxCpuStats *linuxcpustat;
+        int i;
+        unsigned long user, system, idle, iowait, irq, softirq, steal, guest;
+        unsigned long nice, guest_nice;
+        char name[64];
+
+        i = sscanf(line, "%s %lu %lu %lu %lu %lu %lu %lu %lu %lu %lu",
+                   name, &user, &nice, &system, &idle, &iowait, &irq, &softirq,
+                   &steal, &guest, &guest_nice);
+
+        /* drop "cpu 1 2 3 ...", get "cpuX 1 2 3 ..." only */
+        if ((i == EOF) || strncmp(name, "cpu", 3) || (name[3] == '\0')) {
+            continue;
+        }
+
+        if (i < 5) {
+            slog("Parsing cpu stat from %s failed, see \"man proc\"", cpustats);
+            break;
+        }
+
+        cpustat = g_new0(GuestCpuStats, 1);
+        cpustat->type = GUEST_CPU_STATS_TYPE_LINUX;
+
+        linuxcpustat = &cpustat->u.q_linux;
+        linuxcpustat->cpu = atoi(&name[3]);
+        linuxcpustat->user = user * 1000 / clk_tck;
+        linuxcpustat->nice = nice * 1000 / clk_tck;
+        linuxcpustat->system = system * 1000 / clk_tck;
+        linuxcpustat->idle = idle * 1000 / clk_tck;
+
+        if (i > 5) {
+            linuxcpustat->has_iowait = true;
+            linuxcpustat->iowait = iowait * 1000 / clk_tck;
+        }
+
+        if (i > 6) {
+            linuxcpustat->has_irq = true;
+            linuxcpustat->irq = irq * 1000 / clk_tck;
+            linuxcpustat->has_softirq = true;
+            linuxcpustat->softirq = softirq * 1000 / clk_tck;
+        }
+
+        if (i > 8) {
+            linuxcpustat->has_steal = true;
+            linuxcpustat->steal = steal * 1000 / clk_tck;
+        }
+
+        if (i > 9) {
+            linuxcpustat->has_guest = true;
+            linuxcpustat->guest = guest * 1000 / clk_tck;
+        }
+
+        if (i > 10) {
+            linuxcpustat->has_guest = true;
+            linuxcpustat->guest = guest * 1000 / clk_tck;
+            linuxcpustat->has_guestnice = true;
+            linuxcpustat->guestnice = guest_nice * 1000 / clk_tck;
+        }
+
+        QAPI_LIST_APPEND(tail, cpustat);
+    }
+
+    free(line);
+    fclose(fp);
+    return head;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **errp)
@@ -3247,6 +3331,11 @@ GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
     return NULL;
 }
 
+GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
 
 #endif /* CONFIG_FSFREEZE */
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 36f94c0f9c..7ed7664715 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2543,3 +2543,9 @@ GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
     error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
+
+GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 9fa20e791b..869399ea1a 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1576,3 +1576,84 @@
 { 'command': 'guest-get-diskstats',
   'returns': ['GuestDiskStatsInfo']
 }
+
+##
+# @GuestCpuStatsType:
+#
+# An enumeration of OS type
+#
+# Since: 7.1
+##
+{ 'enum': 'GuestCpuStatsType',
+  'data': [ 'linux' ] }
+
+
+##
+# @GuestLinuxCpuStats:
+#
+# CPU statistics of Linux
+#
+# @cpu: CPU index in guest OS
+#
+# @user: Time spent in user mode
+#
+# @nice: Time spent in user mode with low priority (nice)
+#
+# @system: Time spent in system mode
+#
+# @idle: Time spent in the idle task
+#
+# @iowait: Time waiting for I/O to complete (since Linux 2.5.41)
+#
+# @irq: Time servicing interrupts (since Linux 2.6.0-test4)
+#
+# @softirq: Time servicing softirqs (since Linux 2.6.0-test4)
+#
+# @steal: Stolen time by host (since Linux 2.6.11)
+#
+# @guest: ime spent running a virtual CPU for guest operating systems under
+#         the  control of the Linux kernel (since Linux 2.6.24)
+#
+# @guestnice: Time spent running a niced guest (since Linux 2.6.33)
+#
+# Since: 7.1
+##
+{ 'struct': 'GuestLinuxCpuStats',
+  'data': {'cpu': 'int',
+           'user': 'uint64',
+           'nice': 'uint64',
+           'system': 'uint64',
+           'idle': 'uint64',
+           '*iowait': 'uint64',
+           '*irq': 'uint64',
+           '*softirq': 'uint64',
+           '*steal': 'uint64',
+           '*guest': 'uint64',
+           '*guestnice': 'uint64'
+           } }
+
+##
+# @GuestCpuStats:
+#
+# Get statistics of each CPU in millisecond.
+#
+# - @linux: Linux style CPU statistics
+#
+# Since: 7.1
+##
+{ 'union': 'GuestCpuStats',
+  'base': { 'type': 'GuestCpuStatsType' },
+  'discriminator': 'type',
+  'data': { 'linux': 'GuestLinuxCpuStats' } }
+
+##
+# @guest-get-cpustats:
+#
+# Retrieve information about CPU stats.
+# Returns: List of CPU stats of guest.
+#
+# Since: 7.1
+##
+{ 'command': 'guest-get-cpustats',
+  'returns': ['GuestCpuStats']
+}
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 489273574a..737b059e62 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -114,7 +114,7 @@ def c_name(name: str, protect: bool = True) -> str:
                      'and', 'and_eq', 'bitand', 'bitor', 'compl', 'not',
                      'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
     # namespace pollution:
-    polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386'])
+    polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386', 'linux'])
     name = re.sub(r'[^A-Za-z0-9_]', '_', name)
     if protect and (name in (c89_words | c99_words | c11_words | gcc_words
                              | cpp_words | polluted_words)
-- 
2.20.1

Re: [PATCH v3 1/1] qga: add command 'guest-get-cpustats'

[Marc-André Lureau]

Hi

On Wed, Jul 6, 2022 at 1:56 PM zhenwei pi <pizhenwei@bytedance.com> wrote:
>
> A vCPU thread always reaches 100% utilization when:
> - guest uses idle=poll
> - disable HLT vm-exit
> - enable MWAIT
>
> Add new guest agent command 'guest-get-cpustats' to get guest CPU
> statistics, we can know the guest workload and how busy the CPU is.
>
> To avoid compiling error like:
> qga/qga-qapi-types.h:948:28: error: expected member name or ';'
>  after declaration specifiers
>         GuestLinuxCpuStats linux;
>         ~~~~~~~~~~~~~~~~~~ ^
> <built-in>:336:15: note: expanded from here
>
> Also add 'linux' into polluted_words.

That looks good to me, although you may want to split the scripts/qapi change.

Markus, what do you think?

>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  qga/commands-posix.c   | 89 ++++++++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c   |  6 +++
>  qga/qapi-schema.json   | 81 ++++++++++++++++++++++++++++++++++++++
>  scripts/qapi/common.py |  2 +-
>  4 files changed, 177 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0469dc409d..f18530d85f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2893,6 +2893,90 @@ GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>      return guest_get_diskstats(errp);
>  }
>
> +GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> +{
> +    GuestCpuStatsList *head = NULL, **tail = &head;
> +    const char *cpustats = "/proc/stat";
> +    int clk_tck = sysconf(_SC_CLK_TCK);
> +    FILE *fp;
> +    size_t n;
> +    char *line = NULL;
> +
> +    fp = fopen(cpustats, "r");
> +    if (fp  == NULL) {
> +        error_setg_errno(errp, errno, "open(\"%s\")", cpustats);
> +        return NULL;
> +    }
> +
> +    while (getline(&line, &n, fp) != -1) {
> +        GuestCpuStats *cpustat = NULL;
> +        GuestLinuxCpuStats *linuxcpustat;
> +        int i;
> +        unsigned long user, system, idle, iowait, irq, softirq, steal, guest;
> +        unsigned long nice, guest_nice;
> +        char name[64];
> +
> +        i = sscanf(line, "%s %lu %lu %lu %lu %lu %lu %lu %lu %lu %lu",
> +                   name, &user, &nice, &system, &idle, &iowait, &irq, &softirq,
> +                   &steal, &guest, &guest_nice);
> +
> +        /* drop "cpu 1 2 3 ...", get "cpuX 1 2 3 ..." only */
> +        if ((i == EOF) || strncmp(name, "cpu", 3) || (name[3] == '\0')) {
> +            continue;
> +        }
> +
> +        if (i < 5) {
> +            slog("Parsing cpu stat from %s failed, see \"man proc\"", cpustats);
> +            break;
> +        }
> +
> +        cpustat = g_new0(GuestCpuStats, 1);
> +        cpustat->type = GUEST_CPU_STATS_TYPE_LINUX;
> +
> +        linuxcpustat = &cpustat->u.q_linux;
> +        linuxcpustat->cpu = atoi(&name[3]);
> +        linuxcpustat->user = user * 1000 / clk_tck;
> +        linuxcpustat->nice = nice * 1000 / clk_tck;
> +        linuxcpustat->system = system * 1000 / clk_tck;
> +        linuxcpustat->idle = idle * 1000 / clk_tck;
> +
> +        if (i > 5) {
> +            linuxcpustat->has_iowait = true;
> +            linuxcpustat->iowait = iowait * 1000 / clk_tck;
> +        }
> +
> +        if (i > 6) {
> +            linuxcpustat->has_irq = true;
> +            linuxcpustat->irq = irq * 1000 / clk_tck;
> +            linuxcpustat->has_softirq = true;
> +            linuxcpustat->softirq = softirq * 1000 / clk_tck;
> +        }
> +
> +        if (i > 8) {
> +            linuxcpustat->has_steal = true;
> +            linuxcpustat->steal = steal * 1000 / clk_tck;
> +        }
> +
> +        if (i > 9) {
> +            linuxcpustat->has_guest = true;
> +            linuxcpustat->guest = guest * 1000 / clk_tck;
> +        }
> +
> +        if (i > 10) {
> +            linuxcpustat->has_guest = true;
> +            linuxcpustat->guest = guest * 1000 / clk_tck;
> +            linuxcpustat->has_guestnice = true;
> +            linuxcpustat->guestnice = guest_nice * 1000 / clk_tck;
> +        }
> +
> +        QAPI_LIST_APPEND(tail, cpustat);
> +    }
> +
> +    free(line);
> +    fclose(fp);
> +    return head;
> +}
> +
>  #else /* defined(__linux__) */
>
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -3247,6 +3331,11 @@ GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>      return NULL;
>  }
>
> +GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
>
>  #endif /* CONFIG_FSFREEZE */
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 36f94c0f9c..7ed7664715 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2543,3 +2543,9 @@ GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>      error_setg(errp, QERR_UNSUPPORTED);
>      return NULL;
>  }
> +
> +GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 9fa20e791b..869399ea1a 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1576,3 +1576,84 @@
>  { 'command': 'guest-get-diskstats',
>    'returns': ['GuestDiskStatsInfo']
>  }
> +
> +##
> +# @GuestCpuStatsType:
> +#
> +# An enumeration of OS type
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'GuestCpuStatsType',
> +  'data': [ 'linux' ] }
> +
> +
> +##
> +# @GuestLinuxCpuStats:
> +#
> +# CPU statistics of Linux
> +#
> +# @cpu: CPU index in guest OS
> +#
> +# @user: Time spent in user mode
> +#
> +# @nice: Time spent in user mode with low priority (nice)
> +#
> +# @system: Time spent in system mode
> +#
> +# @idle: Time spent in the idle task
> +#
> +# @iowait: Time waiting for I/O to complete (since Linux 2.5.41)
> +#
> +# @irq: Time servicing interrupts (since Linux 2.6.0-test4)
> +#
> +# @softirq: Time servicing softirqs (since Linux 2.6.0-test4)
> +#
> +# @steal: Stolen time by host (since Linux 2.6.11)
> +#
> +# @guest: ime spent running a virtual CPU for guest operating systems under
> +#         the  control of the Linux kernel (since Linux 2.6.24)
> +#
> +# @guestnice: Time spent running a niced guest (since Linux 2.6.33)
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'GuestLinuxCpuStats',
> +  'data': {'cpu': 'int',
> +           'user': 'uint64',
> +           'nice': 'uint64',
> +           'system': 'uint64',
> +           'idle': 'uint64',
> +           '*iowait': 'uint64',
> +           '*irq': 'uint64',
> +           '*softirq': 'uint64',
> +           '*steal': 'uint64',
> +           '*guest': 'uint64',
> +           '*guestnice': 'uint64'
> +           } }
> +
> +##
> +# @GuestCpuStats:
> +#
> +# Get statistics of each CPU in millisecond.
> +#
> +# - @linux: Linux style CPU statistics
> +#
> +# Since: 7.1
> +##
> +{ 'union': 'GuestCpuStats',
> +  'base': { 'type': 'GuestCpuStatsType' },
> +  'discriminator': 'type',
> +  'data': { 'linux': 'GuestLinuxCpuStats' } }
> +
> +##
> +# @guest-get-cpustats:
> +#
> +# Retrieve information about CPU stats.
> +# Returns: List of CPU stats of guest.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'guest-get-cpustats',
> +  'returns': ['GuestCpuStats']
> +}
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 489273574a..737b059e62 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -114,7 +114,7 @@ def c_name(name: str, protect: bool = True) -> str:
>                       'and', 'and_eq', 'bitand', 'bitor', 'compl', 'not',
>                       'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
>      # namespace pollution:
> -    polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386'])
> +    polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386', 'linux'])
>      name = re.sub(r'[^A-Za-z0-9_]', '_', name)
>      if protect and (name in (c89_words | c99_words | c11_words | gcc_words
>                               | cpp_words | polluted_words)
> --
> 2.20.1
>

Re: [PATCH v3 1/1] qga: add command 'guest-get-cpustats'

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Wed, Jul 6, 2022 at 1:56 PM zhenwei pi <pizhenwei@bytedance.com> wrote:
>>
>> A vCPU thread always reaches 100% utilization when:
>> - guest uses idle=poll
>> - disable HLT vm-exit
>> - enable MWAIT
>>
>> Add new guest agent command 'guest-get-cpustats' to get guest CPU
>> statistics, we can know the guest workload and how busy the CPU is.
>>
>> To avoid compiling error like:
>> qga/qga-qapi-types.h:948:28: error: expected member name or ';'
>>  after declaration specifiers
>>         GuestLinuxCpuStats linux;
>>         ~~~~~~~~~~~~~~~~~~ ^
>> <built-in>:336:15: note: expanded from here

What actually happens here: the compiler predefines macro linux like

    #define linux 1

>> Also add 'linux' into polluted_words.

Does polluted_words cover all of the predefined macros then?
Checking...

    $ gcc -E -dD - </dev/null 2>&1 | grep 'define [^_]'
    #define linux 1
    #define unix 1

Yes.  We don't care about the ones starting with '_'.

> That looks good to me, although you may want to split the scripts/qapi change.
>
> Markus, what do you think?

I'd very much prefer a separate patch.  Suggested commit message:

    qapi: Avoid generating C identifier 'linux'

    'linux' is not usable as identifier, because C compilers targeting
    Linux predefine it as a macro expanding to 1.  Add it to
    @polluted_words.  'unix' is already there.

>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

[...]

>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 489273574a..737b059e62 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -114,7 +114,7 @@ def c_name(name: str, protect: bool = True) -> str:
>>                       'and', 'and_eq', 'bitand', 'bitor', 'compl', 'not',
>>                       'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
>>      # namespace pollution:
>> -    polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386'])
>> +    polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386', 'linux'])
>>      name = re.sub(r'[^A-Za-z0-9_]', '_', name)
>>      if protect and (name in (c89_words | c99_words | c11_words | gcc_words
>>                               | cpp_words | polluted_words)
>> --
>> 2.20.1
>>