diff mbox

[v2,10/11] trace-state: [stderr] add support for dynamically enabling/disabling events

Message ID 20110406183503.22854.39534.stgit@ginnungagap.bsc.es
State New
Headers show

Commit Message

=?utf-8?Q?Llu=C3=ADs?= April 6, 2011, 6:35 p.m. UTC
This includes all the control interfaces already provided by the "simple"
backend (i.e., command line, programmatic and monitor).

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 Makefile.objs     |    8 +++++++-
 configure         |    3 +++
 docs/tracing.txt  |    6 ++----
 hmp-commands.hx   |   10 ++++++++--
 monitor.c         |    8 +++++++-
 qemu-config.c     |    7 ++++---
 qemu-options.hx   |    8 +++++++-
 scripts/tracetool |   33 ++++++++++++++++++++++++++++-----
 stderrtrace.c     |   24 ++++++++++++++++++++++++
 stderrtrace.h     |   14 ++++++++++++++
 vl.c              |   11 +++++++++--
 11 files changed, 113 insertions(+), 19 deletions(-)
 create mode 100644 stderrtrace.c
 create mode 100644 stderrtrace.h

Comments

Stefan Hajnoczi April 23, 2011, 2:31 p.m. UTC | #1
On Wed, Apr 6, 2011 at 7:35 PM, Lluís <xscript@gmx.net> wrote:
> This includes all the control interfaces already provided by the "simple"
> backend (i.e., command line, programmatic and monitor).
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  Makefile.objs     |    8 +++++++-
>  configure         |    3 +++
>  docs/tracing.txt  |    6 ++----
>  hmp-commands.hx   |   10 ++++++++--
>  monitor.c         |    8 +++++++-
>  qemu-config.c     |    7 ++++---
>  qemu-options.hx   |    8 +++++++-
>  scripts/tracetool |   33 ++++++++++++++++++++++++++++-----
>  stderrtrace.c     |   24 ++++++++++++++++++++++++
>  stderrtrace.h     |   14 ++++++++++++++
>  vl.c              |   11 +++++++++--
>  11 files changed, 113 insertions(+), 19 deletions(-)
>  create mode 100644 stderrtrace.c
>  create mode 100644 stderrtrace.h

I feel that the monitor commands for the simple backend were a
mistake.  Simple trace is used during development, not production, so
being able to toggle trace events at runtime is probably not worth the
extra user interfaces we've added.  But when the simple backend was
written we thought it would be used in production and planned for
libvirt interfaces and all ;).

Let's not go down that road for the stderr backend which is very
useful today at a tiny cost in code size.

For tracing use cases that require performance or runtime
enabling/disabling trace events, just use the simple, ust, or dtrace
backends.

Please drop this patch.

Stefan
Paolo Bonzini April 24, 2011, 6:24 a.m. UTC | #2
On 04/23/2011 04:31 PM, Stefan Hajnoczi wrote:
> For tracing use cases that require performance or runtime
> enabling/disabling trace events, just use the simple, ust, or dtrace
> backends.

Having -trace events for the stderr backend would still be nice.

Paolo
Stefan Hajnoczi April 24, 2011, 9:33 a.m. UTC | #3
On Sun, Apr 24, 2011 at 7:24 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 04/23/2011 04:31 PM, Stefan Hajnoczi wrote:
>>
>> For tracing use cases that require performance or runtime
>> enabling/disabling trace events, just use the simple, ust, or dtrace
>> backends.
>
> Having -trace events for the stderr backend would still be nice.

That should be doable without ifdefing and duplicating simpletrace.
The tracer and monitor command parts of simpletrace need to be
separated from common TraceEvent and tracetool generation, which can
be reused by stderr.

Stefan
=?utf-8?Q?Llu=C3=ADs?= April 25, 2011, 10:27 a.m. UTC | #4
Stefan Hajnoczi writes:

