Re: [Qemu-devel] [PATCH] trace backend: introduce multi tracing backend

[Kazuya Saito <saito.kazuya@jp.fujitsu.com>]

(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