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

[Lluís Vilanova]

Paolo Bonzini writes:

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

I'll fix these two.


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

And the asserts too.


>> 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 <address@hidden>
>> + *
>> + * 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 follows the name convention of "sources compiled when there is not target"
(like some commandline tools). If there is another naming convention for such
cases, please let me know.


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

This should make tracing faster on the future with multi-threaded TCG, as well
as trace files much smaller if you're tracing something like memory
accesses. Also, bear in mind this series was split from a much larger one for
simplicity. The follow-up one provides much larger performance benefits by
avoiding the generation of TCG code to call the tracing backend when a vCPU is
not traced.


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

Events using these features are being added in parallel to this series, like the
recently accepted "guest_mem_before". I also have some events on my queue
tracing bbl and instruction execution, as well as user-mode syscalls.


Thanks,
  Lluis