Re: [Qemu-devel] [PATCH v3 13/18] trace: dynamically allocate event IDs at runtime

[Lluís Vilanova]

Daniel P Berrange writes:

> Instead of having the code generator assign event IDs and
> event VCPU IDs, assign them when the events are registered
> at runtime. This will allow us allow code to be generated
> from individual trace-events without having to figure out
> globally unique numbering at build time.

> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  scripts/tracetool/format/events_c.py | 10 ++--------
>  scripts/tracetool/format/events_h.py |  7 -------
>  trace/control.c                      | 11 ++++++++++-
>  3 files changed, 12 insertions(+), 16 deletions(-)

> diff --git a/scripts/tracetool/format/events_c.py 
> b/scripts/tracetool/format/events_c.py
> index 7f00b50..1b5790d 100644
> --- a/scripts/tracetool/format/events_c.py
> +++ b/scripts/tracetool/format/events_c.py
> @@ -28,25 +28,19 @@ def generate(events, backend):
>      for e in events:
>          out('uint16_t %s;' % e.api(e.QEMU_DSTATE))
 
> -    next_id = 0
> -    next_vcpu_id = 0
>      for e in events:
> -        id = next_id
> -        next_id += 1
>          if "vcpu" in e.properties:
> -            vcpu_id = next_vcpu_id
> -            next_vcpu_id += 1
> +            vcpu_id = 0
>          else:
>              vcpu_id = "TRACE_VCPU_EVENT_NONE"
>          out('TraceEvent %(event)s = {',
> -            '  .id = %(id)s,',
> +            '  .id = 0,',

Better to use some invalid value, like in TRACE_VCPU_EVENT_NONE.


>              '  .vcpu_id = %(vcpu_id)s,',
>              '  .name = \"%(name)s\",',
>              '  .sstate = %(sstate)s,',
>              '  .dstate = &%(dstate)s ',
>              '};',
>              event = "TRACE_" + e.name.upper() + "_EV",
> -            id = id,
>              vcpu_id = vcpu_id,
>              name = e.name,
>              sstate = "TRACE_%s_ENABLED" % e.name.upper(),
> diff --git a/scripts/tracetool/format/events_h.py 
> b/scripts/tracetool/format/events_h.py
> index 35cbc91..857fcd4 100644
> --- a/scripts/tracetool/format/events_h.py
> +++ b/scripts/tracetool/format/events_h.py
> @@ -31,13 +31,6 @@ def generate(events, backend):
>      for e in events:
>          out('extern uint16_t %s;' % e.api(e.QEMU_DSTATE))
 
> -    numvcpu = 0
> -    for e in events:
> -        if "vcpu" in e.properties:
> -            numvcpu += 1
> -
> -    out("#define TRACE_VCPU_EVENT_COUNT %d" % numvcpu)
> -
>      # static state
>      for e in events:
>          if 'disable' in e.properties:
> diff --git a/trace/control.c b/trace/control.c
> index 0633774..07912bd 100644
> --- a/trace/control.c
> +++ b/trace/control.c
> @@ -35,6 +35,8 @@ typedef struct TraceEventGroup {
 
>  static TraceEventGroup *event_groups;
>  static size_t nevent_groups;
> +static uint32_t next_id;
> +static uint32_t next_vcpu_id;
 
>  QemuOptsList qemu_trace_opts = {
>      .name = "trace",
> @@ -59,6 +61,13 @@ QemuOptsList qemu_trace_opts = {
 
>  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) {
> +            events[i]->vcpu_id = next_vcpu_id++;
> +        }
> +    }
>      event_groups = g_renew(TraceEventGroup, event_groups, nevent_groups + 1);
>      event_groups[nevent_groups].events = events;
>      nevent_groups++;
> @@ -322,5 +331,5 @@ void trace_init_vcpu_events(void)
 
>  uint32_t trace_get_vcpu_event_count(void)
>  {
> -    return TRACE_VCPU_EVENT_COUNT;
> +    return next_vcpu_id;
>  }
> -- 
> 2.7.4

Aha, now I see your reasoning for referencing event.vcpu_id in the fast-path
instead of using an immediate argument (in trace_event_get_vcpu_dstate_by_id(),
or something named like that).

I still think performance would currently be much better with an immediate, at a
low recompilation cost (for now). Still, the multiple TB cache optimization I
sent, if accepted, might dilute these performance benefits.


Cheers,
  Lluis