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

Prerna Saxena
2010-06-18T12:24:24Z

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