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