[3/6,trivial] trace: Cosmetic changes on fast-path tracing

Commit Message

Lluís Vilanova Feb. 25, 2016, 3:03 p.m. UTC

Signed-off-by: Lluís Vilanova <[e-mail]>
---
 trace/control-internal.h |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Stefan Hajnoczi June 9, 2016, 12:11 p.m. UTC | #1

On Thu, Feb 25, 2016 at 04:03:00PM +0100, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <[e-mail]>
> ---
>  trace/control-internal.h |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Paolo Bonzini June 13, 2016, 9:04 a.m. UTC | #2

On 25/02/2016 16:03, Lluís Vilanova wrote:
>  static inline bool trace_event_get_state_dynamic(TraceEvent *ev)
>  {
> -    int id = trace_event_get_id(ev);
> +    TraceEventID id;
> +    assert(ev != NULL);

Please don't add "!= NULL" asserts.  The reason of a crash would be
pretty obvious from the backtrace.

In fact a NULL event is especially unlikely given that all events are
held in an array...

Paolo

> +    assert(trace_event_get_state_static(ev));
> +    id = trace_event_get_id(ev);
>      return trace_event_get_state_dynamic_by_id(id);
>  }
>

Lluís Vilanova June 13, 2016, 1:39 p.m. UTC | #3

Paolo Bonzini writes:

> On 25/02/2016 16:03, Lluís Vilanova wrote:
>> static inline bool trace_event_get_state_dynamic(TraceEvent *ev)
>> {
>> -    int id = trace_event_get_id(ev);
>> +    TraceEventID id;
>> +    assert(ev != NULL);

> Please don't add "!= NULL" asserts.  The reason of a crash would be
> pretty obvious from the backtrace.

> In fact a NULL event is especially unlikely given that all events are
> held in an array...

Actually, I was thinking about removing all "assert(ev)" on a separate patch,
since it is very unlikely this will ever happen (and asserts are always compiled
in in QEMU).


Thanks,
  Lluis

Patch

diff --git a/trace/control-internal.h b/trace/control-internal.h
index c78a45a..d1f99e3 100644
--- a/trace/control-internal.h
+++ b/trace/control-internal.h
@@ -58,14 +58,18 @@  static inline bool trace_event_get_state_static(TraceEvent *ev)
     return ev->sstate;
 }
 
-static inline bool trace_event_get_state_dynamic_by_id(int id)
+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];
 }
 
 static inline bool trace_event_get_state_dynamic(TraceEvent *ev)
 {
-    int id = trace_event_get_id(ev);
+    TraceEventID id;
+    assert(ev != NULL);
+    assert(trace_event_get_state_static(ev));
+    id = trace_event_get_id(ev);
     return trace_event_get_state_dynamic_by_id(id);
 }