diff mbox

[v7,7/7] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state

Message ID 146701876594.26946.9446070273481744487.stgit@fimbulvetr.bsc.es
State New
Headers show

Commit Message

Lluís Vilanova June 27, 2016, 9:12 a.m. UTC
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hmp-commands-info.hx |    6 +-
 hmp-commands.hx      |    7 +-
 monitor.c            |   17 +++++-
 qapi/trace.json      |   32 +++++++++--
 qmp-commands.hx      |   35 +++++++++++-
 trace/qmp.c          |  148 ++++++++++++++++++++++++++++++++++++++++----------
 6 files changed, 202 insertions(+), 43 deletions(-)

Comments

Markus Armbruster June 30, 2016, 4:03 p.m. UTC | #1
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hmp-commands-info.hx |    6 +-
>  hmp-commands.hx      |    7 +-
>  monitor.c            |   17 +++++-
>  qapi/trace.json      |   32 +++++++++--
>  qmp-commands.hx      |   35 +++++++++++-
>  trace/qmp.c          |  148 ++++++++++++++++++++++++++++++++++++++++----------
>  6 files changed, 202 insertions(+), 43 deletions(-)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 3d07ca6..74446c6 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -646,10 +646,10 @@ ETEXI
>  
>      {
>          .name       = "trace-events",
> -        .args_type  = "name:s?",
> -        .params     = "[name]",
> +        .args_type  = "name:s?,vcpu:i?",
> +        .params     = "[name] [vcpu]",
>          .help       = "show available trace-events & their state "
> -                      "(name: event name pattern)",
> +                      "(name: event name pattern; vcpu: vCPU to query, default is any)",
>          .mhandler.cmd = hmp_info_trace_events,
>          .command_completion = info_trace_events_completion,
>      },
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 98b4b1a..848efee 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -281,9 +281,10 @@ ETEXI
>  
>      {
>          .name       = "trace-event",
> -        .args_type  = "name:s,option:b",
> -        .params     = "name on|off",
> -        .help       = "changes status of a specific trace event",
> +        .args_type  = "name:s,option:b,vcpu:i?",
> +        .params     = "name on|off [vcpu]",
> +        .help       = "changes status of a specific trace event "
> +                      "(vcpu: vCPU to set, default is all)",
>          .mhandler.cmd = hmp_trace_event,
>          .command_completion = trace_event_completion,
>      },
> diff --git a/monitor.c b/monitor.c
> index 7bd0f32..91a377b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -908,9 +908,16 @@ static void hmp_trace_event(Monitor *mon, const QDict *qdict)
>  {
>      const char *tp_name = qdict_get_str(qdict, "name");
>      bool new_state = qdict_get_bool(qdict, "option");
> +    int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
>      Error *local_err = NULL;
>  
> -    qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
> +    if (vcpu < -1) {
> +        /* some user-provided negative number */
> +        monitor_printf(mon, "argument vcpu must be positive");
> +        return;

If -2 is not okay, why is -1 okay?

What about:

    bool has_vcpu = qdict_haskey(qdict, "vcpu");
    int vcpu = qdict_get_try_int(qdict, "vcpu", 0);

    if (has_vcpu && vcpu < 0) {

> +    }
> +
> +    qmp_trace_event_set_state(tp_name, new_state, true, true, vcpu != -1, vcpu, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
>      }
> @@ -1070,6 +1077,7 @@ static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
>  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>  {
>      const char *name = qdict_get_try_str(qdict, "name");
> +    int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
>      TraceEventInfoList *events;
>      TraceEventInfoList *elem;
>      Error *local_err = NULL;
> @@ -1077,8 +1085,13 @@ static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>      if (name == NULL) {
>          name = "*";
>      }
> +    if (vcpu < -1) {
> +        /* some user-provided negative number */
> +        monitor_printf(mon, "argument vcpu must be positive");
> +        return;

Likewise.

> +    }
>  
> -    events = qmp_trace_event_get_state(name, &local_err);
> +    events = qmp_trace_event_get_state(name, vcpu != -1, vcpu, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
>          return;
[...]
Lluís Vilanova June 30, 2016, 5:39 p.m. UTC | #2
Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> hmp-commands-info.hx |    6 +-
>> hmp-commands.hx      |    7 +-
>> monitor.c            |   17 +++++-
>> qapi/trace.json      |   32 +++++++++--
>> qmp-commands.hx      |   35 +++++++++++-
>> trace/qmp.c          |  148 ++++++++++++++++++++++++++++++++++++++++----------
>> 6 files changed, 202 insertions(+), 43 deletions(-)
>> 
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index 3d07ca6..74446c6 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -646,10 +646,10 @@ ETEXI
>> 
>> {
>> .name       = "trace-events",
>> -        .args_type  = "name:s?",
>> -        .params     = "[name]",
>> +        .args_type  = "name:s?,vcpu:i?",
>> +        .params     = "[name] [vcpu]",
>> .help       = "show available trace-events & their state "
>> -                      "(name: event name pattern)",
>> +                      "(name: event name pattern; vcpu: vCPU to query, default is any)",
>> .mhandler.cmd = hmp_info_trace_events,
>> .command_completion = info_trace_events_completion,
>> },
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 98b4b1a..848efee 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -281,9 +281,10 @@ ETEXI
>> 
>> {
>> .name       = "trace-event",
>> -        .args_type  = "name:s,option:b",
>> -        .params     = "name on|off",
>> -        .help       = "changes status of a specific trace event",
>> +        .args_type  = "name:s,option:b,vcpu:i?",
>> +        .params     = "name on|off [vcpu]",
>> +        .help       = "changes status of a specific trace event "
>> +                      "(vcpu: vCPU to set, default is all)",
>> .mhandler.cmd = hmp_trace_event,
>> .command_completion = trace_event_completion,
>> },
>> diff --git a/monitor.c b/monitor.c
>> index 7bd0f32..91a377b 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -908,9 +908,16 @@ static void hmp_trace_event(Monitor *mon, const QDict *qdict)
>> {
>> const char *tp_name = qdict_get_str(qdict, "name");
>> bool new_state = qdict_get_bool(qdict, "option");
>> +    int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
>> Error *local_err = NULL;
>> 
>> -    qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
>> +    if (vcpu < -1) {
>> +        /* some user-provided negative number */
>> +        monitor_printf(mon, "argument vcpu must be positive");
>> +        return;

> If -2 is not okay, why is -1 okay?

> What about:

>     bool has_vcpu = qdict_haskey(qdict, "vcpu");
>     int vcpu = qdict_get_try_int(qdict, "vcpu", 0);

>     if (has_vcpu && vcpu < 0) {
[...]

I was trying to differentiate between the default value (-1) and other
user-provided negative values. I'll change it to use qdict_haskey() (I wasn't
aware of it).

Thanks,
  Lluis
diff mbox

Patch

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 3d07ca6..74446c6 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -646,10 +646,10 @@  ETEXI
 
     {
         .name       = "trace-events",
-        .args_type  = "name:s?",
-        .params     = "[name]",
+        .args_type  = "name:s?,vcpu:i?",
+        .params     = "[name] [vcpu]",
         .help       = "show available trace-events & their state "
-                      "(name: event name pattern)",
+                      "(name: event name pattern; vcpu: vCPU to query, default is any)",
         .mhandler.cmd = hmp_info_trace_events,
         .command_completion = info_trace_events_completion,
     },
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 98b4b1a..848efee 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -281,9 +281,10 @@  ETEXI
 
     {
         .name       = "trace-event",
-        .args_type  = "name:s,option:b",
-        .params     = "name on|off",
-        .help       = "changes status of a specific trace event",
+        .args_type  = "name:s,option:b,vcpu:i?",
+        .params     = "name on|off [vcpu]",
+        .help       = "changes status of a specific trace event "
+                      "(vcpu: vCPU to set, default is all)",
         .mhandler.cmd = hmp_trace_event,
         .command_completion = trace_event_completion,
     },
diff --git a/monitor.c b/monitor.c
index 7bd0f32..91a377b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -908,9 +908,16 @@  static void hmp_trace_event(Monitor *mon, const QDict *qdict)
 {
     const char *tp_name = qdict_get_str(qdict, "name");
     bool new_state = qdict_get_bool(qdict, "option");
+    int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
     Error *local_err = NULL;
 
-    qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
+    if (vcpu < -1) {
+        /* some user-provided negative number */
+        monitor_printf(mon, "argument vcpu must be positive");
+        return;
+    }
+
+    qmp_trace_event_set_state(tp_name, new_state, true, true, vcpu != -1, vcpu, &local_err);
     if (local_err) {
         error_report_err(local_err);
     }
@@ -1070,6 +1077,7 @@  static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
 static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
 {
     const char *name = qdict_get_try_str(qdict, "name");
+    int vcpu = qdict_get_try_int(qdict, "vcpu", -1);
     TraceEventInfoList *events;
     TraceEventInfoList *elem;
     Error *local_err = NULL;
@@ -1077,8 +1085,13 @@  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
     if (name == NULL) {
         name = "*";
     }
+    if (vcpu < -1) {
+        /* some user-provided negative number */
+        monitor_printf(mon, "argument vcpu must be positive");
+        return;
+    }
 
-    events = qmp_trace_event_get_state(name, &local_err);
+    events = qmp_trace_event_get_state(name, vcpu != -1, vcpu, &local_err);
     if (local_err) {
         error_report_err(local_err);
         return;
diff --git a/qapi/trace.json b/qapi/trace.json
index 01b0a52..2123a46 100644
--- a/qapi/trace.json
+++ b/qapi/trace.json
@@ -1,6 +1,6 @@ 
 # -*- mode: python -*-
 #
-# Copyright (C) 2011-2014 Lluís Vilanova <vilanova@ac.upc.edu>
+# Copyright (C) 2011-2016 Lluís Vilanova <vilanova@ac.upc.edu>
 #
 # This work is licensed under the terms of the GNU GPL, version 2 or later.
 # See the COPYING file in the top-level directory.
@@ -29,11 +29,14 @@ 
 #
 # @name: Event name.
 # @state: Tracing state.
+# @vcpu: Whether this is a per-vCPU event (since 2.7).
+#
+# An event is per-vCPU if it has the "vcpu" property in the "trace-events" file.
 #
 # Since 2.2
 ##
 { 'struct': 'TraceEventInfo',
-  'data': {'name': 'str', 'state': 'TraceEventState'} }
+  'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} }
 
 ##
 # @trace-event-get-state:
@@ -41,13 +44,23 @@ 
 # Query the state of events.
 #
 # @name: Event name pattern (case-sensitive glob).
+# @vcpu: #optional The vCPU to query (any by default; since 2.7).
 #
 # Returns: a list of @TraceEventInfo for the matching events
 #
+# An event is returned if:
+# - its name matches the @name pattern, and
+# - if @vcpu is given, the event has the "vcpu" property.
+#
+# Therefore, if @vcpu is given, the operation will only match per-vCPU events,
+# returning their state on the specified vCPU. Special case: if @name is an
+# exact match, @vcpu is given and the event does not have the "vcpu" property,
+# an error is returned.
+#
 # Since 2.2
 ##
 { 'command': 'trace-event-get-state',
-  'data': {'name': 'str'},
+  'data': {'name': 'str', '*vcpu': 'int'},
   'returns': ['TraceEventInfo'] }
 
 ##
@@ -58,8 +71,19 @@ 
 # @name: Event name pattern (case-sensitive glob).
 # @enable: Whether to enable tracing.
 # @ignore-unavailable: #optional Do not match unavailable events with @name.
+# @vcpu: #optional The vCPU to act upon (all by default; since 2.7).
+#
+# An event's state is modified if:
+# - its name matches the @name pattern, and
+# - if @vcpu is given, the event has the "vcpu" property.
+#
+# Therefore, if @vcpu is given, the operation will only match per-vCPU events,
+# setting their state on the specified vCPU. Special case: if @name is an exact
+# match, @vcpu is given and the event does not have the "vcpu" property, an
+# error is returned.
 #
 # Since 2.2
 ##
 { 'command': 'trace-event-set-state',
-  'data': {'name': 'str', 'enable': 'bool', '*ignore-unavailable': 'bool'} }
+  'data': {'name': 'str', 'enable': 'bool', '*ignore-unavailable': 'bool',
+           '*vcpu': 'int'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b444c20..8fbee65 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4703,7 +4703,7 @@  EQMP
 
     {
         .name       = "trace-event-get-state",
-        .args_type  = "name:s",
+        .args_type  = "name:s,vcpu:i?",
         .mhandler.cmd_new = qmp_marshal_trace_event_get_state,
     },
 
@@ -4713,6 +4713,20 @@  trace-event-get-state
 
 Query the state of events.
 
+Arguments:
+
+- "name": Event name pattern (json-string).
+- "vcpu": The vCPU to query, any vCPU by default (json-int, optional).
+
+An event is returned if:
+- its name matches the "name" pattern, and
+- if "vcpu" is given, the event has the "vcpu" property.
+
+Therefore, if "vcpu" is given, the operation will only match per-vCPU events,
+returning their state on the specified vCPU. Special case: if "name" is an exact
+match, "vcpu" is given and the event does not have the "vcpu" property, an error
+is returned.
+
 Example:
 
 -> { "execute": "trace-event-get-state", "arguments": { "name": "qemu_memalign" } }
@@ -4721,7 +4735,7 @@  EQMP
 
     {
         .name       = "trace-event-set-state",
-        .args_type  = "name:s,enable:b,ignore-unavailable:b?",
+        .args_type  = "name:s,enable:b,ignore-unavailable:b?,vcpu:i?",
         .mhandler.cmd_new = qmp_marshal_trace_event_set_state,
     },
 
@@ -4731,6 +4745,23 @@  trace-event-set-state
 
 Set the state of events.
 
+Arguments:
+
+- "name": Event name pattern (json-string).
+- "enable": Whether to enable or disable the event (json-bool).
+- "ignore-unavailable": Whether to ignore errors for events that cannot be
+  changed (json-bool, optional).
+- "vcpu": The vCPU to act upon, all vCPUs by default (json-int, optional).
+
+An event's state is modified if:
+- its name matches the "name" pattern, and
+- if "vcpu" is given, the event has the "vcpu" property.
+
+Therefore, if "vcpu" is given, the operation will only match per-vCPU events,
+setting their state on the specified vCPU. Special case: if "name" is an exact
+match, "vcpu" is given and the event does not have the "vcpu" property, an error
+is returned.
+
 Example:
 
 -> { "execute": "trace-event-set-state", "arguments": { "name": "qemu_memalign", "enable": "true" } }
diff --git a/trace/qmp.c b/trace/qmp.c
index 8aa2660..11d2564 100644
--- a/trace/qmp.c
+++ b/trace/qmp.c
@@ -1,7 +1,7 @@ 
 /*
  * QMP commands for tracing events.
  *
- * Copyright (C) 2014 Lluís Vilanova <vilanova@ac.upc.edu>
+ * Copyright (C) 2014-2016 Lluís Vilanova <vilanova@ac.upc.edu>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -12,63 +12,153 @@ 
 #include "trace/control.h"
 
 
-TraceEventInfoList *qmp_trace_event_get_state(const char *name, Error **errp)
+static CPUState *get_cpu(bool has_vcpu, int vcpu, Error **errp)
 {
+    if (has_vcpu) {
+        CPUState *cpu = qemu_get_cpu(vcpu);
+        if (cpu == NULL) {
+            error_setg(errp, "invalid vCPU index %u", vcpu);
+        }
+        return cpu;
+    } else {
+        return NULL;
+    }
+}
+
+static bool check_events(bool has_vcpu, bool ignore_unavailable, bool is_pattern,
+                         const char *name, Error **errp)
+{
+    if (!is_pattern) {
+        TraceEvent *ev = trace_event_name(name);
+
+        /* error for non-existing event */
+        if (ev == NULL) {
+            error_setg(errp, "unknown event \"%s\"", name);
+            return false;
+        }
+
+        /* error for non-vcpu event */
+        if (has_vcpu && !trace_event_is_vcpu(ev)) {
+            error_setg(errp, "event \"%s\" is not vCPU-specific", name);
+            return false;
+        }
+
+        /* error for unavailable event */
+        if (!ignore_unavailable && !trace_event_get_state_static(ev)) {
+            error_setg(errp, "event \"%s\" is disabled", name);
+            return false;
+        }
+
+        return true;
+    } else {
+        /* error for unavailable events */
+        TraceEvent *ev = NULL;
+        while ((ev = trace_event_pattern(name, ev)) != NULL) {
+            if (!ignore_unavailable && !trace_event_get_state_static(ev)) {
+                error_setg(errp, "event \"%s\" is disabled", trace_event_get_name(ev));
+                return false;
+            }
+        }
+        return true;
+    }
+}
+
+TraceEventInfoList *qmp_trace_event_get_state(const char *name,
+                                              bool has_vcpu, int64_t vcpu,
+                                              Error **errp)
+{
+    Error *err = NULL;
     TraceEventInfoList *events = NULL;
-    bool found = false;
     TraceEvent *ev;
+    bool is_pattern = trace_event_is_pattern(name);
+    CPUState *cpu;
 
+    /* Check provided vcpu */
+    cpu = get_cpu(has_vcpu, vcpu, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return NULL;
+    }
+
+    /* Check events */
+    if (!check_events(has_vcpu, true, is_pattern, name, errp)) {
+        return NULL;
+    }
+
+    /* Get states (all errors checked above) */
     ev = NULL;
     while ((ev = trace_event_pattern(name, ev)) != NULL) {
-        TraceEventInfoList *elem = g_new(TraceEventInfoList, 1);
+        TraceEventInfoList *elem;
+        bool is_vcpu = trace_event_is_vcpu(ev);
+        if (has_vcpu && !is_vcpu) {
+            continue;
+        }
+
+        elem = g_new(TraceEventInfoList, 1);
         elem->value = g_new(TraceEventInfo, 1);
+        elem->value->vcpu = is_vcpu;
         elem->value->name = g_strdup(trace_event_get_name(ev));
+
         if (!trace_event_get_state_static(ev)) {
             elem->value->state = TRACE_EVENT_STATE_UNAVAILABLE;
-        } else if (!trace_event_get_state_dynamic(ev)) {
-            elem->value->state = TRACE_EVENT_STATE_DISABLED;
         } else {
-            elem->value->state = TRACE_EVENT_STATE_ENABLED;
+            if (has_vcpu) {
+                if (is_vcpu) {
+                    if (trace_event_get_vcpu_state_dynamic(cpu, ev)) {
+                        elem->value->state = TRACE_EVENT_STATE_ENABLED;
+                    } else {
+                        elem->value->state = TRACE_EVENT_STATE_DISABLED;
+                    }
+                }
+                /* else: already skipped above */
+            } else {
+                if (trace_event_get_state_dynamic(ev)) {
+                    elem->value->state = TRACE_EVENT_STATE_ENABLED;
+                } else {
+                    elem->value->state = TRACE_EVENT_STATE_DISABLED;
+                }
+            }
         }
         elem->next = events;
         events = elem;
-        found = true;
-    }
-
-    if (!found && !trace_event_is_pattern(name)) {
-        error_setg(errp, "unknown event \"%s\"", name);
     }
 
     return events;
 }
 
 void qmp_trace_event_set_state(const char *name, bool enable,
-                               bool has_ignore_unavailable,
-                               bool ignore_unavailable, Error **errp)
+                               bool has_ignore_unavailable, bool ignore_unavailable,
+                               bool has_vcpu, int64_t vcpu,
+                               Error **errp)
 {
-    bool found = false;
+    Error *err = NULL;
     TraceEvent *ev;
+    bool is_pattern = trace_event_is_pattern(name);
+    CPUState *cpu;
 
-    /* Check all selected events are dynamic */
-    ev = NULL;
-    while ((ev = trace_event_pattern(name, ev)) != NULL) {
-        found = true;
-        if (!(has_ignore_unavailable && ignore_unavailable) &&
-            !trace_event_get_state_static(ev)) {
-            error_setg(errp, "cannot set dynamic tracing state for \"%s\"",
-                       trace_event_get_name(ev));
-            return;
-        }
+    /* Check provided vcpu */
+    cpu = get_cpu(has_vcpu, vcpu, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
     }
-    if (!found && !trace_event_is_pattern(name)) {
-        error_setg(errp, "unknown event \"%s\"", name);
+
+    /* Check events */
+    if (!check_events(has_vcpu, has_ignore_unavailable && ignore_unavailable,
+                      is_pattern, name, errp)) {
         return;
     }
 
-    /* Apply changes */
+    /* Apply changes (all errors checked above) */
     ev = NULL;
     while ((ev = trace_event_pattern(name, ev)) != NULL) {
-        if (trace_event_get_state_static(ev)) {
+        if (!trace_event_get_state_static(ev) ||
+            (has_vcpu && !trace_event_is_vcpu(ev))) {
+            continue;
+        }
+        if (has_vcpu) {
+            trace_event_set_vcpu_state_dynamic(cpu, ev, enable);
+        } else {
             trace_event_set_state_dynamic(ev, enable);
         }
     }