Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <address@hidden> writes:
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> Reviewed-by: Stefan Hajnoczi <address@hidden>
>> ---
>> monitor.c       |    4 +-
>> qapi/trace.json |   20 ++++++--
>> qmp-commands.hx |   17 ++++++-
>> trace/qmp.c     |  143 
>> ++++++++++++++++++++++++++++++++++++++++++++-----------
>> 4 files changed, 147 insertions(+), 37 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index a27e115..bb89877 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -910,7 +910,7 @@ static void hmp_trace_event(Monitor *mon, const QDict 
>> *qdict)
>> bool new_state = qdict_get_bool(qdict, "option");
>> Error *local_err = NULL;
>> 
>> -    qmp_trace_event_set_state(tp_name, new_state, true, true, &local_err);
>> +    qmp_trace_event_set_state(tp_name, new_state, true, true, false, 0, 
>> &local_err);
>> if (local_err) {
>> error_report_err(local_err);
>> }
>> @@ -1069,7 +1069,7 @@ static void hmp_info_cpustats(Monitor *mon, const 
>> QDict *qdict)
>> 
>> static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>> {
>> -    TraceEventInfoList *events = qmp_trace_event_get_state("*", NULL);
>> +    TraceEventInfoList *events = qmp_trace_event_get_state("*", false, 0, 
>> NULL);
>> TraceEventInfoList *elem;
>> 
>> for (elem = events; elem != NULL; elem = elem->next) {

> The new feature remains inaccessible in HMP.  Any plans to extend HMP?
> Any reasons not to?

My bad, I forgot about it. I'll see to adding it.


>> diff --git a/qapi/trace.json b/qapi/trace.json
>> index 01b0a52..25d8095 100644
>> --- a/qapi/trace.json
>> +++ b/qapi/trace.json
>> @@ -1,6 +1,6 @@
>> # -*- mode: python -*-
>> #
>> -# Copyright (C) 2011-2014 Lluís Vilanova <address@hidden>
>> +# Copyright (C) 2011-2016 Lluís Vilanova <address@hidden>
>> #
>> # 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,12 @@
>> #
>> # @name: Event name.
>> # @state: Tracing state.
>> +# @vcpu: Whether this is a per-vCPU event (since 2.7).
>> #
>> # Since 2.2
>> ##
>> { 'struct': 'TraceEventInfo',
>> -  'data': {'name': 'str', 'state': 'TraceEventState'} }
>> +  'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} }
>> 
>> ##
>> # @trace-event-get-state:
>> @@ -41,13 +42,18 @@
>> # Query the state of events.
>> #
>> # @name: Event name pattern (case-sensitive glob).
>> +# @vcpu: #optional The vCPU to check (any by default; since 2.7).
>> #
>> # Returns: a list of @TraceEventInfo for the matching events
>> #
>> +# For any event without the "vcpu" property:

> All events have the vcpu property, but for some of them the property's
> value is false.

Hmmm, do you mean in the generated event array? Here, I was referring to wether
an event has the "vcpu" property in "trace-events". I can clarify that.


>> +# - If @name is a pattern and @vcpu is set, events are ignored.

> I figure "ignored" means they're not included in the return value.
> Correct?

Exactly.


>> +# - If @name is not a pattern and @vcpu is set, an error is raised.

> Perhaps we could clarify as follows:

> # Returns: a list of @TraceEventInfo for the matching events
> #
> # An event matches if
> # - its name matches the @name pattern, and
> # - if @vcpu is given, its vCPU equals @vcpu.
> # It follows that with @vcpu given, the query can match only per-vCPU
> # events.  Special case: if the name is an exact match, you get an error
> # instead of an empty list.

Doesn't sound entirely right to me:

  # Returns: a list of @TraceEventInfo for the matching events
  #
  # An event matches if
  # - its name matches the @name pattern, and
  # - if @vcpu is given, the event is a per-vCPU one (has the "vcpu" property in
  #   "trace-events").
  #
  # Therefore, if @vcpu is given, the query will only match per-vCPU events.
  # Special case: if @name is an exact match and @vcpu is given, you will get an
  # error if the event does not have the "vcpu" property.


>> +#
>> # Since 2.2
>> ##
>> { 'command': 'trace-event-get-state',
>> -  'data': {'name': 'str'},
>> +  'data': {'name': 'str', '*vcpu': 'int'},
>> 'returns': ['TraceEventInfo'] }
>> 
>> ##
>> @@ -58,8 +64,14 @@
>> # @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).
>> +#
>> +# For any event without the "vcpu" property:
>> +# - If @name is a pattern and @vcpu is set, events are ignored.
>> +# - If @name is not a pattern and @vcpu is set, an error is raised.

> Likewise.

Ok.


>> #
>> # 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 28801a2..c9eb25c 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4676,7 +4676,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,
>> },
>> 
>> @@ -4686,6 +4686,11 @@ trace-event-get-state
>> 
>> Query the state of events.
>> 
>> +Arguments:
>> +
>> +- "name": Event name pattern (json-string).
>> +- "vcpu": Specific vCPU to query, any vCPU by default (json-int, optional).
>> +

> Less information than you give in the schema.  Easy enough to fix.

I guess you mean extending the docs here to cover the same info given in the
JSON file.


>> Example:
>> 
-> { "execute": "trace-event-get-state", "arguments": { "name": "qemu_memalign" 
} }
>> @@ -4694,7 +4699,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,
>> },
>> 
>> @@ -4704,6 +4709,14 @@ 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": Specific vCPU to set, all vCPUs by default (json-int, optional).
>> +

