| Message ID | 20200723132903.1980743-2-dinechin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace: Add a trace backend for the recorder library | expand |
On Thu, Jul 23, 2020 at 03:29:02PM +0200, Christophe de Dinechin wrote: > diff --git a/configure b/configure > index 4bd80ed507..3770ff873d 100755 > --- a/configure > +++ b/configure > @@ -7761,6 +7761,20 @@ fi > if have_backend "log"; then > echo "CONFIG_TRACE_LOG=y" >> $config_host_mak > fi > +if have_backend "recorder"; then > + recorder_minver="1.0.10" > + if $pkg_config --atleast-version=$recorder_minver recorder ; then > + recorder_cflags="$($pkg_config --cflags recorder)" > + recorder_libs="$($pkg_config --libs recorder)" > + LIBS="$recorder_libs $LIBS" > + libs_qga="$recorder_libs $libs_qga" > + QEMU_CFLAGS="$QEMU_CFLAGS $recorder_cflags" > + zstd="yes" Why enable zstd? This happens after the zstd library dependency probe, so it overrides the zstd setting even if the probe failed earlier on. This seems strange. > +#if defined(CONFIG_TRACE_RECORDER) > + { > + .name = "recorder", > + .args_type = "op:s?,arg:s?", > + .params = "trace|dump [arg]", > + .help = "trace selected recorders or print recorder dump", > + .cmd = hmp_recorder, > + }, > + > +SRST > +``trace`` *cmds* How is this sub-command different from the "trace-event <name> on|off" HMP command? > + Activate or deactivate tracing for individual recorder traces. > + See recorder_trace_set(3) for the syntax of *cmds* > + For example, to activate trace ``foo`` and disable alll traces s/alll/all/ > + ending in ``_warning``, use ``foo:.*_warning=0``. > + Using ``help`` will list available traces and their current setting. "help" is not listed in .params. How is this sub-command different from the "info trace-events" HMP command? > +#ifdef CONFIG_TRACE_RECORDER > +static void hmp_recorder(Monitor *mon, const QDict *qdict) > +{ > + const char *op = qdict_get_try_str(qdict, "op"); > + const char *arg = qdict_get_try_str(qdict, "arg"); > + > + if (!op) { > + monitor_printf(mon, "missing recorder command\"%s\"\n", op); op is NULL, why format it? There is a space missing after "command". > +def generate_c_begin(events, group): > + out('#include "qemu/osdep.h"', > + '#include "trace/control.h"', > + '#include "trace/simple.h"', Is this header needed? > +RECORDER_CONSTRUCTOR > +void recorder_trace_init(void) This function is called 3 times: 1. RECORDER_CONSTRUCTOR 2. trace_init_backends() 3. module_load_file() That is unusual for an "init" function. Are all 3 really necessary? Please add a comment explaining what is going on here and/or rename it to recorder_trace_reinit(). > +{ > + const char *traces = getenv("RECORDER_TRACES"); > + recorder_trace_set(traces); > + > + /* > + * Allow a dump in case we receive some unhandled signal > + * For example, send USR2 to a hung process to get a dump > + */ > + if (traces) { > + recorder_dump_on_common_signals(0, 0); Did you check all programs (i.e. qemu-system-*, qemu-user-*, qemu-img, etc) to see whether installing various signal handlers is okay? The library documentation says the following signals are handled: SIGQUIT, SIGILL, SIGABRT, SIGBUS, SIGSEGV, SIGSYS, SIGXCPU, SIGXFSZ, SIGINFO, SIGUSR1, SIGUSR2, SIGSTKFLT, SIGPWR QEMU uses SIGBUS and SIGUSR1 ("SIG_IPI"). I'm not sure of the signal policy for qemu-user-* but you might need to take special precautions. I don't think installing signal handlers from an __attribute__((constructor)) function is a viable approach since there is no control over the ordering. If other object files do the same then whose signal handler ends up being installed? It may be cleaner to have a separate function called recorder_setup_signals() that the programs can call when they install signal handlers. That way the recorder signal handlers are installed along with all the other signal handlers at program startup instead of being installed from a constructor function that is hard to find.
Christophe de Dinechin <dinechin@redhat.com> writes: > The recorder library provides support for low-cost continuous > recording of events, which can then be replayed. This makes it > possible to collect data after the fact, for example to show the > events that led to a crash or unexpected condition. > > In this series, minimal recorder support in qemu is implemented using > the existing tracing interface. For each trace, a corresponding > recorder is created with a generic name and a default size of 8 entries. > > In addition, it is possible to explicitly enable recorders that are > not qemu traces, for example in order to use actually record events > rather than trace them, or to use the real-time graphing capabilities. > For that reason, a limited set of recorder-related macros are defined > as no-ops even if the recorder trace backend is not configured. > > Recorder-specific features, notably the ability to perform a > post-mortem dump and to group traces by topic, are not integrated in > this series, as doing so would require modifying the trace > infrastructure, which is a non-objective here. This may be the topic > of later series if there is any interest for it. > > HMP COMMAND: > The 'recorder' hmp command has been added, which supports two > sub-commands: > * recorder dump: Dump the current state of the recorder. You can > give that command a recorder name, to only dump that recorder. > * recorder trace: Set traces using the recorder_trace_set() syntax. > You can use "recorder trace help" to list all available recorders. Standard comment for patches adding HMP-only monitor commands: In general, functionality available in HMP should also available in QMP. Exceptions include functionality that makes no sense in QMP, or is of use only for human users. If you think your command is an exception, please explain why in the commit message. If it isn't, you need to implement it for QMP (including suitable test cases), then rewrite the HMP version to reuse either the QMP command or a common core. Example for "makes no sense in QMP": setting the current CPU, because a QMP monitor doesn't have a current CPU. Examples for "is of use only for human users": HMP command "help", the integrated pocket calculator.
diff --git a/MAINTAINERS b/MAINTAINERS index 3395abd4e1..764b5915d7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2484,6 +2484,7 @@ F: stubs/ Tracing M: Stefan Hajnoczi <stefanha@redhat.com> S: Maintained +R: Christophe de Dinechin <dinechin@redhat.com> F: trace/ F: trace-events F: docs/qemu-option-trace.rst.inc diff --git a/configure b/configure index 4bd80ed507..3770ff873d 100755 --- a/configure +++ b/configure @@ -7761,6 +7761,20 @@ fi if have_backend "log"; then echo "CONFIG_TRACE_LOG=y" >> $config_host_mak fi +if have_backend "recorder"; then + recorder_minver="1.0.10" + if $pkg_config --atleast-version=$recorder_minver recorder ; then + recorder_cflags="$($pkg_config --cflags recorder)" + recorder_libs="$($pkg_config --libs recorder)" + LIBS="$recorder_libs $LIBS" + libs_qga="$recorder_libs $libs_qga" + QEMU_CFLAGS="$QEMU_CFLAGS $recorder_cflags" + zstd="yes" + echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak + else + feature_not_found "recorder" "Install recorder-devel package" + fi +fi if have_backend "ust"; then echo "CONFIG_TRACE_UST=y" >> $config_host_mak fi diff --git a/hmp-commands.hx b/hmp-commands.hx index d548a3ab74..f164ff6d65 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -302,6 +302,28 @@ SRST Open, close, or flush the trace file. If no argument is given, the status of the trace file is displayed. ERST +#endif + +#if defined(CONFIG_TRACE_RECORDER) + { + .name = "recorder", + .args_type = "op:s?,arg:s?", + .params = "trace|dump [arg]", + .help = "trace selected recorders or print recorder dump", + .cmd = hmp_recorder, + }, + +SRST +``trace`` *cmds* + Activate or deactivate tracing for individual recorder traces. + See recorder_trace_set(3) for the syntax of *cmds* + For example, to activate trace ``foo`` and disable alll traces + ending in ``_warning``, use ``foo:.*_warning=0``. + Using ``help`` will list available traces and their current setting. +``dump`` [*name*] + Dump the recorder. If *name* is given, only the specific named recorder + will be dumped. +ERST #endif { @@ -1828,4 +1850,3 @@ ERST .sub_table = hmp_info_cmds, .flags = "p", }, - diff --git a/monitor/misc.c b/monitor/misc.c index e847b58a8c..aff72158d7 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -60,6 +60,7 @@ #ifdef CONFIG_TRACE_SIMPLE #include "trace/simple.h" #endif +#include "trace/recorder.h" #include "exec/memory.h" #include "exec/exec-all.h" #include "qemu/option.h" @@ -226,6 +227,30 @@ static void hmp_trace_file(Monitor *mon, const QDict *qdict) } #endif +#ifdef CONFIG_TRACE_RECORDER +static void hmp_recorder(Monitor *mon, const QDict *qdict) +{ + const char *op = qdict_get_try_str(qdict, "op"); + const char *arg = qdict_get_try_str(qdict, "arg"); + + if (!op) { + monitor_printf(mon, "missing recorder command\"%s\"\n", op); + help_cmd(mon, "recorder"); + } else if (!strcmp(op, "trace")) { + recorder_trace_set(arg); + } else if (!strcmp(op, "dump")) { + if (!arg || !*arg) { + recorder_dump(); + } else { + recorder_dump_for(arg); + } + } else { + monitor_printf(mon, "unexpected recorder command \"%s\"\n", op); + help_cmd(mon, "recorder"); + } +} +#endif + static void hmp_info_help(Monitor *mon, const QDict *qdict) { help_cmd(mon, "info"); diff --git a/scripts/tracetool/backend/recorder.py b/scripts/tracetool/backend/recorder.py new file mode 100644 index 0000000000..82d983ff31 --- /dev/null +++ b/scripts/tracetool/backend/recorder.py @@ -0,0 +1,52 @@ +# -*- coding: utf-8 -*- + +""" +Trace back-end for recorder library +""" + +__author__ = "Christophe de Dinechin <christophe@dinechin.org>" +__copyright__ = "Copyright 2020, Christophe de Dinechin and Red Hat" +__license__ = "GPL version 2 or (at your option) any later version" + +__maintainer__ = "Christophe de Dinechin" +__email__ = "christophe@dinechin.org" + + +from tracetool import out + +PUBLIC = True + +def generate_h_begin(events, group): + out('#include <recorder/recorder.h>', '') + + for event in events: + out('RECORDER_DECLARE(%(name)s);', name=event.name) + + +def generate_h(event, group): + argnames = ", ".join(event.args.names()) + if len(event.args) > 0: + argnames = ", " + argnames + + out(' record(%(event)s, %(fmt)s %(argnames)s);', + event=event.name, + fmt=event.fmt.rstrip("\n"), + argnames=argnames) + + +def generate_h_backend_dstate(event, group): + out(' RECORDER_TWEAK(%(event_id)s) || \\', event_id=event.name) + +def generate_c_begin(events, group): + out('#include "qemu/osdep.h"', + '#include "trace/control.h"', + '#include "trace/simple.h"', + '#include <recorder/recorder.h>', + '') + + for event in events: + out('RECORDER_DEFINE(%(name)s, 8,', + ' "Tracetool recorder for %(api)s(%(args)s)");', + name=event.name, + api=event.api(), + args=event.args) diff --git a/trace/Makefile.objs b/trace/Makefile.objs index c544509adf..13667e98d5 100644 --- a/trace/Makefile.objs +++ b/trace/Makefile.objs @@ -54,6 +54,7 @@ $(obj)/generated-tcg-tracers.h-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/ util-obj-$(CONFIG_TRACE_SIMPLE) += simple.o util-obj-$(CONFIG_TRACE_FTRACE) += ftrace.o +util-obj-$(CONFIG_TRACE_RECORDER) += recorder.o util-obj-y += control.o obj-y += control-target.o util-obj-y += qmp.o diff --git a/trace/control.c b/trace/control.c index 2ffe000818..71f170f73b 100644 --- a/trace/control.c +++ b/trace/control.c @@ -23,6 +23,7 @@ #ifdef CONFIG_TRACE_SYSLOG #include <syslog.h> #endif +#include "trace/recorder.h" #include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/config-file.h" @@ -282,6 +283,10 @@ bool trace_init_backends(void) openlog(NULL, LOG_PID, LOG_DAEMON); #endif +#ifdef CONFIG_TRACE_RECORDER + recorder_trace_init(); +#endif + return true; } diff --git a/trace/recorder.c b/trace/recorder.c new file mode 100644 index 0000000000..c47abc363c --- /dev/null +++ b/trace/recorder.c @@ -0,0 +1,27 @@ +/* + * Recorder-based trace backend + * + * Copyright Red Hat 2020 + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "trace/recorder.h" + +RECORDER_CONSTRUCTOR +void recorder_trace_init(void) +{ + const char *traces = getenv("RECORDER_TRACES"); + recorder_trace_set(traces); + + /* + * Allow a dump in case we receive some unhandled signal + * For example, send USR2 to a hung process to get a dump + */ + if (traces) { + recorder_dump_on_common_signals(0, 0); + } +} diff --git a/trace/recorder.h b/trace/recorder.h new file mode 100644 index 0000000000..e69f633437 --- /dev/null +++ b/trace/recorder.h @@ -0,0 +1,32 @@ +/* + * Recorder-based trace backend + * + * Copyright Red Hat 2020 + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#ifndef TRACE_RECORDER_H +#define TRACE_RECORDER_H + +#ifdef CONFIG_TRACE_RECORDER + +#include <recorder/recorder.h> + +extern void recorder_trace_init(void); + +#else + +/* Disable recorder macros */ +#define RECORDER(Name, Size, Description) +#define RECORDER_DEFINE(Name, Size, Description) +#define RECORDER_DECLARE(Name) +#define RECORD(Name, ...) +#define record(Name, ...) +#define recorder_trace_init() + +#endif /* CONFIG_TRACE_RECORDER */ + +#endif /* TRACE_RECORDER_H */ diff --git a/util/module.c b/util/module.c index 0ab00851f0..03179a09aa 100644 --- a/util/module.c +++ b/util/module.c @@ -22,6 +22,8 @@ #ifdef CONFIG_MODULE_UPGRADES #include "qemu-version.h" #endif +#include "trace/recorder.h" + typedef struct ModuleEntry { @@ -150,6 +152,10 @@ static int module_load_file(const char *fname) g_module_close(g_module); ret = -EINVAL; } else { +#ifdef CONFIG_TRACE_RECORDER + /* New recorders may have been pulled in, activate them if necessary */ + recorder_trace_init(); +#endif QTAILQ_FOREACH(e, &dso_init_list, node) { e->init(); register_module_init(e->init, e->type);
The recorder library provides support for low-cost continuous recording of events, which can then be replayed. This makes it possible to collect data after the fact, for example to show the events that led to a crash or unexpected condition. In this series, minimal recorder support in qemu is implemented using the existing tracing interface. For each trace, a corresponding recorder is created with a generic name and a default size of 8 entries. In addition, it is possible to explicitly enable recorders that are not qemu traces, for example in order to use actually record events rather than trace them, or to use the real-time graphing capabilities. For that reason, a limited set of recorder-related macros are defined as no-ops even if the recorder trace backend is not configured. Recorder-specific features, notably the ability to perform a post-mortem dump and to group traces by topic, are not integrated in this series, as doing so would require modifying the trace infrastructure, which is a non-objective here. This may be the topic of later series if there is any interest for it. HMP COMMAND: The 'recorder' hmp command has been added, which supports two sub-commands: * recorder dump: Dump the current state of the recorder. You can give that command a recorder name, to only dump that recorder. * recorder trace: Set traces using the recorder_trace_set() syntax. You can use "recorder trace help" to list all available recorders. Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> --- MAINTAINERS | 1 + configure | 14 ++++++++ hmp-commands.hx | 23 +++++++++++- monitor/misc.c | 25 +++++++++++++ scripts/tracetool/backend/recorder.py | 52 +++++++++++++++++++++++++++ trace/Makefile.objs | 1 + trace/control.c | 5 +++ trace/recorder.c | 27 ++++++++++++++ trace/recorder.h | 32 +++++++++++++++++ util/module.c | 6 ++++ 10 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 scripts/tracetool/backend/recorder.py create mode 100644 trace/recorder.c create mode 100644 trace/recorder.h