Re: [Qemu-devel] [PATCH 2/2] trace: check backend dstate in trace_event_get_state()

[Stefan Hajnoczi]

On Fri, Jul 28, 2017 at 12:05:44PM +0100, Daniel P. Berrange wrote:
> On Fri, Jul 28, 2017 at 10:20:53AM +0100, Stefan Hajnoczi wrote:
> > Code that checks dstate is unaware of SystemTap and LTTng UST dstate, so
> > the following trace event will not fire when solely enabled by SystemTap
> > or LTTng UST:
> > 
> >   if (trace_event_get_state(TRACE_MY_EVENT)) {
> >       str = g_strdup_printf("Expensive string to generate ...",
> >                             ...);
> >       trace_my_event(str);
> >       g_free(str);
> >   }
> > 
> > Most users of trace_event_get_state() want to know both QEMU and backend
> > dstate for the event.  Update the macro to include backend dstate.
> > 
> > Introduce trace_event_get_state_qemu() for those callers who really only
> > want QEMU dstate.  This includes the trace backends (like 'log') which
> > should only trigger when event are enabled via QEMU.
> 
> I'm not convinced this is quite right as is.
> 
> The QEMU state for an event is set via cli flags to -trace and that
> is intended for use with with things like simpletrace or log which
> have no other way to filter which events are turned on at runtime.
> For these calling trace_event_get_state_dynamic_by_id() is right.
> 
> For DTrace / LTT-NG, and event is active if the kernel has turned
> on the dynamic probe location.  Any command line args like -trace
> should not influence this at all, so we should *not* involve QEMU
> state at all - only the backend state. So for these we should only
> be calling the backend specific test and ignoring
> trace_event_get_state_dynamic_by_id() entirely. Your patch though
> makes it consider both.

I think you're referring to this:

  #define trace_event_get_state(id)                       \
      ((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
                           id ##_BACKEND_DSTATE()))

We could change it to:

  #define trace_event_get_state(id)                       \
      ((id ##_ENABLED) && id ##_BACKEND_DSTATE())

and modify the log, syslog, ftrace, etc backends to emit
trace_event_get_state_dynamic_by_id(id) as their backend dstate.

However, in the multi-backend case with ./configure
--enable-trace-backends=log,syslog this expands to:

  ((id ##_ENABLED) && (trace_event_get_state_dynamic_by_id(id) || \
                       trace_event_get_state_dynamic_by_id(id) || \
                       false))

I couldn't bring myself to have multiple calls to
trace_event_get_state_dynamic_by_id() :).

What do you think?

signature.asc
Description: PGP signature