> Likewise.

Ok.


>> Example:
>> 
-> { "execute": "trace-event-set-state", "arguments": { "name": 
"qemu_memalign", "enable": "true" } }
>> diff --git a/trace/qmp.c b/trace/qmp.c
>> index 8aa2660..c18e750 100644
>> --- a/trace/qmp.c
>> +++ b/trace/qmp.c
>> @@ -1,7 +1,7 @@
>> /*
>> * QMP commands for tracing events.
>> *
>> - * Copyright (C) 2014 Lluís Vilanova <address@hidden>
>> + * Copyright (C) 2014-2016 Lluís Vilanova <address@hidden>
>> *
>> * 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,148 @@
>> #include "trace/control.h"
>> 
>> 
>> -TraceEventInfoList *qmp_trace_event_get_state(const char *name, Error 
>> **errp)
>> +static bool get_cpu(bool has_vcpu, int vcpu, CPUState **cpu, Error **errp)
>> +{
>> +    if (has_vcpu) {
>> +        *cpu = qemu_get_cpu(vcpu);
>> +        if (*cpu == NULL) {
>> +            error_setg(errp, "invalid vCPU index %u", vcpu);
>> +            return false;
>> +        }
>> +    } else {
>> +        *cpu = NULL;
>> +    }
>> +    return true;
>> +}

> This returns three things: a bool (via return value), a CPUState *
> (via @cpu) and an Error (via customary @errp).  Possible combinations:

> * Success with has_vcpu: true, non-null CPU *, no error
> * Failure with has_vcpu: false, null CPU *, error set
> * Success without has_vcpu: true, null CPU *, no error

> Usage:

>     if (!get_cpu(has_vcpu, vcpu, &cpu, errp)) {
>         error out...
>     }

> If you dispense with the bool and return the pointer instead, you'd get:

>     Error *err = NULL;

>     cpu = get_cpu(has_vcpu, vcpu, &err);
>     if (err) {
>         error_propagate(errp, err);
>         error out...
>     }

> This is more in line with how we use Error elsewhere.  Needs more code,
> though.  Since it's just a local helper function, the choice is yours.

Oh, I didn't think of using err to check errors. That seems more readable.


Thanks,
  Lluis