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

[Stefan Hajnoczi <stefanha@gmail.com>]

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.