> On Sun, Apr 24, 2011 at 7:24 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 04/23/2011 04:31 PM, Stefan Hajnoczi wrote:
>>> 
>>> For tracing use cases that require performance or runtime
>>> enabling/disabling trace events, just use the simple, ust, or dtrace
>>> backends.
>> 
>> Having -trace events for the stderr backend would still be nice.

> That should be doable without ifdefing and duplicating simpletrace.
> The tracer and monitor command parts of simpletrace need to be
> separated from common TraceEvent and tracetool generation, which can
> be reused by stderr.

That's exactly what I thought, but I tried to preserve as much as
possible the original patch that was sent to me.

But in any case, I'm still not sure if stderr should have programatic
tracing state controls.


Lluis
Paolo Bonzini April 25, 2011, 6:10 p.m. UTC | #5
On 04/25/2011 12:27 PM, Lluís wrote:
> But in any case, I'm still not sure if stderr should have programatic
> tracing state controls.

Yes, please, stderr is even more useful than simple when you're using it 
under gdb.

Paolo
Fabien Chouteau April 26, 2011, 12:30 p.m. UTC | #6
On 04/25/2011 08:10 PM, Paolo Bonzini wrote:
> On 04/25/2011 12:27 PM, Lluís wrote:
>> But in any case, I'm still not sure if stderr should have programatic
>> tracing state controls.
> 
> Yes, please, stderr is even more useful than simple when you're using it under gdb.

Agreed, trace control seems really useful with stderr, and we should be able to
do this in a generic way (shared by simple and stderr backends).
Stefan Hajnoczi April 26, 2011, 12:38 p.m. UTC | #7
On Tue, Apr 26, 2011 at 1:30 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 04/25/2011 08:10 PM, Paolo Bonzini wrote:
>> On 04/25/2011 12:27 PM, Lluís wrote:
>>> But in any case, I'm still not sure if stderr should have programatic
>>> tracing state controls.
>>
>> Yes, please, stderr is even more useful than simple when you're using it under gdb.
>
> Agreed, trace control seems really useful with stderr, and we should be able to
> do this in a generic way (shared by simple and stderr backends).

The commonality between stderr and simple is having a set of trace
events with on/off states.  Generating trace.h/trace.c is mostly
common.  Toggling trace event states from the monitor as well as
-trace events=<file> are common.

The simple backend additionally allows setting and flushing the output
file.  It also supports dumping the trace buffer.

Stefan
Paolo Bonzini April 26, 2011, 12:59 p.m. UTC | #8
On 04/26/2011 02:38 PM, Stefan Hajnoczi wrote:
> The simple backend additionally allows setting and flushing the output
> file.  It also supports dumping the trace buffer.

I agree that neither of these would be a particularly interesting 
addition to the stderr backend.

Paolo
=?utf-8?Q?Llu=C3=ADs?= April 26, 2011, 2:01 p.m. UTC | #9
Stefan Hajnoczi writes:

> On Tue, Apr 26, 2011 at 1:30 PM, Fabien Chouteau <chouteau@adacore.com> wrote:
>> On 04/25/2011 08:10 PM, Paolo Bonzini wrote:
>>> On 04/25/2011 12:27 PM, Lluís wrote:
>>>> But in any case, I'm still not sure if stderr should have programatic
>>>> tracing state controls.
>>> 
>>> Yes, please, stderr is even more useful than simple when you're using it under gdb.
>> 
>> Agreed, trace control seems really useful with stderr, and we should be able to
>> do this in a generic way (shared by simple and stderr backends).

> The commonality between stderr and simple is having a set of trace
> events with on/off states.  Generating trace.h/trace.c is mostly
> common.  Toggling trace event states from the monitor as well as
> -trace events=<file> are common.

> The simple backend additionally allows setting and flushing the output
> file.  It also supports dumping the trace buffer.

Ok, what I was thinking about long time ago is providing some generic
"trace_*" interface for the common cmdline and monitor commands to use
(basically list event names and query or change their dynamic state,
which is the only common part on all backends).

