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