Re: [Qemu-devel] Re: [PATCH 3/3] Toggle tracepoint state

[Prerna Saxena]

On 06/17/2010 09:33 PM, Stefan Hajnoczi wrote:
> On Wed, Jun 16, 2010 at 06:14:35PM +0530, Prerna Saxena wrote:
> > This patch adds support for dynamically enabling/disabling of tracepoints.
> > This is done by internally maintaining each tracepoint's state, and
> > permitting logging of data from a tracepoint only if it is in an
> > 'active' state.
> > ....
> > ....
> >   typedef struct {
> > +    char *tp_name;
> > +    bool state;
> > +    unsigned int hash;
> > +} Tracepoint;
>
> The tracing infrastructure avoids using the name 'tracepoint'.  It calls
> them trace events.  I didn't deliberately choose that name, but was
> unaware at the time that Linux tracing calls them tracepoints.  Given
> that 'trace event' is currently used, it would be nice to remain
> consistent/reduce confusion.
>
> How about:
> typedef struct {
>      const char *name;
>      bool enabled;
>      unsigned int hash;
> } TraceEventState;
>
> Or a nicer overall change might be to rename enum TraceEvent to
> TraceEventID and Tracepoint to TraceEvent.

I'll change that !

> > +
> > +typedef struct {
> >       unsigned long event;
> >       unsigned long x1;
> >       unsigned long x2;
> > @@ -18,11 +24,29 @@ enum {
> >   static TraceRecord trace_buf[TRACE_BUF_LEN];
> >   static unsigned int trace_idx;
> >   static FILE *trace_fp;
> > +static Tracepoint trace_list[NR_TRACEPOINTS];
> > +
> > +void init_tracepoint(const char *tname, TraceEvent tevent)
> > +{
> > +    if (!tname || tevent>  NR_TRACEPOINTS) {
> > +        return;
> > +    }
>
> I'd drop this check because only trace.c should use init_tracepoint()
> and you have ensured it uses correct arguments.  Just a coding style
> suggestion; having redundant checks makes the code more verbose, may
> lead the reader to assume that this function really is called with junk
> arguments, and silently returning will not help make the issue visible.

Ok.

> > +    trace_list[tevent].tp_name = (char*)qemu_malloc(strlen(tname)+1);
> > +    strncpy(trace_list[tevent].tp_name, tname, strlen(tname));
>
> Or use qemmu_strdup() but we don't really need to allocate memory at all
> here.  Just hold the const char* to a string literal since the trace
> event is a static object that is built into the binary.

Ok

> > +    trace_list[tevent].hash = qemu_hash(tname);
> > +    trace_list[tevent].state = 1; /* Enable all by default */
> > +}
> > ...
> > +void do_info_all_tracepoints(Monitor *mon)
> > +{
> > +    unsigned int i;
> > +
> > +    for (i=0; i<NR_TRACEPOINTS; i++) {
> > +        monitor_printf(mon, "%s [Event ID %u] : state %u\n",
> > +                                trace_list[i].tp_name, i, trace_list[i].state);
> > +        }
>
> Whitespace and indentation.

Will fix.

> > +}
> > +
> > +static Tracepoint* find_tracepoint_by_name(const char *tname)
> > +{
> > +    unsigned int i, name_hash;
> > +
> > +    if (!tname) {
> > +        return NULL;
> > +    }
> > +
> > +    name_hash = qemu_hash(tname);
> > +
> > +    for (i=0; i<NR_TRACEPOINTS; i++) {
> > +        if (trace_list[i].hash == name_hash&&
> > +                       !strncmp(trace_list[i].tp_name, tname, strlen(tname))) {
> > +            return&trace_list[i];
> > +        }
> > +    }
> > +    return NULL; /* indicates end of list reached without a match */
>
> I don't see the need for hashing.  We don't actually have a hash table
> and are doing a linear search.  There will be a smallish constant number
> of trace events and only change_tracepoint_by_name() needs to do a
> lookup.  There's no need to optimize that.

I was only optimising lookups. Instead of doing a strlen()-size 
comparison for each tracepoint, we end up doing a constant int-size 
comparison till there is possibility of hash collision. Strlen()-size 
lookups isnt really an issure right now, but will be more glaring if 
qemu ends up having a much larger number of tracepoints.

> Is strncmp() used so looking up "foo" is like searching for foo*?  The
> strlen(tname) should be outside the loop.  I think that could be useful
> but we should document it or just use strcmp().

The hash would take care of most such cases. But this might be an issue 
in the rare event of a hash collision happening when foo_1 and foo_2 
match as well. I'll fix this.

> > +}
> > +
> > +void change_tracepoint_state(const char *tname, bool tstate)
> > +{
> > +    Tracepoint *tp;
> > +
> > +    tp = find_tracepoint_by_name(tname);
> > +    if (tp) {
> > +        tp->state = tstate;
> > +    }
> > +}
> > diff --git a/tracetool b/tracetool
> > index 2c73bab..00af205 100755
> > --- a/tracetool
> > +++ b/tracetool
> > @@ -123,14 +123,20 @@ linetoc_end_nop()
> >   linetoh_begin_simple()
> >   {
> >       cat<<EOF
> > +#include<stdbool.h>
> > +
> >   typedef unsigned int TraceEvent;
> >
> > +void init_tracepoint_table(void);
> > +void init_tracepoint(const char *tname, TraceEvent tevent);
> >   void trace1(TraceEvent event, unsigned long x1);
> >   void trace2(TraceEvent event, unsigned long x1, unsigned long x2);
> >   void trace3(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3);
> >   void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3, unsigned long x4);
> >   void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned 
> > long x3, unsigned long x4, unsigned long x5);
> >   void do_info_trace(Monitor *mon);
> > +void do_info_all_tracepoints(Monitor *mon);
> > +void change_tracepoint_state(const char *tname, bool tstate);
> >   EOF
> >
> >       simple_event_num=0
> > @@ -163,22 +169,38 @@ EOF
> >
> >   linetoh_end_simple()
> >   {
> > -    return
> > +    cat<<EOF
> > +#define NR_TRACEPOINTS $simple_event_num
> > +EOF
> >   }
> >
> >   linetoc_begin_simple()
> >   {
> > -    return
> > +    cat<<EOF
> > +#include "trace.h"
> > +
> > +void init_tracepoint_table(void) {
> > +EOF
>
> Have you looked at module.h?  Perhaps that provides a good solution for
> initializing trace events.  It would allow the vl.c changes to be done
> without an #ifdef.

Thanks for the tip, I'll check.

> > +    simple_event_num=0
> > +
> >   }
> >
> >   linetoc_simple()
> >   {
> > -    return
> > +    local name
> > +    name=$(get_name "$1")
> > +    cat<<EOF
> > +init_tracepoint("$name", $simple_event_num);
> > +EOF
> > +    simple_event_num=$((simple_event_num + 1))
> >   }
> >
> >   linetoc_end_simple()
> >   {
> > -    return
> > +    cat<<EOF
> > +return;
>
> Please don't use return at the end of a void function.  Coding style.

Ok.

> > +}
> > +EOF
> >  ...

--
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India