(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.kaz...@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
