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

[Kazuya Saito]

(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