Re: [Qemu-devel] [PATCH v5 14/20] trace: dynamically allocate trace_dstate in CPUState

[Lluís Vilanova]

Daniel P Berrange writes:

> The CPUState struct has a bitmap tracking which VCPU
> events are currently active. This is indexed based on
> the event ID values, and sized according the maximum
> TraceEventVCPUID enum value.

> When we start dynamically assigning IDs at runtime,
> we can't statically declare a bitmap without making
> an assumption about the max event count. This problem
> can be solved by dynamically allocating the per-CPU
> dstate bitmap.

> Signed-off-by: Daniel P. Berrange <address@hidden>

Besides the comments below:

Reviewed-by: Lluís Vilanova <address@hidden>


> ---
>  include/qom/cpu.h | 9 ++++++---
>  qom/cpu.c         | 8 ++++++--
>  trace/control.c   | 5 +++++
>  trace/control.h   | 7 +++++++
>  4 files changed, 24 insertions(+), 5 deletions(-)

> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index ce0c406..bbffde7 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -27,7 +27,6 @@
>  #include "qemu/bitmap.h"
>  #include "qemu/queue.h"
>  #include "qemu/thread.h"
> -#include "trace/generated-events.h"
 
>  typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,
>                                       void *opaque);
> @@ -350,8 +349,12 @@ struct CPUState {
>      struct KVMState *kvm_state;
>      struct kvm_run *kvm_run;
 
> -    /* Used for events with 'vcpu' and *without* the 'disabled' properties */
> -    DECLARE_BITMAP(trace_dstate, TRACE_VCPU_EVENT_COUNT);
> +    /*
> +     * Used for events with 'vcpu' and *without* the 'disabled' properties

Nitpick: phrase must end with a dot.


> +     * Dynamically allocated based on bitmap requried to hold up to
> +     * trace_get_vcpu_event_count() entries.
> +     */
> +    unsigned long *trace_dstate;
 
>      /* 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 f783b5a..09180f0 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -29,6 +29,7 @@
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/qdev-properties.h"
> +#include "trace/control.h"
 
>  bool cpu_exists(int64_t id)
>  {
> @@ -350,12 +351,15 @@ static void cpu_common_initfn(Object *obj)
>      qemu_mutex_init(&cpu->work_mutex);
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
> -    bitmap_zero(cpu->trace_dstate, TRACE_VCPU_EVENT_COUNT);
> +
> +    cpu->trace_dstate = bitmap_new(trace_get_vcpu_event_count());
>  }
 
>  static void cpu_common_finalize(Object *obj)
>  {
> -    cpu_exec_exit(CPU(obj));
> +    CPUState *cpu = CPU(obj);
> +    cpu_exec_exit(cpu);
> +    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 7be654d..9704f4d 100644
> --- a/trace/control.c
> +++ b/trace/control.c
> @@ -310,3 +310,8 @@ void trace_init_vcpu_events(void)
>          }
>      }
>  }
> +
> +uint32_t trace_get_vcpu_event_count(void)
> +{
> +    return TRACE_VCPU_EVENT_COUNT;
> +}
> diff --git a/trace/control.h b/trace/control.h
> index f241c7f..7a068ec 100644
> --- a/trace/control.h
> +++ b/trace/control.h
> @@ -236,6 +236,13 @@ char *trace_opt_parse(const char *optarg);
>  void trace_init_vcpu_events(void);
 
 
> +/**
> + * trace_get_vcpu_event_count:
> + *
> + * Return the number of known vcpu-specific events
> + */
> +uint32_t trace_get_vcpu_event_count(void);
> +
>  #include "trace/control-internal.h"

Nitpick: I tried to leave 2 empty lines between code and inclusion of "internal"
headers, and also tried to group related functions (1 empty line inside the
group, 2 empty lines between groups).


>  #endif /* TRACE__CONTROL_H */
> -- 
> 2.7.4


Thanks,
  Lluis