Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for events with the 'vcpu' property

[Paolo Bonzini <pbonzini@redhat.com>]

First of all, a generic problem I see with your patches is that the
newly-introduced APIs are not providing a good abstraction.

If something is only used internally, as is the case for
trace_event_get_cpu_id, you don't need accessors.  On the other hand,
when you have a repeated expression such as

     trace_event_get_cpu_id(ev) != trace_event_cpu_count()

then you need an API such as trace_event_is_vcpu(ev).

Another small ugliness is that you are using "vcpu" in trace-events and
in the generated files, but "cpu" in the C file.  My suggestion is to
prefix functions with vcpu_trace_event if they refer to per-VCPU trace
events, and only use the VCPU ids in those functions.

On 25/02/2016 16:03, Lluís Vilanova wrote:
> +static inline bool trace_event_get_cpu_state_dynamic(CPUState *cpu,
> +                                                     TraceEvent *ev)
>  {
> -    int id = trace_event_get_id(ev);
> +    TraceEventVCPUID id;
> +    assert(cpu != NULL);
>      assert(ev != NULL);

Please do not add more "!= NULL" asserts.  In fact, we should remove the
others; in this case the ev != NULL assertion is particularly pointless
since it comes after a dereference.

>      assert(trace_event_get_state_static(ev));
> -    trace_events_enabled_count += state - trace_events_dstate[id];
> -    trace_events_dstate[id] = state;
> +    assert(trace_event_get_cpu_id(ev) != trace_event_cpu_count());
> +    id = trace_event_get_cpu_id(ev);
> +    return trace_event_get_cpu_state_dynamic_by_cpu_id(cpu, id);

Based on the above suggestion regarding APIs:

    assert(trace_event_is_vcpu(ev));
    return vcpu_trace_event_get_state_dynamic(cpu, ev->cpu_id);

>  }
>  
>  #endif  /* TRACE__CONTROL_INTERNAL_H */
> diff --git a/trace/control-stub.c b/trace/control-stub.c
> new file mode 100644
> index 0000000..858b13e
> --- /dev/null
> +++ b/trace/control-stub.c
> @@ -0,0 +1,29 @@
> +/*
> + * Interface for configuring and controlling the state of tracing events.
> + *
> + * Copyright (C) 2014-2016 Lluís Vilanova <vilanova@ac.upc.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "trace/control.h"

This is not a stub, in fact it has a bunch of duplicate code with
trace/control.c.

The actual stubs are trace_event_set_cpu_state_dynamic() (which I'd
rename to vcpu_trace_event_set_state_dynamic) and
vcpu_trace_event_set_state_dynamic_all that does a CPU_FOREACH.

That said, I am skeptical about the benefit of the interfaces you are
adding.  They add a lot of complication and overhead (especially
regarding the memory/cache overhead of the dstate array) without a clear
use case, in my opinion; all the processing you do at run-time is just
as well suited for later filtering.

I also believe that it's a bad idea to add "stuff" to trace-tool without
a user; unless I'm mistaken neither "vcpu" nor "tcg" trace events are
unused in qemu.git, and this means that the ~400 lines added in this
series are actually dead code.

Paolo