From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56904) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEtYY-0004cD-86 for qemu-devel@nongnu.org; Mon, 20 Jun 2016 03:23:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bEtYT-0007Uf-Ub for qemu-devel@nongnu.org; Mon, 20 Jun 2016 03:23:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bEtYT-0007UQ-ME for qemu-devel@nongnu.org; Mon, 20 Jun 2016 03:23:33 -0400 From: Markus Armbruster References: <146590985033.16561.16324808287190305042.stgit@fimbulvetr.bsc.es> <146590988276.16561.7454935023049676591.stgit@fimbulvetr.bsc.es> Date: Mon, 20 Jun 2016 09:23:29 +0200 In-Reply-To: <146590988276.16561.7454935023049676591.stgit@fimbulvetr.bsc.es> (=?utf-8?Q?=22Llu=C3=ADs?= Vilanova"'s message of "Tue, 14 Jun 2016 15:11:22 +0200") Message-ID: <87y460coi6.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Llu=C3=ADs?= Vilanova Cc: qemu-devel@nongnu.org, Eduardo Habkost , Luiz Capitulino , Stefan Hajnoczi , Paolo Bonzini Llu=C3=ADs Vilanova writes: > Signed-off-by: Llu=C3=ADs Vilanova > Reviewed-by: Stefan Hajnoczi > --- > 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 =3D qdict_get_bool(qdict, "option"); > Error *local_err =3D NULL; >=20=20 > - 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 Q= Dict *qdict) >=20=20 > static void hmp_info_trace_events(Monitor *mon, const QDict *qdict) > { > - TraceEventInfoList *events =3D qmp_trace_event_get_state("*", NULL); > + TraceEventInfoList *events =3D qmp_trace_event_get_state("*", false,= 0, NULL); > TraceEventInfoList *elem; >=20=20 > for (elem =3D events; elem !=3D NULL; elem =3D elem->next) { The new feature remains inaccessible in HMP. Any plans to extend HMP? Any reasons not to? > 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=C3=ADs Vilanova > +# Copyright (C) 2011-2016 Llu=C3=ADs Vilanova > # > # This work is licensed under the terms of the GNU GPL, version 2 or lat= er. > # 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'} } >=20=20 > ## > # @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. > +# - 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? > +# - 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. > +# > # Since 2.2 > ## > { 'command': 'trace-event-get-state', > - 'data': {'name': 'str'}, > + 'data': {'name': 'str', '*vcpu': 'int'}, > 'returns': ['TraceEventInfo'] } >=20=20 > ## > @@ -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 @n= ame. > +# @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. > # > # 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 >=20=20 > { > .name =3D "trace-event-get-state", > - .args_type =3D "name:s", > + .args_type =3D "name:s,vcpu:i?", > .mhandler.cmd_new =3D qmp_marshal_trace_event_get_state, > }, >=20=20 > @@ -4686,6 +4686,11 @@ trace-event-get-state >=20=20 > Query the state of events. >=20=20 > +Arguments: > + > +- "name": Event name pattern (json-string). > +- "vcpu": Specific vCPU to query, any vCPU by default (json-int, optiona= l). > + Less information than you give in the schema. Easy enough to fix. > Example: >=20=20 > -> { "execute": "trace-event-get-state", "arguments": { "name": "qemu_me= malign" } } > @@ -4694,7 +4699,7 @@ EQMP >=20=20 > { > .name =3D "trace-event-set-state", > - .args_type =3D "name:s,enable:b,ignore-unavailable:b?", > + .args_type =3D "name:s,enable:b,ignore-unavailable:b?,vcpu:i?", > .mhandler.cmd_new =3D qmp_marshal_trace_event_set_state, > }, >=20=20 > @@ -4704,6 +4709,14 @@ trace-event-set-state >=20=20 > Set the state of events. >=20=20 > +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. > Example: >=20=20 > -> { "execute": "trace-event-set-state", "arguments": { "name": "qemu_me= malign", "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=C3=ADs Vilanova > + * Copyright (C) 2014-2016 Llu=C3=ADs Vilanova > * > * This work is licensed under the terms of the GNU GPL, version 2 or la= ter. > * See the COPYING file in the top-level directory. > @@ -12,63 +12,148 @@ > #include "trace/control.h" >=20=20 >=20=20 > -TraceEventInfoList *qmp_trace_event_get_state(const char *name, Error **= errp) > +static bool get_cpu(bool has_vcpu, int vcpu, CPUState **cpu, Error **err= p) > +{ > + if (has_vcpu) { > + *cpu =3D qemu_get_cpu(vcpu); > + if (*cpu =3D=3D NULL) { > + error_setg(errp, "invalid vCPU index %u", vcpu); > + return false; > + } > + } else { > + *cpu =3D 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 =3D NULL; cpu =3D 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. > + > +static bool check_events(bool has_vcpu, bool ignore_unavailable, bool is= _pattern, > + const char *name, Error **errp) > +{ > + if (!is_pattern) { > + TraceEvent *ev =3D trace_event_name(name); > + > + /* error for non-existing event */ > + if (ev =3D=3D 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 =3D NULL; > + while ((ev =3D trace_event_pattern(name, ev)) !=3D 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 vcp= u, > + Error **errp) > { > TraceEventInfoList *events =3D NULL; > - bool found =3D false; > TraceEvent *ev; > + bool is_pattern =3D trace_event_is_pattern(name); > + CPUState *cpu; >=20=20 > + /* Check provided vcpu */ > + if (!get_cpu(has_vcpu, vcpu, &cpu, errp)) { > + return NULL; > + } > + > + /* Check events */ > + if (!check_events(has_vcpu, true, is_pattern, name, errp)) { > + return NULL; > + } > + > + /* Get states (all errors checked above) */ > ev =3D NULL; > while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { > - TraceEventInfoList *elem =3D g_new(TraceEventInfoList, 1); > + TraceEventInfoList *elem; > + bool is_vcpu =3D trace_event_is_vcpu(ev); > + if (has_vcpu && !is_vcpu) { > + continue; > + } > + > + elem =3D g_new(TraceEventInfoList, 1); > elem->value =3D g_new(TraceEventInfo, 1); > + elem->value->vcpu =3D is_vcpu; > elem->value->name =3D g_strdup(trace_event_get_name(ev)); > + > if (!trace_event_get_state_static(ev)) { > elem->value->state =3D TRACE_EVENT_STATE_UNAVAILABLE; > - } else if (!trace_event_get_state_dynamic(ev)) { > - elem->value->state =3D TRACE_EVENT_STATE_DISABLED; > } else { > - elem->value->state =3D TRACE_EVENT_STATE_ENABLED; > + if (has_vcpu) { > + if (is_vcpu) { > + if (trace_event_get_vcpu_state_dynamic(cpu, ev)) { > + elem->value->state =3D TRACE_EVENT_STATE_ENABLED; > + } else { > + elem->value->state =3D TRACE_EVENT_STATE_DISABLE= D; > + } > + } > + /* else: already skipped above */ > + } else { > + if (trace_event_get_state_dynamic(ev)) { > + elem->value->state =3D TRACE_EVENT_STATE_ENABLED; > + } else { > + elem->value->state =3D TRACE_EVENT_STATE_DISABLED; > + } > + } > } > elem->next =3D events; > events =3D elem; > - found =3D true; > - } > - > - if (!found && !trace_event_is_pattern(name)) { > - error_setg(errp, "unknown event \"%s\"", name); > } >=20=20 > return events; > } >=20=20 > 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 =3D false; > TraceEvent *ev; > + bool is_pattern =3D trace_event_is_pattern(name); > + CPUState *cpu; >=20=20 > - /* Check all selected events are dynamic */ > - ev =3D NULL; > - while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { > - found =3D 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 */ > + if (!get_cpu(has_vcpu, vcpu, &cpu, errp)) { > + 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_unavail= able, > + is_pattern, name, errp)) { > return; > } >=20=20 > - /* Apply changes */ > + /* Apply changes (all errors checked above) */ > ev =3D NULL; > while ((ev =3D trace_event_pattern(name, ev)) !=3D 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); > } > } Looks okay.