[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state |
Date: |
Mon, 20 Jun 2016 09:23:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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?
> 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.
> +# - 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'] }
>
> ##
> @@ -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.
> #
> # 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.
> 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.
> 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.
> +
> +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)
> {
> TraceEventInfoList *events = NULL;
> - bool found = false;
> TraceEvent *ev;
> + bool is_pattern = trace_event_is_pattern(name);
> + CPUState *cpu;
>
> + /* 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 = 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;
> 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 */
> + 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_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);
> }
> }
Looks okay.
[Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state, Lluís Vilanova, 2016/06/14
- Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state,
Markus Armbruster <=
Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state, Lluís Vilanova, 2016/06/20
Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state, Markus Armbruster, 2016/06/20
Re: [Qemu-devel] [PATCH v4 6/6] trace: Add QAPI/QMP interfaces to query and control per-vCPU tracing state, Lluís Vilanova, 2016/06/20
[Qemu-devel] [PATCH v4 2/6] disas: Remove unused macro '_', Lluís Vilanova, 2016/06/14