With this, every backend is responsible of providing a suitable
implementation. If no implementation is provided, a default one will be
used that simply results in qemu erroring out when any of these
functionalities is invoked.

I think this was already discussed, and the idea was rejected because it
seemed too verbose for qemu to implement tracepoint controls when other
external tools already exist for such backends (e.g., ust). Maybe
providing the default implementation on the previous paragraph would
solve the issues.


Lluis
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index c05f5e5..f7cd67b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -342,15 +342,22 @@  trace-dtrace.o: trace-dtrace.dtrace $(GENERATED_HEADERS)
 	$(call quiet-command,dtrace -o $@ -G -s $<, "  GEN trace-dtrace.o")
 
 simpletrace.o: simpletrace.c $(GENERATED_HEADERS)
+stderrtrace.o: stderrtrace.c $(GENERATED_HEADERS)
 
 ifeq ($(TRACE_BACKEND),dtrace)
 trace-obj-y = trace-dtrace.o
 else
 trace-obj-y = trace.o
+
 ifeq ($(TRACE_BACKEND),simple)
 trace-obj-y += simpletrace.o
 user-obj-y += qemu-timer-common.o
 endif
+
+ifeq ($(TRACE_BACKEND),stderr)
+trace-obj-y += stderrtrace.o
+endif
+
 endif
 
 ######################################################################
@@ -361,4 +368,3 @@  libcacard-y = cac.o event.o vcard.o vreader.o vcard_emul_nss.o vcard_emul_type.o
 vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
 vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
-
diff --git a/configure b/configure
index 8754060..7dc9bdc 100755
--- a/configure
+++ b/configure
@@ -2933,6 +2933,9 @@  echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
 if test "$trace_backend" = "simple"; then
   echo "CONFIG_SIMPLE_TRACE=y" >> $config_host_mak
 fi
+if test "$trace_backend" = "stderr"; then
+  echo "CONFIG_STDERR_TRACE=y" >> $config_host_mak
+fi
 # Set the appropriate trace file.
 if test "$trace_backend" = "simple"; then
   trace_file="\"$trace_file-%u\""
diff --git a/docs/tracing.txt b/docs/tracing.txt
index 26b221f..51fe0ad 100644
--- a/docs/tracing.txt
+++ b/docs/tracing.txt
@@ -134,10 +134,8 @@  effectively turns trace events into debug printfs.
 This is the simplest backend and can be used together with existing code that
 uses DPRINTF().
 
-Note that with this backend trace events cannot be programmatically
-enabled/disabled. Thus, in order to trim down the amount of output and the
-performance impact of tracing, you might want to add the "disable" property in
-the "trace-events" file for those events you are not interested in.
+See the documentation in the "simple" backend for instruction on how to control
+trace event states from the command line, the monitor and/or programmatically.
 
 === Simpletrace ===
 
diff --git a/hmp-commands.hx b/hmp-commands.hx
index c3be311..7cca351 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -180,7 +180,7 @@  STEXI
 Output logs to @var{filename}.
 ETEXI
 
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
     {
         .name       = "trace-event",
         .args_type  = "name:s,option:b",
@@ -194,7 +194,9 @@  STEXI
 @findex trace-event
 changes status of a trace event
 ETEXI
+#endif
 
+#if defined(CONFIG_SIMPLE_TRACE)
     {
         .name       = "trace-file",
         .args_type  = "op:s?,arg:F?",
@@ -1355,10 +1357,14 @@  show roms
 @end table
 ETEXI
 
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE)
 STEXI
 @item info trace
 show contents of trace buffer
+ETEXI
+#endif
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
+STEXI
 @item info trace-events
 show available trace events and their state
 ETEXI
diff --git a/monitor.c b/monitor.c
index 377424e..7991c6c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -590,7 +590,7 @@  static void do_help_cmd(Monitor *mon, const QDict *qdict)
     help_cmd(mon, qdict_get_try_str(qdict, "name"));
 }
 
