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

[Markus Armbruster]

Lluís Vilanova <address@hidden> writes:

> 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.

What is "trace-events"?  The file in the source tree?

>>> +# - 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").

Fails to mention that @vcpu selects the TraceEventInfo for that
particular VCPU.  That's what I tried to express by "its vCPU equals
@vcpu".

>   #
>   # 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.

Yes.

I hope we can get rid of the duplication some day.  Marc-André has
patches.

[...]