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

[Stefan Hajnoczi]

On Fri, Jun 18, 2010 at 1:24 PM, Prerna Saxena
<address@hidden> wrote:
> On 06/17/2010 09:33 PM, Stefan Hajnoczi wrote:
>>> +}
>>> +
>>> +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.

The thing is we only need to do lookups when tracepoints are
enabled/disabled.  That is a very rare operation, not critical path.

This feels like premature optimization.  If there is a performance
bottleneck down the road we can switch to a data structure that has
better time complexity than an array.  This is an easy change to make
later because it will not affect the user interface.

In the meantime linear strcmp() over an array keeps the code simple.
We can drop the tdb_hash() patch and remove some lines from this
patch.

>>> 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.

I also wonder if it's possible to statically initialize the TraceEvent
array in the generated trace.c.  That way we need no vl.c startup
magic and tracing is available even in the very early stages of QEMU
startup.  What do you think?

Stefan