| Message ID | 1473872922-23449-6-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
Daniel P Berrange writes: > Since there will shortly be multiple event groups allowed, > we can no longer use the TraceEventID and TraceEventVCPUID > enums in the trace control APIs. There will in fact be > multiple distinct enums, and the enum values will only be > required to be unique per group. This patch serves no purpose without the event group patches. Also, AFAIR TraceEventVCPUID still needs to be a flat space (they're all used as bitmask indexes), so keeping the enum won't lose any re-compilation benefit. And without wanting to sound like a broken record, you can make the "TRACE_${EVENTNAME}" IDs be global Event* variables (statically initialized in "trace/generated-events.c"). That still allows using their names in the macros, avoids having a (two-level) tree of events, and eliminates the need for the Event::id member (and the trace_event_get_id() function). Cheers, Lluis [...] > diff --git a/trace/simple.c b/trace/simple.c > index 2f09daf..6e8013c 100644 > --- a/trace/simple.c > +++ b/trace/simple.c > @@ -18,7 +18,7 @@ > #include "trace/simple.h" > /** Trace file header event ID */ > -#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */ > +#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with event IDs */ > /** Trace file magic number */ > #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL > @@ -58,7 +58,7 @@ static char *trace_file_name; > /* * Trace buffer entry */ > typedef struct { > - uint64_t event; /* TraceEventID */ > + uint64_t event; /* event ID */ > uint64_t timestamp_ns; > uint32_t length; /* in bytes */ > uint32_t pid; > @@ -202,7 +202,7 @@ void trace_record_write_str(TraceBufferRecord *rec, const char *s, uint32_t slen rec-> rec_off = write_to_buffer(rec->rec_off, (void*)s, slen); > } > -int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize) > +int trace_record_start(TraceBufferRecord *rec, uint32_t event, size_t datasize) > { > unsigned int idx, rec_off, old_idx, new_idx; > uint32_t rec_len = sizeof(TraceRecord) + datasize; > diff --git a/trace/simple.h b/trace/simple.h > index 1e7de45..17ce472 100644 > --- a/trace/simple.h > +++ b/trace/simple.h > @@ -33,7 +33,7 @@ typedef struct { > * > * @arglen number of bytes required for arguments > */ > -int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen); > +int trace_record_start(TraceBufferRecord *rec, uint32_t id, size_t arglen); > /** > * Append a 64-bit argument to a trace record Not incorrect, but it's weird that the simple backend emits 64-bit identifiers while QEMU uses 32-bit ones. Cheers, Lluis
On Thu, Sep 15, 2016 at 01:26:06AM +0200, Lluís Vilanova wrote: > Daniel P Berrange writes: > > > Since there will shortly be multiple event groups allowed, > > we can no longer use the TraceEventID and TraceEventVCPUID > > enums in the trace control APIs. There will in fact be > > multiple distinct enums, and the enum values will only be > > required to be unique per group. > > This patch serves no purpose without the event group patches. > > Also, AFAIR TraceEventVCPUID still needs to be a flat space (they're all used as > bitmask indexes), so keeping the enum won't lose any re-compilation benefit. > > And without wanting to sound like a broken record, you can make the > "TRACE_${EVENTNAME}" IDs be global Event* variables (statically initialized in > "trace/generated-events.c"). That still allows using their names in the macros, > avoids having a (two-level) tree of events, and eliminates the need for the > Event::id member (and the trace_event_get_id() function). Regardless of whether its used in the public API, we still need to be able to assign a unique 32bit event ID to every event, because simpletrace currently needs to output that in its trace files. That said, i have been wondering if people are very tied to the current simple trace output format ? My ideal solution would be for us to dynamically assign id values to trace events when QEMU starts up. If we did this, hen in the simpletrace output file, we would have to insert a new record type which records an (event name, ID) value mapping, the first time any individual event type is emitted. Thus when simpletrace.py comes to load the trace data, instead of using the event ID to lookup the event directly, it would use the event ID to get the name from the trace data, and then lookup the event based on name. > [...] > > diff --git a/trace/simple.c b/trace/simple.c > > index 2f09daf..6e8013c 100644 > > --- a/trace/simple.c > > +++ b/trace/simple.c > > @@ -18,7 +18,7 @@ > > #include "trace/simple.h" > > > /** Trace file header event ID */ > > -#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */ > > +#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with event IDs */ > > > /** Trace file magic number */ > > #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL > > @@ -58,7 +58,7 @@ static char *trace_file_name; > > > /* * Trace buffer entry */ > > typedef struct { > > - uint64_t event; /* TraceEventID */ > > + uint64_t event; /* event ID */ > > uint64_t timestamp_ns; > > uint32_t length; /* in bytes */ > > uint32_t pid; > > @@ -202,7 +202,7 @@ void trace_record_write_str(TraceBufferRecord *rec, const char *s, uint32_t slen > rec-> rec_off = write_to_buffer(rec->rec_off, (void*)s, slen); > > } > > > -int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize) > > +int trace_record_start(TraceBufferRecord *rec, uint32_t event, size_t datasize) > > { > > unsigned int idx, rec_off, old_idx, new_idx; > > uint32_t rec_len = sizeof(TraceRecord) + datasize; > > diff --git a/trace/simple.h b/trace/simple.h > > index 1e7de45..17ce472 100644 > > --- a/trace/simple.h > > +++ b/trace/simple.h > > @@ -33,7 +33,7 @@ typedef struct { > > * > > * @arglen number of bytes required for arguments > > */ > > -int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen); > > +int trace_record_start(TraceBufferRecord *rec, uint32_t id, size_t arglen); > > > /** > > * Append a 64-bit argument to a trace record > > Not incorrect, but it's weird that the simple backend emits 64-bit identifiers > while QEMU uses 32-bit ones. The trace_record_start method *is* given a 32-bit id value not a 64-bit value. That comment you're quoting here is nothing todo with the ID values, it is about writing 64-bit parameter values. Regards, Daniel
Daniel P Berrange writes: > On Thu, Sep 15, 2016 at 01:26:06AM +0200, Lluís Vilanova wrote: >> Daniel P Berrange writes: >> >> > Since there will shortly be multiple event groups allowed, >> > we can no longer use the TraceEventID and TraceEventVCPUID >> > enums in the trace control APIs. There will in fact be >> > multiple distinct enums, and the enum values will only be >> > required to be unique per group. >> >> This patch serves no purpose without the event group patches. >> >> Also, AFAIR TraceEventVCPUID still needs to be a flat space (they're all used as >> bitmask indexes), so keeping the enum won't lose any re-compilation benefit. >> >> And without wanting to sound like a broken record, you can make the >> "TRACE_${EVENTNAME}" IDs be global Event* variables (statically initialized in >> "trace/generated-events.c"). That still allows using their names in the macros, >> avoids having a (two-level) tree of events, and eliminates the need for the >> Event::id member (and the trace_event_get_id() function). > Regardless of whether its used in the public API, we still need to > be able to assign a unique 32bit event ID to every event, because > simpletrace currently needs to output that in its trace files. > That said, i have been wondering if people are very tied to the > current simple trace output format ? My ideal solution would be > for us to dynamically assign id values to trace events when QEMU > starts up. > If we did this, hen in the simpletrace output file, we would have > to insert a new record type which records an (event name, ID) > value mapping, the first time any individual event type is > emitted. > Thus when simpletrace.py comes to load the trace data, instead > of using the event ID to lookup the event directly, it would use > the event ID to get the name from the trace data, and then lookup > the event based on name. A self-describing simpletrace would be so much better... Instead of registering event types the first time they're emitted, it'd be more efficient (and probably simpler) to register them during initialization of the trace subsystem. Probably not worth the effort, but you also have the self-describing CTF trace format. Already has tools for reading, converting and visualizing traces. >> [...] >> > diff --git a/trace/simple.c b/trace/simple.c >> > index 2f09daf..6e8013c 100644 >> > --- a/trace/simple.c >> > +++ b/trace/simple.c >> > @@ -18,7 +18,7 @@ >> > #include "trace/simple.h" >> >> > /** Trace file header event ID */ >> > -#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */ >> > +#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with event IDs */ >> >> > /** Trace file magic number */ >> > #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL >> > @@ -58,7 +58,7 @@ static char *trace_file_name; >> >> > /* * Trace buffer entry */ >> > typedef struct { >> > - uint64_t event; /* TraceEventID */ >> > + uint64_t event; /* event ID */ >> > uint64_t timestamp_ns; >> > uint32_t length; /* in bytes */ >> > uint32_t pid; >> > @@ -202,7 +202,7 @@ void trace_record_write_str(TraceBufferRecord *rec, const char *s, uint32_t slen rec-> rec_off = write_to_buffer(rec->rec_off, (void*)s, slen); >> > } >> >> > -int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize) >> > +int trace_record_start(TraceBufferRecord *rec, uint32_t event, size_t datasize) >> > { >> > unsigned int idx, rec_off, old_idx, new_idx; >> > uint32_t rec_len = sizeof(TraceRecord) + datasize; >> > diff --git a/trace/simple.h b/trace/simple.h >> > index 1e7de45..17ce472 100644 >> > --- a/trace/simple.h >> > +++ b/trace/simple.h >> > @@ -33,7 +33,7 @@ typedef struct { >> > * >> > * @arglen number of bytes required for arguments >> > */ >> > -int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen); >> > +int trace_record_start(TraceBufferRecord *rec, uint32_t id, size_t arglen); >> >> > /** >> > * Append a 64-bit argument to a trace record >> >> Not incorrect, but it's weird that the simple backend emits 64-bit identifiers >> while QEMU uses 32-bit ones. > The trace_record_start method *is* given a 32-bit id value not a 64-bit value. > That comment you're quoting here is nothing todo with the ID values, it is > about writing 64-bit parameter values. Makes sense. Cheers, Lluis
On Thu, Sep 15, 2016 at 02:20:05PM +0200, Lluís Vilanova wrote: > Daniel P Berrange writes: > > > On Thu, Sep 15, 2016 at 01:26:06AM +0200, Lluís Vilanova wrote: > >> Daniel P Berrange writes: > >> > >> > Since there will shortly be multiple event groups allowed, > >> > we can no longer use the TraceEventID and TraceEventVCPUID > >> > enums in the trace control APIs. There will in fact be > >> > multiple distinct enums, and the enum values will only be > >> > required to be unique per group. > >> > >> This patch serves no purpose without the event group patches. > >> > >> Also, AFAIR TraceEventVCPUID still needs to be a flat space (they're all used as > >> bitmask indexes), so keeping the enum won't lose any re-compilation benefit. > >> > >> And without wanting to sound like a broken record, you can make the > >> "TRACE_${EVENTNAME}" IDs be global Event* variables (statically initialized in > >> "trace/generated-events.c"). That still allows using their names in the macros, > >> avoids having a (two-level) tree of events, and eliminates the need for the > >> Event::id member (and the trace_event_get_id() function). > > > Regardless of whether its used in the public API, we still need to > > be able to assign a unique 32bit event ID to every event, because > > simpletrace currently needs to output that in its trace files. > > > That said, i have been wondering if people are very tied to the > > current simple trace output format ? My ideal solution would be > > for us to dynamically assign id values to trace events when QEMU > > starts up. > > > If we did this, hen in the simpletrace output file, we would have > > to insert a new record type which records an (event name, ID) > > value mapping, the first time any individual event type is > > emitted. > > > Thus when simpletrace.py comes to load the trace data, instead > > of using the event ID to lookup the event directly, it would use > > the event ID to get the name from the trace data, and then lookup > > the event based on name. > > A self-describing simpletrace would be so much better... Instead of registering > event types the first time they're emitted, it'd be more efficient (and probably > simpler) to register them during initialization of the trace subsystem. We can easily make it self-describing enough that you don't need to read 'trace-events' in order to parse the data. Whether we go further and also include the arg names and format strings to enable analysers to process the data, I'm not sure. We can do it incrementally I guess. THe only reason I suggested at time of first use is that we have quite a large number of events and so emitting description for all of them would take non-negligble time and possibly use significant space. I will investigate further to see if this is genuinely a problem. Regards, Daniel
diff --git a/trace/control-internal.h b/trace/control-internal.h index 1446498..2981f11 100644 --- a/trace/control-internal.h +++ b/trace/control-internal.h @@ -25,13 +25,13 @@ static inline bool trace_event_is_pattern(const char *str) return strchr(str, '*') != NULL; } -static inline TraceEventID trace_event_get_id(TraceEvent *ev) +static inline uint32_t trace_event_get_id(TraceEvent *ev) { assert(ev != NULL); return ev->id; } -static inline TraceEventVCPUID trace_event_get_vcpu_id(TraceEvent *ev) +static inline uint32_t trace_event_get_vcpu_id(TraceEvent *ev) { return ev->vcpu_id; } @@ -63,7 +63,7 @@ static inline bool trace_event_get_state_dynamic(TraceEvent *ev) } static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu, - TraceEventVCPUID id) + uint32_t id) { /* it's on fast path, avoid consistency checks (asserts) */ if (unlikely(trace_events_enabled_count)) { @@ -76,7 +76,7 @@ static inline bool trace_event_get_vcpu_state_dynamic_by_vcpu_id(CPUState *vcpu, static inline bool trace_event_get_vcpu_state_dynamic(CPUState *vcpu, TraceEvent *ev) { - TraceEventVCPUID id; + uint32_t id; assert(trace_event_is_vcpu(ev)); id = trace_event_get_vcpu_id(ev); return trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, id); diff --git a/trace/control-target.c b/trace/control-target.c index c69dda9..ff1bf43 100644 --- a/trace/control-target.c +++ b/trace/control-target.c @@ -59,7 +59,7 @@ void trace_event_set_state_dynamic(TraceEvent *ev, bool state) void trace_event_set_vcpu_state_dynamic(CPUState *vcpu, TraceEvent *ev, bool state) { - TraceEventVCPUID vcpu_id; + uint32_t vcpu_id; bool state_pre; assert(trace_event_get_state_static(ev)); assert(trace_event_is_vcpu(ev)); diff --git a/trace/control.h b/trace/control.h index e80c220..d660fd2 100644 --- a/trace/control.h +++ b/trace/control.h @@ -19,18 +19,6 @@ typedef struct TraceEventIter { } TraceEventIter; /** - * TraceEventID: - * - * Unique tracing event identifier. - * - * These are named as 'TRACE_${EVENT_NAME}'. - * - * See also: "trace/generated-events.h" - */ -enum TraceEventID; - - -/** * trace_event_iter_init: * @iter: the event iterator struct * @pattern: optional pattern to filter events on name @@ -76,7 +64,7 @@ static bool trace_event_is_pattern(const char *str); * * Get the identifier of an event. */ -static TraceEventID trace_event_get_id(TraceEvent *ev); +static uint32_t trace_event_get_id(TraceEvent *ev); /** * trace_event_get_vcpu_id: @@ -86,7 +74,7 @@ static TraceEventID trace_event_get_id(TraceEvent *ev); * Special value #TRACE_VCPU_EVENT_COUNT means the event is not vCPU-specific * (does not have the "vcpu" property). */ -static TraceEventVCPUID trace_event_get_vcpu_id(TraceEvent *ev); +static uint32_t trace_event_get_vcpu_id(TraceEvent *ev); /** * trace_event_is_vcpu: @@ -111,7 +99,7 @@ static const char * trace_event_get_name(TraceEvent *ev); * If the event has the disabled property, the check will have no performance * impact. * - * As a down side, you must always use an immediate #TraceEventID value. + * As a down side, you must always use an immediate event ID value. */ #define trace_event_get_state(id) \ ((id ##_ENABLED) && trace_event_get_state_dynamic_by_id(id)) @@ -119,8 +107,8 @@ static const char * trace_event_get_name(TraceEvent *ev); /** * trace_event_get_vcpu_state: * @vcpu: Target vCPU. - * @id: Event identifier (TraceEventID). - * @vcpu_id: Per-vCPU event identifier (TraceEventVCPUID). + * @id: Event identifier. + * @vcpu_id: Per-vCPU event identifier. * * Get the tracing state of an event (both static and dynamic) for the given * vCPU. @@ -128,7 +116,7 @@ static const char * trace_event_get_name(TraceEvent *ev); * If the event has the disabled property, the check will have no performance * impact. * - * As a down side, you must always use an immediate #TraceEventID value. + * As a down side, you must always use an immediate event ID value. */ #define trace_event_get_vcpu_state(vcpu, id, vcpu_id) \ ((id ##_ENABLED) && trace_event_get_vcpu_state_dynamic_by_vcpu_id(vcpu, vcpu_id)) diff --git a/trace/event-internal.h b/trace/event-internal.h index 3b9ceb5..d57f2d9 100644 --- a/trace/event-internal.h +++ b/trace/event-internal.h @@ -28,8 +28,8 @@ * Opaque generic description of a tracing event. */ typedef struct TraceEvent { - TraceEventID id; - TraceEventVCPUID vcpu_id; + uint32_t id; + uint32_t vcpu_id; const char * name; const bool sstate; uint16_t *dstate; diff --git a/trace/simple.c b/trace/simple.c index 2f09daf..6e8013c 100644 --- a/trace/simple.c +++ b/trace/simple.c @@ -18,7 +18,7 @@ #include "trace/simple.h" /** Trace file header event ID */ -#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */ +#define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with event IDs */ /** Trace file magic number */ #define HEADER_MAGIC 0xf2b177cb0aa429b4ULL @@ -58,7 +58,7 @@ static char *trace_file_name; /* * Trace buffer entry */ typedef struct { - uint64_t event; /* TraceEventID */ + uint64_t event; /* event ID */ uint64_t timestamp_ns; uint32_t length; /* in bytes */ uint32_t pid; @@ -202,7 +202,7 @@ void trace_record_write_str(TraceBufferRecord *rec, const char *s, uint32_t slen rec->rec_off = write_to_buffer(rec->rec_off, (void*)s, slen); } -int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t datasize) +int trace_record_start(TraceBufferRecord *rec, uint32_t event, size_t datasize) { unsigned int idx, rec_off, old_idx, new_idx; uint32_t rec_len = sizeof(TraceRecord) + datasize; diff --git a/trace/simple.h b/trace/simple.h index 1e7de45..17ce472 100644 --- a/trace/simple.h +++ b/trace/simple.h @@ -33,7 +33,7 @@ typedef struct { * * @arglen number of bytes required for arguments */ -int trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen); +int trace_record_start(TraceBufferRecord *rec, uint32_t id, size_t arglen); /** * Append a 64-bit argument to a trace record
Since there will shortly be multiple event groups allowed, we can no longer use the TraceEventID and TraceEventVCPUID enums in the trace control APIs. There will in fact be multiple distinct enums, and the enum values will only be required to be unique per group. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- trace/control-internal.h | 8 ++++---- trace/control-target.c | 2 +- trace/control.h | 24 ++++++------------------ trace/event-internal.h | 4 ++-- trace/simple.c | 6 +++--- trace/simple.h | 2 +- 6 files changed, 17 insertions(+), 29 deletions(-)