On Thu, Feb 25, 2016 at 04:03:06PM +0100, Lluís Vilanova wrote:
@@ -332,6 +334,16 @@ struct CPUState {
> struct KVMState *kvm_state;
> struct kvm_run *kvm_run;
>
> +#define TRACE_VCPU_DSTATE_TYPE uint32_t
> + TRACE_VCPU_DSTATE_TYPE trace_dstate;
Please use typedef instead of #define.
> + /*
> + * Ensure 'trace_dstate' can encode event states as a bitmask. This
> limits
> + * the number of events with the 'vcpu' property and *without* the
> + * 'disabled' property.
> + */
> + bool too_many_vcpu_events[
> + TRACE_VCPU_EVENT_COUNT > sizeof(TRACE_VCPU_DSTATE_TYPE)*8 ? -1 : 0];
Why limit ourselves to a scalar when "qemu/bitops.h" and "qemu/bitmap.h"
provide functions for arbitrary length bitmaps?
See DECLARE_BITMAP(), set_bit(), clear_bit(), test_bit().
> @@ -61,7 +69,7 @@ static inline bool trace_event_get_state_static(TraceEvent
> *ev)
> static inline bool trace_event_get_state_dynamic_by_id(TraceEventID id)
> {
> /* it's on fast path, avoid consistency checks (asserts) */
> - return unlikely(trace_events_enabled_count) && trace_events_dstate[id];
> + return unlikely(trace_events_enabled_count) && (trace_events_dstate[id]
> > 0);
typeof(trace_events_dstate[0]) is size_t, so trace_events_dstate[id] > 0
is equivalent to trace_events_dstate[id] (due to unsigned). Why change
this line?
> +void trace_event_set_state_dynamic(TraceEvent *ev, bool state)
> +{
> + CPUState *cpu;
> + assert(ev != NULL);
> + assert(trace_event_get_state_static(ev));
> + if (trace_event_get_cpu_id(ev) != trace_event_cpu_count()) {
> + CPU_FOREACH(cpu) {
> + trace_event_set_cpu_state_dynamic(cpu, ev, state);
> + }
> + } else {
> + TraceEventID id = trace_event_get_id(ev);
> + trace_events_enabled_count += state - trace_events_dstate[id];
> + trace_events_dstate[id] = state;
> + }
> +}
I find it a little confusing to use different semantics for
trace_events_dstate[] elements depending on trace_event_get_cpu_id(ev)
!= trace_event_cpu_count(). In other words, it either acts as a vcpu
enabled counter or as an enable/disable flag.
That said, it's nice to preserve the non-cpu_id case since it was
written by Paolo as a performance optimization. Changing it could
introduce a regression so I think your approach is okay.
> +
> +void trace_event_set_cpu_state_dynamic(CPUState *cpu,
> + TraceEvent *ev, bool state)
> +{
> + TraceEventID id;
> + TraceEventVCPUID cpu_id;
> + TRACE_VCPU_DSTATE_TYPE bit;
> + bool state_pre;
> + assert(cpu != NULL);
> + assert(ev != NULL);
> + assert(trace_event_get_state_static(ev));
> + assert(trace_event_get_cpu_id(ev) != trace_event_cpu_count());
> + id = trace_event_get_id(ev);
> + cpu_id = trace_event_get_cpu_id(ev);
> + bit = ((TRACE_VCPU_DSTATE_TYPE)1) << cpu_id;
> + state_pre = cpu->trace_dstate & bit;
> + if ((state_pre == 0) != (state == 0)) {
Simpler expression:
if (state_pre != state)
> @@ -21,7 +21,10 @@
> #include "qemu/error-report.h"
>
> int trace_events_enabled_count;
> -bool trace_events_dstate[TRACE_EVENT_COUNT];
> +/* With the 'vcpu' property, counts how many vCPUs have it enabled. */
> +size_t trace_events_dstate[TRACE_EVENT_COUNT];
The number of cpus has type int (see CPUState *qemu_get_cpu(int index)).
Why did you choose size_t?
signature.asc
Description: PGP signature
