[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for even
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH 4/6] trace: Add per-vCPU tracing states for events with the 'vcpu' property |
Date: |
Thu, 09 Jun 2016 16:17:11 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Stefan Hajnoczi writes:
> 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().
Oh, I wasn't aware of the bitmap functions. Nice catch.
>> @@ -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?
Sorry, I have a tendency to make this type of checks explicit when the types are
not boolean (for a maybe-false sense of future-proofing). I can leave it as it
was if it bothers you.
>> +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.
Yes, it's a bit messy. I'll add some proper documentation about how this is
interpreted.
>> +
>> +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?
It just sounds proper to me to use size_t, since the state can never be negative
(it's either interpreted as a boolean or as an unsigned counter, depending on
the "vcpu" property).
Thanks,
Lluis