| Message ID | 1496975122-16999-3-git-send-email-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Emilio G Cota writes: > There's little point in dynamically allocating the bitmap if we > know at compile-time the max number of events we want to support. > Thus, make room in the struct for the bitmap, which will make things > easier later: this paves the way for upcoming changes, in which > we'll use a u32 to fully capture cpu->trace_dstate. > This change also increases performance by saving a dereference and > improving locality--note that this is important since upcoming work > makes reading this bitmap fairly common. > Signed-off-by: Emilio G. Cota <cota@braap.org> Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu> > --- > include/qom/cpu.h | 9 +++------ > qom/cpu.c | 8 -------- > trace/control.c | 9 ++++++++- > 3 files changed, 11 insertions(+), 15 deletions(-) > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 89ddb68..bc6e20f 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -259,6 +259,7 @@ typedef void (*run_on_cpu_func)(CPUState *cpu, run_on_cpu_data data); > struct qemu_work_item; > #define CPU_UNSET_NUMA_NODE_ID -1 > +#define CPU_TRACE_DSTATE_MAX_EVENTS 32 > /** > * CPUState: > @@ -373,12 +374,8 @@ struct CPUState { > struct KVMState *kvm_state; > struct kvm_run *kvm_run; > - /* > - * Used for events with 'vcpu' and *without* the 'disabled' properties. > - * Dynamically allocated based on bitmap requried to hold up to > - * trace_get_vcpu_event_count() entries. > - */ > - unsigned long *trace_dstate; > + /* Used for events with 'vcpu' and *without* the 'disabled' properties */ > + DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS); > /* TODO Move common fields from CPUArchState here. */ > int cpu_index; /* used by alpha TCG */ > diff --git a/qom/cpu.c b/qom/cpu.c > index 5069876..69fbb9c 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -382,7 +382,6 @@ static void cpu_common_unrealizefn(DeviceState *dev, Error **errp) > static void cpu_common_initfn(Object *obj) > { > - uint32_t count; > CPUState *cpu = CPU(obj); > CPUClass *cc = CPU_GET_CLASS(obj); > @@ -397,18 +396,11 @@ static void cpu_common_initfn(Object *obj) > QTAILQ_INIT(&cpu->breakpoints); > QTAILQ_INIT(&cpu->watchpoints); > - count = trace_get_vcpu_event_count(); > - if (count) { > - cpu->trace_dstate = bitmap_new(count); > - } > - > cpu_exec_initfn(cpu); > } > static void cpu_common_finalize(Object *obj) > { > - CPUState *cpu = CPU(obj); > - g_free(cpu->trace_dstate); > } > static int64_t cpu_common_get_arch_id(CPUState *cpu) > diff --git a/trace/control.c b/trace/control.c > index 9b157b0..83740aa 100644 > --- a/trace/control.c > +++ b/trace/control.c > @@ -65,8 +65,15 @@ void trace_event_register_group(TraceEvent **events) > size_t i; > for (i = 0; events[i] != NULL; i++) { > events[i]->id = next_id++; > - if (events[i]->vcpu_id != TRACE_VCPU_EVENT_NONE) { > + if (events[i]->vcpu_id == TRACE_VCPU_EVENT_NONE) { > + continue; > + } > + > + if (likely(next_vcpu_id < CPU_TRACE_DSTATE_MAX_EVENTS)) { > events[i]->vcpu_id = next_vcpu_id++; > + } else { > + error_report("WARNING: too many vcpu trace events; dropping '%s'", > + events[i]->name); > } > } > event_groups = g_renew(TraceEventGroup, event_groups, nevent_groups + 1); > -- > 2.7.4
Lluís Vilanova writes: > Emilio G Cota writes: >> There's little point in dynamically allocating the bitmap if we >> know at compile-time the max number of events we want to support. >> Thus, make room in the struct for the bitmap, which will make things >> easier later: this paves the way for upcoming changes, in which >> we'll use a u32 to fully capture cpu->trace_dstate. >> This change also increases performance by saving a dereference and >> improving locality--note that this is important since upcoming work >> makes reading this bitmap fairly common. >> Signed-off-by: Emilio G. Cota <cota@braap.org> > Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu> BTW, I think this partially undoes Daniel's changes in b7d48952c375842bd669460fd8384d90cc12286c. You should check with him (CC'ed). Lluis >> --- >> include/qom/cpu.h | 9 +++------ >> qom/cpu.c | 8 -------- >> trace/control.c | 9 ++++++++- >> 3 files changed, 11 insertions(+), 15 deletions(-) >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index 89ddb68..bc6e20f 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -259,6 +259,7 @@ typedef void (*run_on_cpu_func)(CPUState *cpu, run_on_cpu_data data); >> struct qemu_work_item; >> #define CPU_UNSET_NUMA_NODE_ID -1 >> +#define CPU_TRACE_DSTATE_MAX_EVENTS 32 >> /** >> * CPUState: >> @@ -373,12 +374,8 @@ struct CPUState { >> struct KVMState *kvm_state; >> struct kvm_run *kvm_run; >> - /* >> - * Used for events with 'vcpu' and *without* the 'disabled' properties. >> - * Dynamically allocated based on bitmap requried to hold up to >> - * trace_get_vcpu_event_count() entries. >> - */ >> - unsigned long *trace_dstate; >> + /* Used for events with 'vcpu' and *without* the 'disabled' properties */ >> + DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS); >> /* TODO Move common fields from CPUArchState here. */ >> int cpu_index; /* used by alpha TCG */ >> diff --git a/qom/cpu.c b/qom/cpu.c >> index 5069876..69fbb9c 100644 >> --- a/qom/cpu.c >> +++ b/qom/cpu.c >> @@ -382,7 +382,6 @@ static void cpu_common_unrealizefn(DeviceState *dev, Error **errp) >> static void cpu_common_initfn(Object *obj) >> { >> - uint32_t count; >> CPUState *cpu = CPU(obj); >> CPUClass *cc = CPU_GET_CLASS(obj); >> @@ -397,18 +396,11 @@ static void cpu_common_initfn(Object *obj) >> QTAILQ_INIT(&cpu->breakpoints); >> QTAILQ_INIT(&cpu->watchpoints); >> - count = trace_get_vcpu_event_count(); >> - if (count) { >> - cpu->trace_dstate = bitmap_new(count); >> - } >> - >> cpu_exec_initfn(cpu); >> } >> static void cpu_common_finalize(Object *obj) >> { >> - CPUState *cpu = CPU(obj); >> - g_free(cpu->trace_dstate); >> } >> static int64_t cpu_common_get_arch_id(CPUState *cpu) >> diff --git a/trace/control.c b/trace/control.c >> index 9b157b0..83740aa 100644 >> --- a/trace/control.c >> +++ b/trace/control.c >> @@ -65,8 +65,15 @@ void trace_event_register_group(TraceEvent **events) >> size_t i; >> for (i = 0; events[i] != NULL; i++) { >> events[i]->id = next_id++; >> - if (events[i]->vcpu_id != TRACE_VCPU_EVENT_NONE) { >> + if (events[i]->vcpu_id == TRACE_VCPU_EVENT_NONE) { >> + continue; >> + } >> + >> + if (likely(next_vcpu_id < CPU_TRACE_DSTATE_MAX_EVENTS)) { >> events[i]->vcpu_id = next_vcpu_id++; >> + } else { >> + error_report("WARNING: too many vcpu trace events; dropping '%s'", >> + events[i]->name); >> } >> } >> event_groups = g_renew(TraceEventGroup, event_groups, nevent_groups + 1); >> -- >> 2.7.4
On Sun, Jun 25, 2017 at 12:41:57PM +0300, Lluís Vilanova wrote: > Lluís Vilanova writes: > > > Emilio G Cota writes: > >> There's little point in dynamically allocating the bitmap if we > >> know at compile-time the max number of events we want to support. > >> Thus, make room in the struct for the bitmap, which will make things > >> easier later: this paves the way for upcoming changes, in which > >> we'll use a u32 to fully capture cpu->trace_dstate. > > >> This change also increases performance by saving a dereference and > >> improving locality--note that this is important since upcoming work > >> makes reading this bitmap fairly common. > > >> Signed-off-by: Emilio G. Cota <cota@braap.org> > > > Reviewed-by: Lluís Vilanova <vilanova@ac.upc.edu> > > BTW, I think this partially undoes Daniel's changes in > b7d48952c375842bd669460fd8384d90cc12286c. > > You should check with him (CC'ed). That's ok - I only made those changes in order to remove the reliance on the generated max vcpu event ID counter. Choosing to hardcode a fixed limit on number of vcpu events instead is a fine alternative. > > Lluis > > > >> --- > >> include/qom/cpu.h | 9 +++------ > >> qom/cpu.c | 8 -------- > >> trace/control.c | 9 ++++++++- > >> 3 files changed, 11 insertions(+), 15 deletions(-) > > >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h > >> index 89ddb68..bc6e20f 100644 > >> --- a/include/qom/cpu.h > >> +++ b/include/qom/cpu.h > >> @@ -259,6 +259,7 @@ typedef void (*run_on_cpu_func)(CPUState *cpu, run_on_cpu_data data); > >> struct qemu_work_item; > > >> #define CPU_UNSET_NUMA_NODE_ID -1 > >> +#define CPU_TRACE_DSTATE_MAX_EVENTS 32 > > >> /** > >> * CPUState: > >> @@ -373,12 +374,8 @@ struct CPUState { > >> struct KVMState *kvm_state; > >> struct kvm_run *kvm_run; > > >> - /* > >> - * Used for events with 'vcpu' and *without* the 'disabled' properties. > >> - * Dynamically allocated based on bitmap requried to hold up to > >> - * trace_get_vcpu_event_count() entries. > >> - */ > >> - unsigned long *trace_dstate; > >> + /* Used for events with 'vcpu' and *without* the 'disabled' properties */ > >> + DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS); > > >> /* TODO Move common fields from CPUArchState here. */ > >> int cpu_index; /* used by alpha TCG */ > >> diff --git a/qom/cpu.c b/qom/cpu.c > >> index 5069876..69fbb9c 100644 > >> --- a/qom/cpu.c > >> +++ b/qom/cpu.c > >> @@ -382,7 +382,6 @@ static void cpu_common_unrealizefn(DeviceState *dev, Error **errp) > > >> static void cpu_common_initfn(Object *obj) > >> { > >> - uint32_t count; > >> CPUState *cpu = CPU(obj); > >> CPUClass *cc = CPU_GET_CLASS(obj); > > >> @@ -397,18 +396,11 @@ static void cpu_common_initfn(Object *obj) > >> QTAILQ_INIT(&cpu->breakpoints); > >> QTAILQ_INIT(&cpu->watchpoints); > > >> - count = trace_get_vcpu_event_count(); > >> - if (count) { > >> - cpu->trace_dstate = bitmap_new(count); > >> - } > >> - > >> cpu_exec_initfn(cpu); > >> } > > >> static void cpu_common_finalize(Object *obj) > >> { > >> - CPUState *cpu = CPU(obj); > >> - g_free(cpu->trace_dstate); > >> } > > >> static int64_t cpu_common_get_arch_id(CPUState *cpu) > >> diff --git a/trace/control.c b/trace/control.c > >> index 9b157b0..83740aa 100644 > >> --- a/trace/control.c > >> +++ b/trace/control.c > >> @@ -65,8 +65,15 @@ void trace_event_register_group(TraceEvent **events) > >> size_t i; > >> for (i = 0; events[i] != NULL; i++) { > >> events[i]->id = next_id++; > >> - if (events[i]->vcpu_id != TRACE_VCPU_EVENT_NONE) { > >> + if (events[i]->vcpu_id == TRACE_VCPU_EVENT_NONE) { > >> + continue; > >> + } > >> + > >> + if (likely(next_vcpu_id < CPU_TRACE_DSTATE_MAX_EVENTS)) { > >> events[i]->vcpu_id = next_vcpu_id++; > >> + } else { > >> + error_report("WARNING: too many vcpu trace events; dropping '%s'", > >> + events[i]->name); > >> } This should be an abort IMHO, as it would be considered a bug to have added > 32 vcpu events. I'd also suggest that the top level 'trace-events' file get a comment added to the effect that we only support 32 events right now. Regards, Daniel
diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 89ddb68..bc6e20f 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -259,6 +259,7 @@ typedef void (*run_on_cpu_func)(CPUState *cpu, run_on_cpu_data data); struct qemu_work_item; #define CPU_UNSET_NUMA_NODE_ID -1 +#define CPU_TRACE_DSTATE_MAX_EVENTS 32 /** * CPUState: @@ -373,12 +374,8 @@ struct CPUState { struct KVMState *kvm_state; struct kvm_run *kvm_run; - /* - * Used for events with 'vcpu' and *without* the 'disabled' properties. - * Dynamically allocated based on bitmap requried to hold up to - * trace_get_vcpu_event_count() entries. - */ - unsigned long *trace_dstate; + /* Used for events with 'vcpu' and *without* the 'disabled' properties */ + DECLARE_BITMAP(trace_dstate, CPU_TRACE_DSTATE_MAX_EVENTS); /* TODO Move common fields from CPUArchState here. */ int cpu_index; /* used by alpha TCG */ diff --git a/qom/cpu.c b/qom/cpu.c index 5069876..69fbb9c 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -382,7 +382,6 @@ static void cpu_common_unrealizefn(DeviceState *dev, Error **errp) static void cpu_common_initfn(Object *obj) { - uint32_t count; CPUState *cpu = CPU(obj); CPUClass *cc = CPU_GET_CLASS(obj); @@ -397,18 +396,11 @@ static void cpu_common_initfn(Object *obj) QTAILQ_INIT(&cpu->breakpoints); QTAILQ_INIT(&cpu->watchpoints); - count = trace_get_vcpu_event_count(); - if (count) { - cpu->trace_dstate = bitmap_new(count); - } - cpu_exec_initfn(cpu); } static void cpu_common_finalize(Object *obj) { - CPUState *cpu = CPU(obj); - g_free(cpu->trace_dstate); } static int64_t cpu_common_get_arch_id(CPUState *cpu) diff --git a/trace/control.c b/trace/control.c index 9b157b0..83740aa 100644 --- a/trace/control.c +++ b/trace/control.c @@ -65,8 +65,15 @@ void trace_event_register_group(TraceEvent **events) size_t i; for (i = 0; events[i] != NULL; i++) { events[i]->id = next_id++; - if (events[i]->vcpu_id != TRACE_VCPU_EVENT_NONE) { + if (events[i]->vcpu_id == TRACE_VCPU_EVENT_NONE) { + continue; + } + + if (likely(next_vcpu_id < CPU_TRACE_DSTATE_MAX_EVENTS)) { events[i]->vcpu_id = next_vcpu_id++; + } else { + error_report("WARNING: too many vcpu trace events; dropping '%s'", + events[i]->name); } } event_groups = g_renew(TraceEventGroup, event_groups, nevent_groups + 1);
There's little point in dynamically allocating the bitmap if we know at compile-time the max number of events we want to support. Thus, make room in the struct for the bitmap, which will make things easier later: this paves the way for upcoming changes, in which we'll use a u32 to fully capture cpu->trace_dstate. This change also increases performance by saving a dereference and improving locality--note that this is important since upcoming work makes reading this bitmap fairly common. Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/qom/cpu.h | 9 +++------ qom/cpu.c | 8 -------- trace/control.c | 9 ++++++++- 3 files changed, 11 insertions(+), 15 deletions(-)