* [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
@ 2014-01-28 4:35 Kazuya Saito
2014-01-30 21:00 ` Stefan Hajnoczi
2014-01-31 10:37 ` Stefan Hajnoczi
0 siblings, 2 replies; 8+ messages in thread
From: Kazuya Saito @ 2014-01-28 4:35 UTC (permalink / raw)
To: stefanha, qemu-devel@nongnu.org
This patch implements "multi tracing backend" which enables several
tracing backend simultaneously.
QEMU has multiple trace backends, but one of them needs to be chosen at
compile time. When investigating issues of QEMU, it'd be much more
convenient if we can use multiple trace backends without recompiling,
especially 'ftrace backend' and 'dtrace backend'. From the performance
perspective, 'ftrace backend' should be the one which runs while the
system is in operation, and it provides useful information. But, for
some issues, 'dtrace backend' is needed for investigation as 'dtrace
backend' provides more information. This patch enables both 'ftrace
backend' and 'dtrace backend' (, and some other backends if necessary)
at compile time so that we can switch between the two without
recompiling.
usage:
We have only to set some tracing backends as follows.
$ ./configure --enable-trace-backend=ftrace,dtrace
Of course, we can compile with single backend as well as before.
$ ./configure --enable-trace-backend=simple
Note: Now, we can select nop, ftrace, dtrace, stderr, ust, simple as
tracing backend. However, we can select ftrace, dtrace, stderr,
simple when selecting more than two backends. Though it's
needless to say about nop, I excluded ust backend because I didn't
test it since it doesn't support LTTng 2.x.
Signed-off-by: Kazuya Saito <saito.kazuya@jp.fujitsu.com>
---
Makefile | 2 +-
configure | 68 ++++++++++---------
scripts/tracetool.py | 9 ++-
scripts/tracetool/__init__.py | 21 ++++--
scripts/tracetool/backend/__init__.py | 20 ++++--
scripts/tracetool/backend/common.py | 78 +++++++++++++++++++++
scripts/tracetool/backend/dtrace.py | 107 +++++++++++++++++------------
scripts/tracetool/backend/ftrace.py | 60 +++++++++-------
scripts/tracetool/backend/simple.py | 124 +++++++++++++++++----------------
scripts/tracetool/backend/stderr.py | 44 +++++++-----
trace/Makefile.objs | 22 ++++---
trace/ftrace.c | 25 +------
trace/ftrace.h | 1 +
trace/multi.c | 52 ++++++++++++++
trace/simple.c | 18 +-----
trace/simple.h | 2 +-
trace/stderr.c | 30 --------
17 files changed, 403 insertions(+), 280 deletions(-)
create mode 100644 scripts/tracetool/backend/common.py
create mode 100644 trace/multi.c
delete mode 100644 trace/stderr.c
diff --git a/Makefile b/Makefile
index bdff4e4..7bcacf5 100644
--- a/Makefile
+++ b/Makefile
@@ -52,7 +52,7 @@ GENERATED_HEADERS += trace/generated-events.h
GENERATED_SOURCES += trace/generated-events.c
GENERATED_HEADERS += trace/generated-tracers.h
-ifeq ($(TRACE_BACKEND),dtrace)
+ifeq ($(findstring dtrace,$(TRACE_BACKEND)),dtrace)
GENERATED_HEADERS += trace/generated-tracers-dtrace.h
endif
GENERATED_SOURCES += trace/generated-tracers.c
diff --git a/configure b/configure
index 3782a6a..ce3d7d6 100755
--- a/configure
+++ b/configure
@@ -3375,15 +3375,18 @@ fi
##########################################
# For 'dtrace' backend, test if 'dtrace' command is present
-if test "$trace_backend" = "dtrace"; then
- if ! has 'dtrace' ; then
- error_exit "dtrace command is not found in PATH $PATH"
- fi
- trace_backend_stap="no"
- if has 'stap' ; then
- trace_backend_stap="yes"
+IFS=','
+for backend in ${trace_backend}; do
+ if test "$backend" = "dtrace"; then
+ if ! has 'dtrace' ; then
+ error_exit "dtrace command is not found in PATH $PATH"
+ fi
+ trace_backend_stap="no"
+ if has 'stap' ; then
+ trace_backend_stap="yes"
+ fi
fi
-fi
+done
##########################################
# check and set a backend for coroutine
@@ -4262,33 +4265,34 @@ echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
if test "$trace_backend" = "nop"; then
echo "CONFIG_TRACE_NOP=y" >> $config_host_mak
fi
-if test "$trace_backend" = "simple"; then
- echo "CONFIG_TRACE_SIMPLE=y" >> $config_host_mak
- trace_default=no
- # Set the appropriate trace file.
- trace_file="\"$trace_file-\" FMT_pid"
-fi
-if test "$trace_backend" = "stderr"; then
- echo "CONFIG_TRACE_STDERR=y" >> $config_host_mak
- trace_default=no
-fi
-if test "$trace_backend" = "ust"; then
- echo "CONFIG_TRACE_UST=y" >> $config_host_mak
-fi
-if test "$trace_backend" = "dtrace"; then
- echo "CONFIG_TRACE_DTRACE=y" >> $config_host_mak
- if test "$trace_backend_stap" = "yes" ; then
- echo "CONFIG_TRACE_SYSTEMTAP=y" >> $config_host_mak
+
+for backend in ${trace_backend}; do
+ if test "$backend" = "simple"; then
+ echo "CONFIG_TRACE_SIMPLE=y" >> $config_host_mak
+ trace_default=no
+ # Set the appropriate trace file.
+ trace_file="\"$trace_file-\" FMT_pid"
fi
-fi
-if test "$trace_backend" = "ftrace"; then
- if test "$linux" = "yes" ; then
- echo "CONFIG_TRACE_FTRACE=y" >> $config_host_mak
+ if test "$backend" = "stderr"; then
+ echo "CONFIG_TRACE_STDERR=y" >> $config_host_mak
trace_default=no
- else
- feature_not_found "ftrace(trace backend)"
fi
-fi
+ if test "$backend" = "dtrace"; then
+ echo "CONFIG_TRACE_DTRACE=y" >> $config_host_mak
+ if test "$trace_backend_stap" = "yes" ; then
+ echo "CONFIG_TRACE_SYSTEMTAP=y" >> $config_host_mak
+ fi
+ fi
+ if test "$backend" = "ftrace"; then
+ if test "$linux" = "yes" ; then
+ echo "CONFIG_TRACE_FTRACE=y" >> $config_host_mak
+ trace_default=no
+ else
+ feature_not_found "ftrace(trace backend)"
+ fi
+ fi
+done
+
echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
if test "$trace_default" = "yes"; then
echo "CONFIG_TRACE_DEFAULT=y" >> $config_host_mak
diff --git a/scripts/tracetool.py b/scripts/tracetool.py
index 5f4890f..2c7c0c0 100755
--- a/scripts/tracetool.py
+++ b/scripts/tracetool.py
@@ -112,10 +112,11 @@ def main(args):
error_opt("backend not set")
if check_backend:
- if tracetool.backend.exists(arg_backend):
- sys.exit(0)
- else:
- sys.exit(1)
+ for backend in arg_backend.split(","):
+ if tracetool.backend.exists(backend):
+ sys.exit(0)
+ else:
+ sys.exit(1)
if arg_format == "stap":
if binary is None:
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 175df08..a0addb5 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -242,14 +242,19 @@ def generate(fevents, format, backend,
if not tracetool.format.exists(mformat):
raise TracetoolError("unknown format: %s" % format)
- backend = str(backend)
+ backends = str(backend).split(",")
if len(backend) is 0:
raise TracetoolError("backend not set")
- mbackend = backend.replace("-", "_")
- if not tracetool.backend.exists(mbackend):
- raise TracetoolError("unknown backend: %s" % backend)
- if not tracetool.backend.compatible(mbackend, mformat):
+ compat = False
+ for backend in backends:
+ mbackend = backend.replace("-", "_")
+ if not tracetool.backend.exists(mbackend):
+ raise TracetoolError("unknown backend: %s" % backend)
+
+ compat |= tracetool.backend.compatible(mbackend, mformat)
+
+ if not compat:
raise TracetoolError("backend '%s' not compatible with format '%s'" %
(backend, format))
@@ -259,15 +264,15 @@ def generate(fevents, format, backend,
events = _read_events(fevents)
- if backend == "nop":
+ if backends == ["nop"]:
( e.properies.add("disable") for e in events )
tracetool.format.generate_begin(mformat, events)
- tracetool.backend.generate("nop", format,
+ tracetool.backend.generate(["nop"], format,
[ e
for e in events
if "disable" in e.properties ])
- tracetool.backend.generate(backend, format,
+ tracetool.backend.generate(backends, format,
[ e
for e in events
if "disable" not in e.properties ])
diff --git a/scripts/tracetool/backend/__init__.py b/scripts/tracetool/backend/__init__.py
index f0314ee..40efcb5 100644
--- a/scripts/tracetool/backend/__init__.py
+++ b/scripts/tracetool/backend/__init__.py
@@ -110,20 +110,28 @@ def compatible(backend, format):
def _empty(events):
pass
-def generate(backend, format, events):
+def generate(backends, format, events):
"""Generate the per-event output for the given (backend, format) pair."""
- if not compatible(backend, format):
+ compat = False
+ for backend in backends:
+ compat |= compatible(backend, format)
+ if not compat:
raise ValueError("backend '%s' not compatible with format '%s'" %
(backend, format))
- backend = backend.replace("-", "_")
+ backends = map(lambda x: x.replace("-", "_"), backends)
format = format.replace("-", "_")
- if backend == "nop":
+ if backends == ["nop"]:
func = tracetool.try_import("tracetool.format." + format,
"nop", _empty)[1]
+ func(events)
+ elif set(backends).issubset(["dtrace", "ftrace", "simple", "stderr"]):
+ func = tracetool.try_import("tracetool.backend.common",
+ format, None)[1]
+ func(backends, events)
else:
func = tracetool.try_import("tracetool.backend." + backend,
format, None)[1]
-
- func(events)
+ func(events)
+
diff --git a/scripts/tracetool/backend/common.py b/scripts/tracetool/backend/common.py
new file mode 100644
index 0000000..1e1da06
--- /dev/null
+++ b/scripts/tracetool/backend/common.py
@@ -0,0 +1,78 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+"""
+Common part of tracing backend.
+"""
+
+__author__ = "Kazuya Saito <saito.kazuya@jp.fujitsu.com>"
+__copyright__ = "Copyright (C) 2013 Fujitsu, Ltd."
+__license__ = "GPL version 2 or (at your option) any later version"
+
+__maintainer__ = "Stefan Hajnoczi"
+__email__ = "stefanha@redhat.com"
+
+
+from tracetool import out
+import tracetool
+
+PUBLIC = False
+
+def c(backends, events):
+ func_head_lst = []
+ func_body_lst = []
+ for backend in backends:
+ func_head_lst.append(tracetool.try_import("tracetool.backend." + backend,
+ "c_head", None)[1])
+ func_body_lst.append(tracetool.try_import("tracetool.backend." + backend,
+ "c_body", None)[1])
+
+ out('#include "trace/control.h"',
+ '#ifndef CONFIG_TRACE_DTRACE',
+ '#include "trace.h"',
+ '#endif'
+ '',
+ )
+ for func_head in func_head_lst:
+ func_head()
+
+ for num, event in enumerate(events):
+ out('void trace_%(name)s(%(args)s) {',
+ name = event.name,
+ args = event.args)
+
+ for func_body in func_body_lst:
+ func_body(num, event)
+ out('}')
+
+def h(backends, events):
+ func_head_lst = []
+ func_body_lst = []
+ for backend in backends:
+ func_head_lst.append(tracetool.try_import("tracetool.backend." + backend,
+ "h_head", None)[1])
+ func_body_lst.append(tracetool.try_import("tracetool.backend." + backend,
+ "h_body", None)[1])
+
+ for func_head in func_head_lst:
+ func_head()
+
+ for event in events:
+ out('void trace_%(name)s(%(args)s);',
+ name = event.name,
+ args = event.args)
+
+ for func_body in func_body_lst:
+ func_body(event)
+
+def stap(backends, events):
+ # Only dtrace backend has format "stap"
+ func = tracetool.try_import("tracetool.backend.dtrace",
+ "stap", None)[1]
+ func(events)
+
+def d(backends, events):
+ # Only dtrace backend has format "d"
+ func = tracetool.try_import("tracetool.backend.dtrace",
+ "d", None)[1]
+ func(events)
diff --git a/scripts/tracetool/backend/dtrace.py b/scripts/tracetool/backend/dtrace.py
index e31bc79..e8968c5 100644
--- a/scripts/tracetool/backend/dtrace.py
+++ b/scripts/tracetool/backend/dtrace.py
@@ -38,43 +38,56 @@ def _binary():
def c(events):
pass
+def c_head():
+ out('#include <sys/sdt.h>',
+ '#include "trace/generated-tracers.h"',
+ '')
+
+def c_body(num, event):
+ out('{ /* dtrace */',
+ ' QEMU_%(uppername)s(%(argnames)s);',
+ '}',
+ name = event.name,
+ args = event.args,
+ uppername = event.name.upper(),
+ argnames = ", ".join(event.args.names()),
+ )
def h(events):
+ pass
+
+def h_head():
out('#include "trace/generated-tracers-dtrace.h"',
'')
-
- for e in events:
- out('static inline void trace_%(name)s(%(args)s) {',
- ' QEMU_%(uppername)s(%(argnames)s);',
- '}',
- name = e.name,
- args = e.args,
- uppername = e.name.upper(),
- argnames = ", ".join(e.args.names()),
- )
-
+
+def h_body(event):
+ pass
def d(events):
+ d_head()
out('provider qemu {')
-
for e in events:
- args = str(e.args)
+ d_body(e)
+ out('',
+ '};')
+
+def d_head():
+ pass
- # DTrace provider syntax expects foo() for empty
- # params, not foo(void)
- if args == 'void':
- args = ''
+def d_body(event):
+ args = str(event.args)
- # Define prototype for probe arguments
- out('',
- 'probe %(name)s(%(args)s);',
- name = e.name,
- args = args,
- )
+ # DTrace provider syntax expects foo() for empty
+ # params, not foo(void)
+ if args == 'void':
+ args = ''
+ # Define prototype for probe arguments
out('',
- '};')
-
+ 'probe %(name)s(%(args)s);',
+ name = event.name,
+ args = args,
+ )
# Technically 'self' is not used by systemtap yet, but
# they recommended we keep it in the reserved list anyway
@@ -86,24 +99,30 @@ RESERVED_WORDS = (
)
def stap(events):
+ stap_head()
for e in events:
- # Define prototype for probe arguments
- out('probe %(probeprefix)s.%(name)s = process("%(binary)s").mark("%(name)s")',
- '{',
- probeprefix = _probeprefix(),
- name = e.name,
- binary = _binary(),
- )
-
- i = 1
- if len(e.args) > 0:
- for name in e.args.names():
- # Append underscore to reserved keywords
- if name in RESERVED_WORDS:
- name += '_'
- out(' %s = $arg%d;' % (name, i))
- i += 1
-
- out('}')
-
+ stap_body(e)
out()
+
+def stap_head():
+ pass
+
+def stap_body(event):
+ # Define prototype for probe arguments
+ out('probe %(probeprefix)s.%(name)s = process("%(binary)s").mark("%(name)s")',
+ '{',
+ probeprefix = _probeprefix(),
+ name = event.name,
+ binary = _binary(),
+ )
+
+ i = 1
+ if len(event.args) > 0:
+ for name in event.args.names():
+ # Append underscore to reserved keywords
+ if name in RESERVED_WORDS:
+ name += '_'
+ out(' %s = $arg%d;' % (name, i))
+ i += 1
+
+ out('}')
diff --git a/scripts/tracetool/backend/ftrace.py b/scripts/tracetool/backend/ftrace.py
index 888c361..52283a3 100644
--- a/scripts/tracetool/backend/ftrace.py
+++ b/scripts/tracetool/backend/ftrace.py
@@ -22,33 +22,43 @@ PUBLIC = True
def c(events):
pass
+def c_head():
+ out('#include "trace/ftrace.h"',
+ '',
+ )
+
+def c_body(num, event):
+ argnames = ", ".join(event.args.names())
+ if len(event.args) > 0:
+ argnames = ", " + argnames
+
+ out('{ /* ftrace */',
+ ' char ftrace_buf[MAX_TRACE_STRLEN];',
+ ' int unused __attribute__ ((unused));',
+ ' int trlen;',
+ ' bool _state = trace_event_get_state(%(event_id)s);',
+ ' if (_state) {',
+ ' trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
+ ' "%(name)s " %(fmt)s "\\n" %(argnames)s);',
+ ' trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
+ ' unused = write(trace_marker_fd, ftrace_buf, trlen);',
+ ' }',
+ '}',
+ name = event.name,
+ args = event.args,
+ event_id = "TRACE_" + event.name.upper(),
+ fmt = event.fmt.rstrip("\n"),
+ argnames = argnames,
+ )
+
+
def h(events):
+ pass
+
+def h_head():
out('#include "trace/ftrace.h"',
- '#include "trace/control.h"',
'',
)
- for e in events:
- argnames = ", ".join(e.args.names())
- if len(e.args) > 0:
- argnames = ", " + argnames
-
- out('static inline void trace_%(name)s(%(args)s)',
- '{',
- ' char ftrace_buf[MAX_TRACE_STRLEN];',
- ' int unused __attribute__ ((unused));',
- ' int trlen;',
- ' bool _state = trace_event_get_state(%(event_id)s);',
- ' if (_state) {',
- ' trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
- ' "%(name)s " %(fmt)s "\\n" %(argnames)s);',
- ' trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
- ' unused = write(trace_marker_fd, ftrace_buf, trlen);',
- ' }',
- '}',
- name = e.name,
- args = e.args,
- event_id = "TRACE_" + e.name.upper(),
- fmt = e.fmt.rstrip("\n"),
- argnames = argnames,
- )
+def h_body(event):
+ pass
diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index 37ef599..59d6104 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -27,77 +27,79 @@ def is_string(arg):
return False
def c(events):
- out('#include "trace.h"',
- '#include "trace/control.h"',
- '#include "trace/simple.h"',
+ pass
+
+def c_head():
+ out('#include "trace/simple.h"',
+ '',
+ )
+
+def c_body(num, event):
+ out('{ /* simple */',
+ ' TraceBufferRecord rec;',
+ name = event.name,
+ args = event.args,
+ )
+ sizes = []
+ for type_, name in event.args:
+ if is_string(type_):
+ out(' size_t arg%(name)s_len = %(name)s ? MIN(strlen(%(name)s), MAX_TRACE_STRLEN) : 0;',
+ name = name,
+ )
+ strsizeinfo = "4 + arg%s_len" % name
+ sizes.append(strsizeinfo)
+ else:
+ sizes.append("8")
+ sizestr = " + ".join(sizes)
+ if len(event.args) == 0:
+ sizestr = '0'
+
+
+ out('',
+ ' TraceEvent *eventp = trace_event_id(%(event_id)s);',
+ ' bool _state = trace_event_get_state_dynamic(eventp);',
+ ' if (!_state) {',
+ ' return;',
+ ' }',
'',
+ ' if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
+ ' return; /* Trace Buffer Full, Event Dropped ! */',
+ ' }',
+ event_id = num,
+ size_str = sizestr,
)
- for num, event in enumerate(events):
- out('void trace_%(name)s(%(args)s)',
- '{',
- ' TraceBufferRecord rec;',
- name = event.name,
- args = event.args,
- )
- sizes = []
+ if len(event.args) > 0:
for type_, name in event.args:
+ # string
if is_string(type_):
- out(' size_t arg%(name)s_len = %(name)s ? MIN(strlen(%(name)s), MAX_TRACE_STRLEN) : 0;',
+ out(' trace_record_write_str(&rec, %(name)s, arg%(name)s_len);',
+ name = name,
+ )
+ # pointer var (not string)
+ elif type_.endswith('*'):
+ out(' trace_record_write_u64(&rec, (uintptr_t)(uint64_t *)%(name)s);',
name = name,
)
- strsizeinfo = "4 + arg%s_len" % name
- sizes.append(strsizeinfo)
+ # primitive data type
else:
- sizes.append("8")
- sizestr = " + ".join(sizes)
- if len(event.args) == 0:
- sizestr = '0'
-
-
- out('',
- ' TraceEvent *eventp = trace_event_id(%(event_id)s);',
- ' bool _state = trace_event_get_state_dynamic(eventp);',
- ' if (!_state) {',
- ' return;',
- ' }',
- '',
- ' if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
- ' return; /* Trace Buffer Full, Event Dropped ! */',
- ' }',
- event_id = num,
- size_str = sizestr,
- )
-
- if len(event.args) > 0:
- for type_, name in event.args:
- # string
- if is_string(type_):
- out(' trace_record_write_str(&rec, %(name)s, arg%(name)s_len);',
- name = name,
- )
- # pointer var (not string)
- elif type_.endswith('*'):
- out(' trace_record_write_u64(&rec, (uintptr_t)(uint64_t *)%(name)s);',
- name = name,
- )
- # primitive data type
- else:
- out(' trace_record_write_u64(&rec, (uint64_t)%(name)s);',
- name = name,
- )
-
- out(' trace_record_finish(&rec);',
- '}',
- '')
+ out(' trace_record_write_u64(&rec, (uint64_t)%(name)s);',
+ name = name,
+ )
+
+ out(' trace_record_finish(&rec);',
+ '}',
+ '')
def h(events):
+ h_head()
+ h_body()
+
+def h_head():
out('#include "trace/simple.h"',
- '')
+ '',
+ )
- for event in events:
- out('void trace_%(name)s(%(args)s);',
- name = event.name,
- args = event.args,
- )
+def h_body(event):
+ pass
diff --git a/scripts/tracetool/backend/stderr.py b/scripts/tracetool/backend/stderr.py
index 6f93dbd..0d36397 100644
--- a/scripts/tracetool/backend/stderr.py
+++ b/scripts/tracetool/backend/stderr.py
@@ -22,27 +22,35 @@ PUBLIC = True
def c(events):
pass
+def c_head():
+ pass
+
+def c_body(num, event):
+ argnames = ", ".join(event.args.names())
+ if len(event.args) > 0:
+ argnames = ", " + argnames
+
+ out('{ /* stderr */',
+ ' bool _state = trace_event_get_state(%(event_id)s);',
+ ' if (_state) {',
+ ' fprintf(stderr, "%(name)s " %(fmt)s "\\n" %(argnames)s);',
+ ' }',
+ '}',
+ name = event.name,
+ args = event.args,
+ event_id = "TRACE_" + event.name.upper(),
+ fmt = event.fmt.rstrip("\n"),
+ argnames = argnames,
+ )
+
def h(events):
+ pass
+
+def h_head():
out('#include <stdio.h>',
'#include "trace/control.h"',
'',
)
- for e in events:
- argnames = ", ".join(e.args.names())
- if len(e.args) > 0:
- argnames = ", " + argnames
-
- out('static inline void trace_%(name)s(%(args)s)',
- '{',
- ' bool _state = trace_event_get_state(%(event_id)s);',
- ' if (_state) {',
- ' fprintf(stderr, "%(name)s " %(fmt)s "\\n" %(argnames)s);',
- ' }',
- '}',
- name = e.name,
- args = e.args,
- event_id = "TRACE_" + e.name.upper(),
- fmt = e.fmt.rstrip("\n"),
- argnames = argnames,
- )
+def h_body(event):
+ pass
diff --git a/trace/Makefile.objs b/trace/Makefile.objs
index 3b88e49..7eb0f69 100644
--- a/trace/Makefile.objs
+++ b/trace/Makefile.objs
@@ -35,8 +35,7 @@ $(obj)/generated-tracers.h-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/conf
######################################################################
# Auto-generated tracing routines (non-DTrace)
-
-ifneq ($(TRACE_BACKEND),dtrace)
+ifneq ($(findstring dtrace,$(TRACE_BACKEND)),dtrace)
$(obj)/generated-tracers.c: $(obj)/generated-tracers.c-timestamp
@cmp -s $< $@ || cp $< $@
$(obj)/generated-tracers.c-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/config-host.mak
@@ -44,18 +43,23 @@ $(obj)/generated-tracers.c-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/conf
--format=c \
--backend=$(TRACE_BACKEND) \
< $< > $@," GEN $(patsubst %-timestamp,%,$@)")
-
$(obj)/generated-tracers.o: $(obj)/generated-tracers.c $(obj)/generated-tracers.h
endif
-
######################################################################
# Auto-generated DTrace code
# Normal practice is to name DTrace probe file with a '.d' extension
# but that gets picked up by QEMU's Makefile as an external dependency
# rule file. So we use '.dtrace' instead
-ifeq ($(TRACE_BACKEND),dtrace)
+ifeq ($(findstring dtrace,$(TRACE_BACKEND)),dtrace)
+$(obj)/generated-tracers.c: $(obj)/generated-tracers.c-timestamp
+ @cmp -s $< $@ || cp $< $@
+$(obj)/generated-tracers.c-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/config-host.mak
+ $(call quiet-command,$(TRACETOOL) \
+ --format=c \
+ --backend=$(TRACE_BACKEND) \
+ < $< > $@," GEN $(patsubst %-timestamp,%,$@)")
$(obj)/generated-tracers.dtrace: $(obj)/generated-tracers.dtrace-timestamp
$(obj)/generated-tracers.dtrace-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/config-host.mak
$(call quiet-command,$(TRACETOOL) \
@@ -67,15 +71,15 @@ $(obj)/generated-tracers.dtrace-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)
$(obj)/generated-tracers-dtrace.h: $(obj)/generated-tracers.dtrace
$(call quiet-command,dtrace -o $@ -h -s $<, " GEN $@")
-$(obj)/generated-tracers.o: $(obj)/generated-tracers.dtrace
+$(obj)/generated-tracers.o: $(obj)/generated-tracers.dtrace $(obj)/generated-tracers.c $(obj)/generated-tracers.h
endif
######################################################################
# Backend code
util-obj-$(CONFIG_TRACE_DEFAULT) += default.o
-util-obj-$(CONFIG_TRACE_SIMPLE) += simple.o
-util-obj-$(CONFIG_TRACE_STDERR) += stderr.o
-util-obj-$(CONFIG_TRACE_FTRACE) += ftrace.o
+util-obj-$(CONFIG_TRACE_SIMPLE) += multi.o simple.o
+util-obj-$(CONFIG_TRACE_STDERR) += multi.o
+util-obj-$(CONFIG_TRACE_FTRACE) += multi.o ftrace.o
util-obj-y += control.o
util-obj-y += generated-tracers.o
diff --git a/trace/ftrace.c b/trace/ftrace.c
index 46b7fdb..e2f118e 100644
--- a/trace/ftrace.c
+++ b/trace/ftrace.c
@@ -42,35 +42,13 @@ static int find_debugfs(char *debugfs)
return 1;
}
-void trace_print_events(FILE *stream, fprintf_function stream_printf)
-{
- TraceEventID i;
-
- for (i = 0; i < trace_event_count(); i++) {
- TraceEvent *ev = trace_event_id(i);
- stream_printf(stream, "%s [Event ID %u] : state %u\n",
- trace_event_get_name(ev), i, trace_event_get_state_dynamic(ev));
- }
-}
-
-void trace_event_set_state_dynamic_backend(TraceEvent *ev, bool state)
-{
- ev->dstate = state;
-}
-
-bool trace_backend_init(const char *events, const char *file)
+bool ftrace_backend_init(const char *events, const char *file)
{
char debugfs[PATH_MAX];
char path[PATH_MAX];
int debugfs_found;
int trace_fd = -1;
- if (file) {
- fprintf(stderr, "error: -trace file=...: "
- "option not supported by the selected tracing backend\n");
- return false;
- }
-
debugfs_found = find_debugfs(debugfs);
if (debugfs_found) {
snprintf(path, PATH_MAX, "%s/tracing/tracing_on", debugfs);
@@ -97,6 +75,5 @@ bool trace_backend_init(const char *events, const char *file)
return false;
}
- trace_backend_init_events(events);
return true;
}
diff --git a/trace/ftrace.h b/trace/ftrace.h
index 94cb8d5..f508f33 100644
--- a/trace/ftrace.h
+++ b/trace/ftrace.h
@@ -6,5 +6,6 @@
#define STR(x) _STR(x)
extern int trace_marker_fd;
+bool ftrace_backend_init(const char *events, const char *file);
#endif /* ! TRACE_FTRACE_H */
diff --git a/trace/multi.c b/trace/multi.c
new file mode 100644
index 0000000..ab2b79f
--- /dev/null
+++ b/trace/multi.c
@@ -0,0 +1,52 @@
+/*
+ * Multi trace backend
+ */
+
+#include <stdio.h>
+#include "trace.h"
+#include "trace/control.h"
+
+void trace_print_events(FILE *stream, fprintf_function stream_printf)
+{
+ TraceEventID i;
+
+ for (i = 0; i < trace_event_count(); i++) {
+ TraceEvent *ev = trace_event_id(i);
+ stream_printf(stream, "%s [Event ID %u] : state %u\n",
+ trace_event_get_name(ev), i, trace_event_get_state_dynamic(ev));
+ }
+}
+
+void trace_event_set_state_dynamic_backend(TraceEvent *ev, bool state)
+{
+ ev->dstate = state;
+}
+
+bool trace_backend_init(const char *events, const char *file)
+{
+ bool retval = true;
+
+#ifndef CONFIG_TRACE_SIMPLE
+ if (file) {
+ fprintf(stderr, "error: -trace file=...: "
+ "option not supported by the selected tracing backend\n");
+ return false;
+ }
+#endif
+
+#ifdef CONFIG_TRACE_SIMPLE
+ retval &= simpletrace_backend_init(events, file);
+#endif
+
+#ifdef CONFIG_TRACE_FTRACE
+ retval &= ftrace_backend_init(events, file);
+#endif
+
+ if (!retval){
+ fprintf(stderr, "failed to initialize tracing backend.\n");
+ return false;
+ }
+
+ trace_backend_init_events(events);
+ return true;
+}
diff --git a/trace/simple.c b/trace/simple.c
index 1e3f691..75c8e17 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -356,22 +356,6 @@ void st_flush_trace_buffer(void)
flush_trace_file(true);
}
-void trace_print_events(FILE *stream, fprintf_function stream_printf)
-{
- unsigned int i;
-
- for (i = 0; i < trace_event_count(); i++) {
- TraceEvent *ev = trace_event_id(i);
- stream_printf(stream, "%s [Event ID %u] : state %u\n",
- trace_event_get_name(ev), i, trace_event_get_state_dynamic(ev));
- }
-}
-
-void trace_event_set_state_dynamic_backend(TraceEvent *ev, bool state)
-{
- ev->dstate = state;
-}
-
/* Helper function to create a thread with signals blocked. Use glib's
* portable threads since QEMU abstractions cannot be used due to reentrancy in
* the tracer. Also note the signal masking on POSIX hosts so that the thread
@@ -400,7 +384,7 @@ static GThread *trace_thread_create(GThreadFunc fn)
return thread;
}
-bool trace_backend_init(const char *events, const char *file)
+bool simpletrace_backend_init(const char *events, const char *file)
{
GThread *thread;
diff --git a/trace/simple.h b/trace/simple.h
index 5260d9a..da9b7f6 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -53,5 +53,5 @@ void trace_record_write_str(TraceBufferRecord *rec, const char *s, uint32_t slen
* Don't append any more arguments to the trace record after calling this.
*/
void trace_record_finish(TraceBufferRecord *rec);
-
+bool simpletrace_backend_init(const char *events, const char *file);
#endif /* TRACE_SIMPLE_H */
diff --git a/trace/stderr.c b/trace/stderr.c
deleted file mode 100644
index e212efd..0000000
--- a/trace/stderr.c
+++ /dev/null
@@ -1,30 +0,0 @@
-#include "trace.h"
-#include "trace/control.h"
-
-
-void trace_print_events(FILE *stream, fprintf_function stream_printf)
-{
- TraceEventID i;
-
- for (i = 0; i < trace_event_count(); i++) {
- TraceEvent *ev = trace_event_id(i);
- stream_printf(stream, "%s [Event ID %u] : state %u\n",
- trace_event_get_name(ev), i, trace_event_get_state_dynamic(ev));
- }
-}
-
-void trace_event_set_state_dynamic_backend(TraceEvent *ev, bool state)
-{
- ev->dstate = state;
-}
-
-bool trace_backend_init(const char *events, const char *file)
-{
- if (file) {
- fprintf(stderr, "error: -trace file=...: "
- "option not supported by the selected tracing backend\n");
- return false;
- }
- trace_backend_init_events(events);
- return true;
-}
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-01-28 4:35 [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend Kazuya Saito
@ 2014-01-30 21:00 ` Stefan Hajnoczi
2014-02-04 5:24 ` Kazuya Saito
2014-01-31 10:37 ` Stefan Hajnoczi
1 sibling, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-01-30 21:00 UTC (permalink / raw)
To: Kazuya Saito; +Cc: qemu-devel@nongnu.org, stefanha
On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
Some initial comments, I will continue reviewing tomorrow.
> This patch implements "multi tracing backend" which enables several
> tracing backend simultaneously.
>
> QEMU has multiple trace backends, but one of them needs to be chosen at
> compile time. When investigating issues of QEMU, it'd be much more
> convenient if we can use multiple trace backends without recompiling,
> especially 'ftrace backend' and 'dtrace backend'. From the performance
> perspective, 'ftrace backend' should be the one which runs while the
> system is in operation, and it provides useful information. But, for
> some issues, 'dtrace backend' is needed for investigation as 'dtrace
> backend' provides more information. This patch enables both 'ftrace
> backend' and 'dtrace backend' (, and some other backends if necessary)
> at compile time so that we can switch between the two without
> recompiling.
>
> usage:
> We have only to set some tracing backends as follows.
>
> $ ./configure --enable-trace-backend=ftrace,dtrace
>
> Of course, we can compile with single backend as well as before.
>
> $ ./configure --enable-trace-backend=simple
Great, this functionality has been suggested before so I'm sure it will
come in handy.
> Note: Now, we can select nop, ftrace, dtrace, stderr, ust, simple as
> tracing backend. However, we can select ftrace, dtrace, stderr,
> simple when selecting more than two backends. Though it's
> needless to say about nop, I excluded ust backend because I didn't
> test it since it doesn't support LTTng 2.x.
I have just reviewed and merged the LTTng 2.x patch series.
You can base your patch on:
git://github.com/stefanha/qemu.git tracing-next
> Signed-off-by: Kazuya Saito <saito.kazuya@jp.fujitsu.com>
> ---
> Makefile | 2 +-
> configure | 68 ++++++++++---------
> scripts/tracetool.py | 9 ++-
> scripts/tracetool/__init__.py | 21 ++++--
> scripts/tracetool/backend/__init__.py | 20 ++++--
> scripts/tracetool/backend/common.py | 78 +++++++++++++++++++++
> scripts/tracetool/backend/dtrace.py | 107 +++++++++++++++++------------
> scripts/tracetool/backend/ftrace.py | 60 +++++++++-------
> scripts/tracetool/backend/simple.py | 124 +++++++++++++++++----------------
> scripts/tracetool/backend/stderr.py | 44 +++++++-----
> trace/Makefile.objs | 22 ++++---
> trace/ftrace.c | 25 +------
> trace/ftrace.h | 1 +
> trace/multi.c | 52 ++++++++++++++
> trace/simple.c | 18 +-----
> trace/simple.h | 2 +-
> trace/stderr.c | 30 --------
> 17 files changed, 403 insertions(+), 280 deletions(-)
> create mode 100644 scripts/tracetool/backend/common.py
> create mode 100644 trace/multi.c
> delete mode 100644 trace/stderr.c
>
> diff --git a/Makefile b/Makefile
> index bdff4e4..7bcacf5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -52,7 +52,7 @@ GENERATED_HEADERS += trace/generated-events.h
> GENERATED_SOURCES += trace/generated-events.c
>
> GENERATED_HEADERS += trace/generated-tracers.h
> -ifeq ($(TRACE_BACKEND),dtrace)
> +ifeq ($(findstring dtrace,$(TRACE_BACKEND)),dtrace)
> GENERATED_HEADERS += trace/generated-tracers-dtrace.h
> endif
> GENERATED_SOURCES += trace/generated-tracers.c
> diff --git a/configure b/configure
> index 3782a6a..ce3d7d6 100755
> --- a/configure
> +++ b/configure
> @@ -3375,15 +3375,18 @@ fi
>
> ##########################################
> # For 'dtrace' backend, test if 'dtrace' command is present
> -if test "$trace_backend" = "dtrace"; then
> - if ! has 'dtrace' ; then
> - error_exit "dtrace command is not found in PATH $PATH"
> - fi
> - trace_backend_stap="no"
> - if has 'stap' ; then
> - trace_backend_stap="yes"
> +IFS=','
> +for backend in ${trace_backend}; do
> + if test "$backend" = "dtrace"; then
> + if ! has 'dtrace' ; then
> + error_exit "dtrace command is not found in PATH $PATH"
> + fi
> + trace_backend_stap="no"
> + if has 'stap' ; then
> + trace_backend_stap="yes"
> + fi
> fi
> -fi
> +done
Please unset IFS, either at the end of the loop (if you're sure the body
of the loop doesn't depend on the default IFS whitespace splitting) or
both in the body and at the end of the loop:
IFS=','
for backend in ${trace_backend}; do
unset IFS
...
IFS=','
done
unset IFS
Failure to unset IFS means the rest of the script will split on commas
instead of whitespace.
I think the following is an easier solution:
if grep dtrace "$trace_backend" >/dev/null; then
...
fi
>
> ##########################################
> # check and set a backend for coroutine
> @@ -4262,33 +4265,34 @@ echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
> if test "$trace_backend" = "nop"; then
> echo "CONFIG_TRACE_NOP=y" >> $config_host_mak
> fi
> -if test "$trace_backend" = "simple"; then
> - echo "CONFIG_TRACE_SIMPLE=y" >> $config_host_mak
> - trace_default=no
> - # Set the appropriate trace file.
> - trace_file="\"$trace_file-\" FMT_pid"
> -fi
> -if test "$trace_backend" = "stderr"; then
> - echo "CONFIG_TRACE_STDERR=y" >> $config_host_mak
> - trace_default=no
> -fi
> -if test "$trace_backend" = "ust"; then
> - echo "CONFIG_TRACE_UST=y" >> $config_host_mak
> -fi
> -if test "$trace_backend" = "dtrace"; then
> - echo "CONFIG_TRACE_DTRACE=y" >> $config_host_mak
> - if test "$trace_backend_stap" = "yes" ; then
> - echo "CONFIG_TRACE_SYSTEMTAP=y" >> $config_host_mak
> +
> +for backend in ${trace_backend}; do
> + if test "$backend" = "simple"; then
> + echo "CONFIG_TRACE_SIMPLE=y" >> $config_host_mak
> + trace_default=no
> + # Set the appropriate trace file.
> + trace_file="\"$trace_file-\" FMT_pid"
> fi
> -fi
> -if test "$trace_backend" = "ftrace"; then
> - if test "$linux" = "yes" ; then
> - echo "CONFIG_TRACE_FTRACE=y" >> $config_host_mak
> + if test "$backend" = "stderr"; then
> + echo "CONFIG_TRACE_STDERR=y" >> $config_host_mak
> trace_default=no
> - else
> - feature_not_found "ftrace(trace backend)"
> fi
> -fi
> + if test "$backend" = "dtrace"; then
> + echo "CONFIG_TRACE_DTRACE=y" >> $config_host_mak
> + if test "$trace_backend_stap" = "yes" ; then
> + echo "CONFIG_TRACE_SYSTEMTAP=y" >> $config_host_mak
> + fi
> + fi
> + if test "$backend" = "ftrace"; then
> + if test "$linux" = "yes" ; then
> + echo "CONFIG_TRACE_FTRACE=y" >> $config_host_mak
> + trace_default=no
> + else
> + feature_not_found "ftrace(trace backend)"
> + fi
> + fi
> +done
> +
> echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
> if test "$trace_default" = "yes"; then
> echo "CONFIG_TRACE_DEFAULT=y" >> $config_host_mak
> diff --git a/scripts/tracetool.py b/scripts/tracetool.py
> index 5f4890f..2c7c0c0 100755
> --- a/scripts/tracetool.py
> +++ b/scripts/tracetool.py
> @@ -112,10 +112,11 @@ def main(args):
> error_opt("backend not set")
>
> if check_backend:
> - if tracetool.backend.exists(arg_backend):
> - sys.exit(0)
> - else:
> - sys.exit(1)
> + for backend in arg_backend.split(","):
> + if tracetool.backend.exists(backend):
> + sys.exit(0)
> + else:
> + sys.exit(1)
>
> if arg_format == "stap":
> if binary is None:
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index 175df08..a0addb5 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -242,14 +242,19 @@ def generate(fevents, format, backend,
> if not tracetool.format.exists(mformat):
> raise TracetoolError("unknown format: %s" % format)
>
> - backend = str(backend)
> + backends = str(backend).split(",")
> if len(backend) is 0:
Before you modified the code it converted 'backend' to a string. Now it
tests len(backend) without converting it to a string.
I suggest s/backend/backends/ in this line to avoid that semantic
change.
> raise TracetoolError("backend not set")
> - mbackend = backend.replace("-", "_")
> - if not tracetool.backend.exists(mbackend):
> - raise TracetoolError("unknown backend: %s" % backend)
>
> - if not tracetool.backend.compatible(mbackend, mformat):
> + compat = False
> + for backend in backends:
> + mbackend = backend.replace("-", "_")
> + if not tracetool.backend.exists(mbackend):
> + raise TracetoolError("unknown backend: %s" % backend)
> +
> + compat |= tracetool.backend.compatible(mbackend, mformat)
> +
> + if not compat:
> raise TracetoolError("backend '%s' not compatible with format '%s'" %
> (backend, format))
This suggests we will generate output in formats that only some backends
suggest. It might be help to add a comment like:
# At least one backend must support the format
Just a hint to the reader that this behavior is intentional.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-01-28 4:35 [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend Kazuya Saito
2014-01-30 21:00 ` Stefan Hajnoczi
@ 2014-01-31 10:37 ` Stefan Hajnoczi
2014-02-04 5:26 ` Kazuya Saito
1 sibling, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-01-31 10:37 UTC (permalink / raw)
To: Kazuya Saito; +Cc: qemu-devel@nongnu.org
On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
Okay, more feedback after looking at the rest of the patch.
I think "ust" backend support will simplify the patch a little (see
details below).
Looking forward to the next revision. Let me know if you have any
questions about my feedback.
> diff --git a/scripts/tracetool/backend/__init__.py b/scripts/tracetool/backend/__init__.py
> index f0314ee..40efcb5 100644
> --- a/scripts/tracetool/backend/__init__.py
> +++ b/scripts/tracetool/backend/__init__.py
> @@ -110,20 +110,28 @@ def compatible(backend, format):
> def _empty(events):
> pass
>
> -def generate(backend, format, events):
> +def generate(backends, format, events):
> """Generate the per-event output for the given (backend, format) pair."""
Please update the doc comment to reflect the new argument.
> - if not compatible(backend, format):
> + compat = False
> + for backend in backends:
> + compat |= compatible(backend, format)
> + if not compat:
> raise ValueError("backend '%s' not compatible with format '%s'" %
> (backend, format))
>
> - backend = backend.replace("-", "_")
> + backends = map(lambda x: x.replace("-", "_"), backends)
> format = format.replace("-", "_")
>
> - if backend == "nop":
> + if backends == ["nop"]:
> func = tracetool.try_import("tracetool.format." + format,
> "nop", _empty)[1]
> + func(events)
> + elif set(backends).issubset(["dtrace", "ftrace", "simple", "stderr"]):
> + func = tracetool.try_import("tracetool.backend.common",
> + format, None)[1]
> + func(backends, events)
> else:
> func = tracetool.try_import("tracetool.backend." + backend,
> format, None)[1]
This extra case exists because "ust" isn't converted yet?
backend/__init__.py should not know about all backends (it's a pain to
hardcode the backend names and keep them up-to-date). I hope this
change can be dropped in the next revision of the patch since the "ust"
backend will no longer be different.
> +def c(backends, events):
> + func_head_lst = []
> + func_body_lst = []
> + for backend in backends:
> + func_head_lst.append(tracetool.try_import("tracetool.backend." + backend,
> + "c_head", None)[1])
> + func_body_lst.append(tracetool.try_import("tracetool.backend." + backend,
> + "c_body", None)[1])
> +
> + out('#include "trace/control.h"',
> + '#ifndef CONFIG_TRACE_DTRACE',
> + '#include "trace.h"',
> + '#endif'
> + '',
> + )
> + for func_head in func_head_lst:
> + func_head()
Can the CONFIG_TRACE_DTRACE include be emitted by dtrace.c_head()? Then
we don't need to hardcode it into this generic file (it shouldn't know
about specific backends).
Perhaps you could even make a c_include() interface, if necessary.
> def h(events):
> + pass
I thought all code generation now happens in backends.common.h(), so
this function will not be called anymore?
The same is true for c() defined in this file.
> def d(events):
> + d_head()
> out('provider qemu {')
> -
> for e in events:
> - args = str(e.args)
> + d_body(e)
> + out('',
> + '};')
> +
> +def d_head():
> + pass
This function seems unnecessary. Can you drop it?
> @@ -86,24 +99,30 @@ RESERVED_WORDS = (
> )
>
> def stap(events):
> + stap_head()
> for e in events:
> - # Define prototype for probe arguments
> - out('probe %(probeprefix)s.%(name)s = process("%(binary)s").mark("%(name)s")',
> - '{',
> - probeprefix = _probeprefix(),
> - name = e.name,
> - binary = _binary(),
> - )
> -
> - i = 1
> - if len(e.args) > 0:
> - for name in e.args.names():
> - # Append underscore to reserved keywords
> - if name in RESERVED_WORDS:
> - name += '_'
> - out(' %s = $arg%d;' % (name, i))
> - i += 1
> -
> - out('}')
> -
> + stap_body(e)
> out()
> +
> +def stap_head():
> + pass
Same here, I think it can be dropped.
> ######################################################################
> # Backend code
>
> util-obj-$(CONFIG_TRACE_DEFAULT) += default.o
> -util-obj-$(CONFIG_TRACE_SIMPLE) += simple.o
> -util-obj-$(CONFIG_TRACE_STDERR) += stderr.o
> -util-obj-$(CONFIG_TRACE_FTRACE) += ftrace.o
> +util-obj-$(CONFIG_TRACE_SIMPLE) += multi.o simple.o
> +util-obj-$(CONFIG_TRACE_STDERR) += multi.o
What happened to stderr.o?
> +util-obj-$(CONFIG_TRACE_FTRACE) += multi.o ftrace.o
How about adding multi.o to util-obj-y just like control.o below? All
these object files are added to libqemuutil.a. The linker will only
pull in object files that are needed (based on symbol dependencies) so
there is no harm in uncoditionally building multi.o.
(In fact, I think it's better to always build it to avoid bitrot.)
> diff --git a/trace/multi.c b/trace/multi.c
> new file mode 100644
> index 0000000..ab2b79f
> --- /dev/null
> +++ b/trace/multi.c
> @@ -0,0 +1,52 @@
> +/*
> + * Multi trace backend
> + */
> +
> +#include <stdio.h>
> +#include "trace.h"
> +#include "trace/control.h"
> +
> +void trace_print_events(FILE *stream, fprintf_function stream_printf)
> +{
> + TraceEventID i;
> +
> + for (i = 0; i < trace_event_count(); i++) {
> + TraceEvent *ev = trace_event_id(i);
> + stream_printf(stream, "%s [Event ID %u] : state %u\n",
> + trace_event_get_name(ev), i, trace_event_get_state_dynamic(ev));
> + }
> +}
> +
> +void trace_event_set_state_dynamic_backend(TraceEvent *ev, bool state)
> +{
> + ev->dstate = state;
> +}
> +
> +bool trace_backend_init(const char *events, const char *file)
> +{
> + bool retval = true;
> +
> +#ifndef CONFIG_TRACE_SIMPLE
> + if (file) {
> + fprintf(stderr, "error: -trace file=...: "
> + "option not supported by the selected tracing backend\n");
> + return false;
> + }
> +#endif
Not sure if this is right. We may need to use -trace file=... for
simple or ftrace. stderr should just ignore it.
> +
> +#ifdef CONFIG_TRACE_SIMPLE
> + retval &= simpletrace_backend_init(events, file);
> +#endif
> +
> +#ifdef CONFIG_TRACE_FTRACE
> + retval &= ftrace_backend_init(events, file);
> +#endif
> +
> + if (!retval){
> + fprintf(stderr, "failed to initialize tracing backend.\n");
> + return false;
> + }
Instead of retval &= it would be helpful to check the return value after
each *_init() call and print a more detailed error message:
#ifdef CONFIG_TRACE_SIMPLE
if (!simpletrace_backend_init(events, file)) {
fprintf(stderr, "failed to initialize simple tracing backend.\n");
return false;
}
#endif
That way the user has a better chance of figuring out what is wrong
(although the error message still isn't very detailed :)).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-01-30 21:00 ` Stefan Hajnoczi
@ 2014-02-04 5:24 ` Kazuya Saito
2014-02-04 8:34 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Kazuya Saito @ 2014-02-04 5:24 UTC (permalink / raw)
To: stefanha; +Cc: qemu-devel@nongnu.org
(2014/01/31 6:00), Stefan Hajnoczi wrote:> On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
>
> Some initial comments, I will continue reviewing tomorrow.
Thank you for your comment.
>> This patch implements "multi tracing backend" which enables several
>> tracing backend simultaneously.
>>
>> QEMU has multiple trace backends, but one of them needs to be chosen at
>> compile time. When investigating issues of QEMU, it'd be much more
>> convenient if we can use multiple trace backends without recompiling,
>> especially 'ftrace backend' and 'dtrace backend'. From the performance
>> perspective, 'ftrace backend' should be the one which runs while the
>> system is in operation, and it provides useful information. But, for
>> some issues, 'dtrace backend' is needed for investigation as 'dtrace
>> backend' provides more information. This patch enables both 'ftrace
>> backend' and 'dtrace backend' (, and some other backends if necessary)
>> at compile time so that we can switch between the two without
>> recompiling.
>>
>> usage:
>> We have only to set some tracing backends as follows.
>>
>> $ ./configure --enable-trace-backend=ftrace,dtrace
>>
>> Of course, we can compile with single backend as well as before.
>>
>> $ ./configure --enable-trace-backend=simple
>
> Great, this functionality has been suggested before so I'm sure it will
> come in handy.
>
>> Note: Now, we can select nop, ftrace, dtrace, stderr, ust, simple as
>> tracing backend. However, we can select ftrace, dtrace, stderr,
>> simple when selecting more than two backends. Though it's
>> needless to say about nop, I excluded ust backend because I didn't
>> test it since it doesn't support LTTng 2.x.
>
> I have just reviewed and merged the LTTng 2.x patch series.
>
> You can base your patch on:
> git://github.com/stefanha/qemu.git tracing-next
Thank you for the information. I'll base my patch on tracing-next
branch and post it here.
>> Signed-off-by: Kazuya Saito <saito.kazuya@jp.fujitsu.com>
>> ---
>> Makefile | 2 +-
>> configure | 68 ++++++++++---------
>> scripts/tracetool.py | 9 ++-
>> scripts/tracetool/__init__.py | 21 ++++--
>> scripts/tracetool/backend/__init__.py | 20 ++++--
>> scripts/tracetool/backend/common.py | 78 +++++++++++++++++++++
>> scripts/tracetool/backend/dtrace.py | 107 +++++++++++++++++------------
>> scripts/tracetool/backend/ftrace.py | 60 +++++++++-------
>> scripts/tracetool/backend/simple.py | 124 +++++++++++++++++----------------
>> scripts/tracetool/backend/stderr.py | 44 +++++++-----
>> trace/Makefile.objs | 22 ++++---
>> trace/ftrace.c | 25 +------
>> trace/ftrace.h | 1 +
>> trace/multi.c | 52 ++++++++++++++
>> trace/simple.c | 18 +-----
>> trace/simple.h | 2 +-
>> trace/stderr.c | 30 --------
>> 17 files changed, 403 insertions(+), 280 deletions(-)
>> create mode 100644 scripts/tracetool/backend/common.py
>> create mode 100644 trace/multi.c
>> delete mode 100644 trace/stderr.c
>>
>> diff --git a/Makefile b/Makefile
>> index bdff4e4..7bcacf5 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -52,7 +52,7 @@ GENERATED_HEADERS += trace/generated-events.h
>> GENERATED_SOURCES += trace/generated-events.c
>>
>> GENERATED_HEADERS += trace/generated-tracers.h
>> -ifeq ($(TRACE_BACKEND),dtrace)
>> +ifeq ($(findstring dtrace,$(TRACE_BACKEND)),dtrace)
>> GENERATED_HEADERS += trace/generated-tracers-dtrace.h
>> endif
>> GENERATED_SOURCES += trace/generated-tracers.c
>> diff --git a/configure b/configure
>> index 3782a6a..ce3d7d6 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3375,15 +3375,18 @@ fi
>>
>> ##########################################
>> # For 'dtrace' backend, test if 'dtrace' command is present
>> -if test "$trace_backend" = "dtrace"; then
>> - if ! has 'dtrace' ; then
>> - error_exit "dtrace command is not found in PATH $PATH"
>> - fi
>> - trace_backend_stap="no"
>> - if has 'stap' ; then
>> - trace_backend_stap="yes"
>> +IFS=','
>> +for backend in ${trace_backend}; do
>> + if test "$backend" = "dtrace"; then
>> + if ! has 'dtrace' ; then
>> + error_exit "dtrace command is not found in PATH $PATH"
>> + fi
>> + trace_backend_stap="no"
>> + if has 'stap' ; then
>> + trace_backend_stap="yes"
>> + fi
>> fi
>> -fi
>> +done
>
> Please unset IFS, either at the end of the loop (if you're sure the body
> of the loop doesn't depend on the default IFS whitespace splitting) or
> both in the body and at the end of the loop:
>
> IFS=','
> for backend in ${trace_backend}; do
> unset IFS
> ...
> IFS=','
> done
> unset IFS
>
> Failure to unset IFS means the rest of the script will split on commas
> instead of whitespace.
>
> I think the following is an easier solution:
> if grep dtrace "$trace_backend" >/dev/null; then
> ...
> fi
I'll fix it as you said. And the following loops (tracing
backend-specific routines) will be fixed in a similar way.
>> ##########################################
>> # check and set a backend for coroutine
>> @@ -4262,33 +4265,34 @@ echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
>> if test "$trace_backend" = "nop"; then
>> echo "CONFIG_TRACE_NOP=y" >> $config_host_mak
>> fi
>> -if test "$trace_backend" = "simple"; then
>> - echo "CONFIG_TRACE_SIMPLE=y" >> $config_host_mak
>> - trace_default=no
>> - # Set the appropriate trace file.
>> - trace_file="\"$trace_file-\" FMT_pid"
>> -fi
>> -if test "$trace_backend" = "stderr"; then
>> - echo "CONFIG_TRACE_STDERR=y" >> $config_host_mak
>> - trace_default=no
>> -fi
>> -if test "$trace_backend" = "ust"; then
>> - echo "CONFIG_TRACE_UST=y" >> $config_host_mak
>> -fi
>> -if test "$trace_backend" = "dtrace"; then
>> - echo "CONFIG_TRACE_DTRACE=y" >> $config_host_mak
>> - if test "$trace_backend_stap" = "yes" ; then
>> - echo "CONFIG_TRACE_SYSTEMTAP=y" >> $config_host_mak
>> +
>> +for backend in ${trace_backend}; do
>> + if test "$backend" = "simple"; then
>> + echo "CONFIG_TRACE_SIMPLE=y" >> $config_host_mak
>> + trace_default=no
>> + # Set the appropriate trace file.
>> + trace_file="\"$trace_file-\" FMT_pid"
>> fi
>> -fi
>> -if test "$trace_backend" = "ftrace"; then
>> - if test "$linux" = "yes" ; then
>> - echo "CONFIG_TRACE_FTRACE=y" >> $config_host_mak
>> + if test "$backend" = "stderr"; then
>> + echo "CONFIG_TRACE_STDERR=y" >> $config_host_mak
>> trace_default=no
>> - else
>> - feature_not_found "ftrace(trace backend)"
>> fi
>> -fi
>> + if test "$backend" = "dtrace"; then
>> + echo "CONFIG_TRACE_DTRACE=y" >> $config_host_mak
>> + if test "$trace_backend_stap" = "yes" ; then
>> + echo "CONFIG_TRACE_SYSTEMTAP=y" >> $config_host_mak
>> + fi
>> + fi
>> + if test "$backend" = "ftrace"; then
>> + if test "$linux" = "yes" ; then
>> + echo "CONFIG_TRACE_FTRACE=y" >> $config_host_mak
>> + trace_default=no
>> + else
>> + feature_not_found "ftrace(trace backend)"
>> + fi
>> + fi
>> +done
>> +
>> echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
>> if test "$trace_default" = "yes"; then
>> echo "CONFIG_TRACE_DEFAULT=y" >> $config_host_mak
>> diff --git a/scripts/tracetool.py b/scripts/tracetool.py
>> index 5f4890f..2c7c0c0 100755
>> --- a/scripts/tracetool.py
>> +++ b/scripts/tracetool.py
>> @@ -112,10 +112,11 @@ def main(args):
>> error_opt("backend not set")
>>
>> if check_backend:
>> - if tracetool.backend.exists(arg_backend):
>> - sys.exit(0)
>> - else:
>> - sys.exit(1)
>> + for backend in arg_backend.split(","):
>> + if tracetool.backend.exists(backend):
>> + sys.exit(0)
>> + else:
>> + sys.exit(1)
>>
>> if arg_format == "stap":
>> if binary is None:
>> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
>> index 175df08..a0addb5 100644
>> --- a/scripts/tracetool/__init__.py
>> +++ b/scripts/tracetool/__init__.py
>> @@ -242,14 +242,19 @@ def generate(fevents, format, backend,
>> if not tracetool.format.exists(mformat):
>> raise TracetoolError("unknown format: %s" % format)
>>
>> - backend = str(backend)
>> + backends = str(backend).split(",")
>> if len(backend) is 0:
>
> Before you modified the code it converted 'backend' to a string. Now it
> tests len(backend) without converting it to a string.
>
> I suggest s/backend/backends/ in this line to avoid that semantic
> change.
"backends" is a list not a string. So, I'll modify it to the following.
backends = str(backend).split(",")
for backend in backends:
if len(backend) is 0:
>> raise TracetoolError("backend not set")
>> - mbackend = backend.replace("-", "_")
>> - if not tracetool.backend.exists(mbackend):
>> - raise TracetoolError("unknown backend: %s" % backend)
>>
>> - if not tracetool.backend.compatible(mbackend, mformat):
>> + compat = False
>> + for backend in backends:
>> + mbackend = backend.replace("-", "_")
>> + if not tracetool.backend.exists(mbackend):
>> + raise TracetoolError("unknown backend: %s" % backend)
>> +
>> + compat |= tracetool.backend.compatible(mbackend, mformat)
>> +
>> + if not compat:
>> raise TracetoolError("backend '%s' not compatible with format '%s'" %
>> (backend, format))
>
> This suggests we will generate output in formats that only some backends
> suggest. It might be help to add a comment like:
> # At least one backend must support the format
>
> Just a hint to the reader that this behavior is intentional.
OK, I'll insert the comment so that the reader can understand easier.
Thanks,
Kazuya Saito
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-01-31 10:37 ` Stefan Hajnoczi
@ 2014-02-04 5:26 ` Kazuya Saito
2014-02-04 9:02 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Kazuya Saito @ 2014-02-04 5:26 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel@nongnu.org
(2014/01/31 19:37), Stefan Hajnoczi wrote:> On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
>
> Okay, more feedback after looking at the rest of the patch.
Thank you for the feedback.
> I think "ust" backend support will simplify the patch a little (see
> details below).
I'll try to support "ust" backend in the next revision.
> Looking forward to the next revision. Let me know if you have any
> questions about my feedback.
>
>> diff --git a/scripts/tracetool/backend/__init__.py b/scripts/tracetool/backend/__init__.py
>> index f0314ee..40efcb5 100644
>> --- a/scripts/tracetool/backend/__init__.py
>> +++ b/scripts/tracetool/backend/__init__.py
>> @@ -110,20 +110,28 @@ def compatible(backend, format):
>> def _empty(events):
>> pass
>>
>> -def generate(backend, format, events):
>> +def generate(backends, format, events):
>> """Generate the per-event output for the given (backend, format) pair."""
>
> Please update the doc comment to reflect the new argument.
I see.
>> - if not compatible(backend, format):
>> + compat = False
>> + for backend in backends:
>> + compat |= compatible(backend, format)
>> + if not compat:
>> raise ValueError("backend '%s' not compatible with format '%s'" %
>> (backend, format))
>>
>> - backend = backend.replace("-", "_")
>> + backends = map(lambda x: x.replace("-", "_"), backends)
>> format = format.replace("-", "_")
>>
>> - if backend == "nop":
>> + if backends == ["nop"]:
>> func = tracetool.try_import("tracetool.format." + format,
>> "nop", _empty)[1]
>> + func(events)
>> + elif set(backends).issubset(["dtrace", "ftrace", "simple", "stderr"]):
>> + func = tracetool.try_import("tracetool.backend.common",
>> + format, None)[1]
>> + func(backends, events)
>> else:
>> func = tracetool.try_import("tracetool.backend." + backend,
>> format, None)[1]
>
> This extra case exists because "ust" isn't converted yet?
Yes, you're right.
> backend/__init__.py should not know about all backends (it's a pain to
> hardcode the backend names and keep them up-to-date).
I agree with you.
> I hope this
> change can be dropped in the next revision of the patch since the "ust"
> backend will no longer be different.
OK. I'll try it.
However, the above should be changed because of "events" backend even if
it supports "ust" backend. I think there is two solutions for this.
The first is to define common.events_h() and common.events_c().
The other is to branch it for "events" backend as below.
if backends == ["nop"]:
func = tracetool.try_import("tracetool.format." + format,
"nop", _empty)[1]
func(events)
elif backends == ["events"]:
func = tracetool.try_import("tracetool.backend.events",
format, None)[1]
func(events)
else:
func = tracetool.try_import("tracetool.backend.common",
format, None)[1]
func(backends, events)
I think the first is better because backend/__init__.py shouldn't care
"events" backend. Do you have any suggestions?
>> +def c(backends, events):
>> + func_head_lst = []
>> + func_body_lst = []
>> + for backend in backends:
>> + func_head_lst.append(tracetool.try_import("tracetool.backend." + backEnd,
>> + "c_head", None)[1])
>> + func_body_lst.append(tracetool.try_import("tracetool.backend." + backend,
>> + "c_body", None)[1])
>> +
>> + out('#include "trace/control.h"',
>> + '#ifndef CONFIG_TRACE_DTRACE',
>> + '#include "trace.h"',
>> + '#endif'
>> + '',
>> + )
>> + for func_head in func_head_lst:
>> + func_head()
>
> Can the CONFIG_TRACE_DTRACE include be emitted by dtrace.c_head()? Then
> we don't need to hardcode it into this generic file (it shouldn't know
> about specific backends).
You're right. I'll fix it.
> Perhaps you could even make a c_include() interface, if necessary.
>
>> def h(events):
>> + pass
>
> I thought all code generation now happens in backends.common.h(), so
> this function will not be called anymore?
It is called in tracetool.backend.compatible(). So, it is required when
selecting only dtrace backend.
> The same is true for c() defined in this file.
It is also required for the same reason as dtrace.h().
>> def d(events):
>> + d_head()
>> out('provider qemu {')
>> -
>> for e in events:
>> - args = str(e.args)
>> + d_body(e)
>> + out('',
>> + '};')
>> +
>> +def d_head():
>> + pass
>
> This function seems unnecessary. Can you drop it?
OK. Although I thought that it would be useful for something by
splitting this function in the same way as other functions, it is only
used in dtrace backend. So, I'm going to revert it back.
>> @@ -86,24 +99,30 @@ RESERVED_WORDS = (
>> )
>>
>> def stap(events):
>> + stap_head()
>> for e in events:
>> - # Define prototype for probe arguments
>> - out('probe %(probeprefix)s.%(name)s = process("%(binary)s").mark("%(name)s")',
>> - '{',
>> - probeprefix = _probeprefix(),
>> - name = e.name,
>> - binary = _binary(),
>> - )
>> -
>> - i = 1
>> - if len(e.args) > 0:
>> - for name in e.args.names():
>> - # Append underscore to reserved keywords
>> - if name in RESERVED_WORDS:
>> - name += '_'
>> - out(' %s = $arg%d;' % (name, i))
>> - i += 1
>> -
>> - out('}')
>> -
>> + stap_body(e)
>> out()
>> +
>> +def stap_head():
>> + pass
>
> Same here, I think it can be dropped.
OK, I'm going to revert it back as well as dtrace.d().
>> ######################################################################
>> # Backend code
>>
>> util-obj-$(CONFIG_TRACE_DEFAULT) += default.o
>> -util-obj-$(CONFIG_TRACE_SIMPLE) += simple.o
>> -util-obj-$(CONFIG_TRACE_STDERR) += stderr.o
>> -util-obj-$(CONFIG_TRACE_FTRACE) += ftrace.o
>> +util-obj-$(CONFIG_TRACE_SIMPLE) += multi.o simple.o
>> +util-obj-$(CONFIG_TRACE_STDERR) += multi.o
>
> What happened to stderr.o?
The all functions defined in stderr.o was moved into multi.o because
there was no function which was only executed when selecting stderr
backend. So, stderr.o was eliminated.
>> +util-obj-$(CONFIG_TRACE_FTRACE) += multi.o ftrace.o
>
> How about adding multi.o to util-obj-y just like control.o below? All
> these object files are added to libqemuutil.a. The linker will only
> pull in object files that are needed (based on symbol dependencies) so
> there is no harm in uncoditionally building multi.o.
If adding multi.o to util-obj-y, compile error occurs when selecting
only dtrace backend. This is because the function trace_print_events(),
trace_event_set_state_dynamic_backend() and trace_backend_init() are
declared doubly in multi.o and default.o.
So, I'm going to leave it. Do you have any suggestions?
> (In fact, I think it's better to always build it to avoid bitrot.)
>
>> diff --git a/trace/multi.c b/trace/multi.c
>> new file mode 100644
>> index 0000000..ab2b79f
>> --- /dev/null
>> +++ b/trace/multi.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * Multi trace backend
>> + */
>> +
>> +#include <stdio.h>
>> +#include "trace.h"
>> +#include "trace/control.h"
>> +
>> +void trace_print_events(FILE *stream, fprintf_function stream_printf)
>> +{
>> + TraceEventID i;
>> +
>> + for (i = 0; i < trace_event_count(); i++) {
>> + TraceEvent *ev = trace_event_id(i);
>> + stream_printf(stream, "%s [Event ID %u] : state %u\n",
>> + trace_event_get_name(ev), i, trace_event_get_state_dynamic(ev));
>> + }
>> +}
>> +
>> +void trace_event_set_state_dynamic_backend(TraceEvent *ev, bool state)
>> +{
>> + ev->dstate = state;
>> +}
>> +
>> +bool trace_backend_init(const char *events, const char *file)
>> +{
>> + bool retval = true;
>> +
>> +#ifndef CONFIG_TRACE_SIMPLE
>> + if (file) {
>> + fprintf(stderr, "error: -trace file=...: "
>> + "option not supported by the selected tracing backend\n");
>> + return false;
>> + }
>> +#endif
>
> Not sure if this is right. We may need to use -trace file=... for
> simple or ftrace. stderr should just ignore it.
The error message is output if the argument "file" is set when using
"stderr", "ftrace" or "dtrace" backend. In other words, only "simple"
backend uses -trace file=...
This is the reason why I implemented it as seen above.
>> +
>> +#ifdef CONFIG_TRACE_SIMPLE
>> + retval &= simpletrace_backend_init(events, file);
>> +#endif
>> +
>> +#ifdef CONFIG_TRACE_FTRACE
>> + retval &= ftrace_backend_init(events, file);
>> +#endif
>> +
>> + if (!retval){
>> + fprintf(stderr, "failed to initialize tracing backend.\n");
>> + return false;
>> + }
>
> Instead of retval &= it would be helpful to check the return value after
> each *_init() call and print a more detailed error message:
>
> #ifdef CONFIG_TRACE_SIMPLE
> if (!simpletrace_backend_init(events, file)) {
> fprintf(stderr, "failed to initialize simple tracing backend.\n");
> return false;
> }
> #endif
>
> That way the user has a better chance of figuring out what is wrong
> (although the error message still isn't very detailed :)).
I see. That is better for the user.
I'll fix it as you said.
Thank you very much for your valuable advice. I'll post the next
revision as soon as possible.
Thanks,
Kazuya Saito
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-02-04 5:24 ` Kazuya Saito
@ 2014-02-04 8:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-02-04 8:34 UTC (permalink / raw)
To: Kazuya Saito; +Cc: qemu-devel@nongnu.org
On Tue, Feb 04, 2014 at 02:24:16PM +0900, Kazuya Saito wrote:
> (2014/01/31 6:00), Stefan Hajnoczi wrote:> On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
> >> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> >> index 175df08..a0addb5 100644
> >> --- a/scripts/tracetool/__init__.py
> >> +++ b/scripts/tracetool/__init__.py
> >> @@ -242,14 +242,19 @@ def generate(fevents, format, backend,
> >> if not tracetool.format.exists(mformat):
> >> raise TracetoolError("unknown format: %s" % format)
> >>
> >> - backend = str(backend)
> >> + backends = str(backend).split(",")
> >> if len(backend) is 0:
> >
> > Before you modified the code it converted 'backend' to a string. Now it
> > tests len(backend) without converting it to a string.
> >
> > I suggest s/backend/backends/ in this line to avoid that semantic
> > change.
>
> "backends" is a list not a string. So, I'll modify it to the following.
>
> backends = str(backend).split(",")
> for backend in backends:
> if len(backend) is 0:
You are right:
>>> ''.split(',')
['']
I thought it returns [].
Stefan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-02-04 5:26 ` Kazuya Saito
@ 2014-02-04 9:02 ` Stefan Hajnoczi
2014-02-05 8:51 ` Kazuya Saito
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-02-04 9:02 UTC (permalink / raw)
To: Kazuya Saito; +Cc: qemu-devel@nongnu.org
On Tue, Feb 04, 2014 at 02:26:13PM +0900, Kazuya Saito wrote:
> (2014/01/31 19:37), Stefan Hajnoczi wrote:> On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
> > I hope this
> > change can be dropped in the next revision of the patch since the "ust"
> > backend will no longer be different.
>
> OK. I'll try it.
> However, the above should be changed because of "events" backend even if
> it supports "ust" backend. I think there is two solutions for this.
> The first is to define common.events_h() and common.events_c().
> The other is to branch it for "events" backend as below.
>
> if backends == ["nop"]:
> func = tracetool.try_import("tracetool.format." + format,
> "nop", _empty)[1]
> func(events)
> elif backends == ["events"]:
> func = tracetool.try_import("tracetool.backend.events",
> format, None)[1]
> func(events)
> else:
> func = tracetool.try_import("tracetool.backend.common",
> format, None)[1]
> func(backends, events)
>
> I think the first is better because backend/__init__.py shouldn't care
> "events" backend. Do you have any suggestions?
Yes, I prefer less special cases too.
> >> def h(events):
> >> + pass
> >
> > I thought all code generation now happens in backends.common.h(), so
> > this function will not be called anymore?
>
> It is called in tracetool.backend.compatible(). So, it is required when
> selecting only dtrace backend.
>
> > The same is true for c() defined in this file.
>
> It is also required for the same reason as dtrace.h().
tracetool.backend.compatible() is testing for the existence of a
function that is not used anywhere else. Backend code doesn't make it
obvious why this is necessary.
We should either make compatible() work (e.g. by testing body_<format>
and ensuring all formats use the "body_" function prefix) or with
something explicit like a <backend>.supported_formats = ['c', 'h'] list.
> >> +util-obj-$(CONFIG_TRACE_FTRACE) += multi.o ftrace.o
> >
> > How about adding multi.o to util-obj-y just like control.o below? All
> > these object files are added to libqemuutil.a. The linker will only
> > pull in object files that are needed (based on symbol dependencies) so
> > there is no harm in uncoditionally building multi.o.
>
> If adding multi.o to util-obj-y, compile error occurs when selecting
> only dtrace backend. This is because the function trace_print_events(),
> trace_event_set_state_dynamic_backend() and trace_backend_init() are
> declared doubly in multi.o and default.o.
> So, I'm going to leave it. Do you have any suggestions?
I guess it should be:
ifeq ($(CONFIG_TRACE_DEFAULT),y)
util-obj-y += default.o
else
util-obj-y += multi.o
endif
> >> +bool trace_backend_init(const char *events, const char *file)
> >> +{
> >> + bool retval = true;
> >> +
> >> +#ifndef CONFIG_TRACE_SIMPLE
> >> + if (file) {
> >> + fprintf(stderr, "error: -trace file=...: "
> >> + "option not supported by the selected tracing backend\n");
> >> + return false;
> >> + }
> >> +#endif
> >
> > Not sure if this is right. We may need to use -trace file=... for
> > simple or ftrace. stderr should just ignore it.
>
> The error message is output if the argument "file" is set when using
> "stderr", "ftrace" or "dtrace" backend. In other words, only "simple"
> backend uses -trace file=...
> This is the reason why I implemented it as seen above.
oops, I misread the code. I thought #ifdef instead of #ifndef.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend
2014-02-04 9:02 ` Stefan Hajnoczi
@ 2014-02-05 8:51 ` Kazuya Saito
0 siblings, 0 replies; 8+ messages in thread
From: Kazuya Saito @ 2014-02-05 8:51 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel@nongnu.org
(2014/02/04 18:02), Stefan Hajnoczi wrote:> On Tue, Feb 04, 2014 at 02:26:13PM +0900, Kazuya Saito wrote:
>> > (2014/01/31 19:37), Stefan Hajnoczi wrote:> On Tue, Jan 28, 2014 at 01:35:20PM +0900, Kazuya Saito wrote:
>>>> > >> def h(events):
>>>> > >> + pass
>>> > >
>>> > > I thought all code generation now happens in backends.common.h(), so
>>> > > this function will not be called anymore?
>> >
>> > It is called in tracetool.backend.compatible(). So, it is required when
>> > selecting only dtrace backend.
>> >
>>> > > The same is true for c() defined in this file.
>> >
>> > It is also required for the same reason as dtrace.h().
> tracetool.backend.compatible() is testing for the existence of a
> function that is not used anywhere else. Backend code doesn't make it
> obvious why this is necessary.
>
> We should either make compatible() work (e.g. by testing body_<format>
> and ensuring all formats use the "body_" function prefix) or with
> something explicit like a <backend>.supported_formats = ['c', 'h'] list.
I think that checking <bakcend>.supported_formats is better. I'll
change compatible() like that.
>>>> > >> +util-obj-$(CONFIG_TRACE_FTRACE) += multi.o ftrace.o
>>> > >
>>> > > How about adding multi.o to util-obj-y just like control.o below? All
>>> > > these object files are added to libqemuutil.a. The linker will only
>>> > > pull in object files that are needed (based on symbol dependencies) so
>>> > > there is no harm in uncoditionally building multi.o.
>> >
>> > If adding multi.o to util-obj-y, compile error occurs when selecting
>> > only dtrace backend. This is because the function trace_print_events(),
>> > trace_event_set_state_dynamic_backend() and trace_backend_init() are
>> > declared doubly in multi.o and default.o.
>> > So, I'm going to leave it. Do you have any suggestions?
> I guess it should be:
>
> ifeq ($(CONFIG_TRACE_DEFAULT),y)
> util-obj-y += default.o
> else
> util-obj-y += multi.o
> endif
Thank you. I'll fix it as above.
Thanks,
Kazuya Saito
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-05 8:52 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-28 4:35 [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend Kazuya Saito
2014-01-30 21:00 ` Stefan Hajnoczi
2014-02-04 5:24 ` Kazuya Saito
2014-02-04 8:34 ` Stefan Hajnoczi
2014-01-31 10:37 ` Stefan Hajnoczi
2014-02-04 5:26 ` Kazuya Saito
2014-02-04 9:02 ` Stefan Hajnoczi
2014-02-05 8:51 ` Kazuya Saito
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).