-#if defined(CONFIG_SIMPLE_TRACE)
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
 {
     const char *tp_name = qdict_get_str(qdict, "name");
@@ -601,7 +601,9 @@  static void do_change_trace_event_state(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "unknown event name \"%s\"\n", tp_name);
     }
 }
+#endif
 
+#if defined(CONFIG_SIMPLE_TRACE)
 static void do_trace_file(Monitor *mon, const QDict *qdict)
 {
     const char *op = qdict_get_try_str(qdict, "op");
@@ -999,7 +1001,9 @@  static void do_info_trace(Monitor *mon)
 {
     st_print_trace((FILE *)mon, &monitor_fprintf);
 }
+#endif
 
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 static void do_info_trace_events(Monitor *mon)
 {
     st_print_trace_events((FILE *)mon, &monitor_fprintf);
@@ -3103,6 +3107,8 @@  static const mon_cmd_t info_cmds[] = {
         .help       = "show current contents of trace buffer",
         .mhandler.info = do_info_trace,
     },
+#endif
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
     {
         .name       = "trace-events",
         .args_type  = "",
diff --git a/qemu-config.c b/qemu-config.c
index 7c7357f..7a20971 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -297,7 +297,7 @@  static QemuOptsList qemu_mon_opts = {
     },
 };
 
-#if defined(CONFIG_SIMPLE_TRACE)
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 static QemuOptsList qemu_trace_opts = {
     .name = "trace",
     .implied_opt_name = "trace",
@@ -306,7 +306,8 @@  static QemuOptsList qemu_trace_opts = {
         {
             .name = "file",
             .type = QEMU_OPT_STRING,
-        },{
+        },
+        {
             .name = "events",
             .type = QEMU_OPT_STRING,
         },
@@ -464,7 +465,7 @@  static QemuOptsList *vm_config_groups[32] = {
     &qemu_global_opts,
     &qemu_mon_opts,
     &qemu_cpudef_opts,
-#if defined(CONFIG_SIMPLE_TRACE)
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
     &qemu_trace_opts,
 #endif
     &qemu_option_rom_opts,
diff --git a/qemu-options.hx b/qemu-options.hx
index ff8e75d..1bcc27b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2356,9 +2356,13 @@  Normally QEMU loads a configuration file from @var{sysconfdir}/qemu.conf and
 @var{sysconfdir}/target-@var{ARCH}.conf on startup.  The @code{-nodefconfig}
 option will prevent QEMU from loading these configuration files at startup.
 ETEXI
-#ifdef CONFIG_SIMPLE_TRACE
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
 DEF("trace", HAS_ARG, QEMU_OPTION_trace,
+#if defined(CONFIG_SIMPLE_TRACE)
     "-trace [file=<file>][,events=<file>]\n"
+#elif defined(CONFIG_STDERR_TRACE)
+    "-trace [events=<file>]\n"
+#endif
     "                specify tracing options\n",
     QEMU_ARCH_ALL)
 STEXI
@@ -2368,7 +2372,9 @@  STEXI
 Specify tracing options.
 
 @table @option
+#if defined(CONFIG_SIMPLE_TRACE)
 @item file=@var{file}
+#endif
 Log output traces to @var{file}.
 @item events=@var{file}
 Immediately enable events listed in @var{file}.
diff --git a/scripts/tracetool b/scripts/tracetool
index e3aec89..17e3f37 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -241,7 +241,12 @@  linetoh_begin_stderr()
 {
     cat <<EOF
 #include <stdio.h>
+#include "stderrtrace.h"
+
+extern TraceEvent trace_list[];
 EOF
+
+    stderr_event_num=0
 }
 
 linetoh_stderr()
@@ -260,29 +265,47 @@  linetoh_stderr()
     cat <<EOF
 static inline void trace_$name($args)
 {
-    fprintf(stderr, "$name $fmt\n" $argnames);
+    if (trace_list[$stderr_event_num].state != 0) {
+        fprintf(stderr, "$name $fmt\n" $argnames);
+    }
 }
 EOF
+    stderr_event_num=$((stderr_event_num + 1))
+
 }
 
 linetoh_end_stderr()
 {
-return
+    cat <<EOF
+#define NR_TRACE_EVENTS $stderr_event_num
+EOF
 }
 
 linetoc_begin_stderr()
 {
-return
+    cat <<EOF
+#include "trace.h"
+
+TraceEvent trace_list[] = {
+EOF
+    stderr_event_num=0
 }
 
 linetoc_stderr()
 {
-return
+    local name
+    name=$(get_name "$1")
+    cat <<EOF
+{.tp_name = "$name", .state=0},
+EOF
+    stderr_event_num=$(($stderr_event_num + 1))
 }
 
 linetoc_end_stderr()
 {
-return
+    cat <<EOF
+};
+EOF
 }
 #END OF STDERR
 
diff --git a/stderrtrace.c b/stderrtrace.c
new file mode 100644
index 0000000..dac373c
--- /dev/null
+++ b/stderrtrace.c
@@ -0,0 +1,24 @@ 
+#include "trace.h"
+
+void st_print_trace_events(FILE *stream, int (*stream_printf)(FILE *stream, const char *fmt, ...))
+{
+    unsigned int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        stream_printf(stream, "%s [Event ID %u] : state %u\n",
+                      trace_list[i].tp_name, i, trace_list[i].state);
+    }
+}
+
+bool st_change_trace_event_state(const char *name, bool enabled)
+{
+    int i;
+
+    for (i = 0; i < NR_TRACE_EVENTS; i++) {
+        if (!strcmp(trace_list[i].tp_name, name)) {
+            trace_list[i].state = enabled;
+            return true;
+        }
+    }
+    return false;
+}
diff --git a/stderrtrace.h b/stderrtrace.h
new file mode 100644
index 0000000..48ac36a
--- /dev/null
+++ b/stderrtrace.h
@@ -0,0 +1,14 @@ 
+#ifndef _STDERRTRACE_H_
+#define _STDERRTRACE_H_
+
+typedef uint64_t TraceEventID;
+
+typedef struct {
+    const char *tp_name;
+    bool state;
+} TraceEvent;
+
+void st_print_trace_events(FILE *stream, fprintf_function stream_printf);
+bool st_change_trace_event_state(const char *name, bool enabled);
+
+#endif /* ! _STDERRTRACE_H_ */
diff --git a/vl.c b/vl.c
index 35d440d..67dc530 100644
--- a/vl.c
+++ b/vl.c
@@ -1966,7 +1966,7 @@  int main(int argc, char **argv, char **envp)
     int show_vnc_port = 0;
 #endif
     int defconfig = 1;
-#if defined(CONFIG_SIMPLE_TRACE)
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
     const char *trace_file = NULL;
     const char *trace_events_file = NULL;
 #endif
@@ -2762,7 +2762,7 @@  int main(int argc, char **argv, char **envp)
                 }
                 xen_mode = XEN_ATTACH;
                 break;
-#if defined(CONFIG_SIMPLE_TRACE)
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
             case QEMU_OPTION_trace:
                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
                 if (opts) {
@@ -2820,6 +2820,13 @@  int main(int argc, char **argv, char **envp)
     if (!st_init(trace_file)) {
         fprintf(stderr, "warning: unable to initialize simple trace backend\n");
     }
+#else
+    if (trace_file) {
+        fprintf(stderr, "option \"-trace file=\" not supported\n");
+        exit(1);
+    }
+#endif
+#if defined(CONFIG_SIMPLE_TRACE) || defined(CONFIG_STDERR_TRACE)
     if (trace_events_file) {
         FILE *trace_events_fp = fopen(trace_events_file, "r");
         if (!trace_events_fp) {