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

[Prerna Saxena <prerna@linux.vnet.ibm.com>]

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