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

[Stefan Hajnoczi]

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 :)).