From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39976 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OPacV-0003Ly-VF for qemu-devel@nongnu.org; Fri, 18 Jun 2010 08:24:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OPacU-0005Jn-Iz for qemu-devel@nongnu.org; Fri, 18 Jun 2010 08:24:27 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:36258) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OPacT-0005JQ-Nn for qemu-devel@nongnu.org; Fri, 18 Jun 2010 08:24:26 -0400 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [202.81.31.245]) by e23smtp07.au.ibm.com (8.14.4/8.13.1) with ESMTP id o5ICOQq8032395 for ; Fri, 18 Jun 2010 22:24:26 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o5ICOLIW819254 for ; Fri, 18 Jun 2010 22:24:22 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o5ICOL8m005557 for ; Fri, 18 Jun 2010 22:24:21 +1000 Message-ID: <4C1B6573.5050705@linux.vnet.ibm.com> Date: Fri, 18 Jun 2010 17:54:19 +0530 From: Prerna Saxena MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH 3/3] Toggle tracepoint state References: <20100616180542.474535e3@zephyr> <20100616181435.7e239947@zephyr> <20100617160358.GB15845@stefan-thinkpad.transitives.com> In-Reply-To: <20100617160358.GB15845@stefan-thinkpad.transitives.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Anthony Liguori , kvm@vger.kernel.org, qemu-devel@nongnu.org, Luiz Capitulino , Maneesh Soni , Ananth 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> + 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> + 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<> +#include >> + >> 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<> +#define NR_TRACEPOINTS $simple_event_num >> +EOF >> } >> >> linetoc_begin_simple() >> { >> - return >> + cat<> +#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<> +init_tracepoint("$name", $simple_event_num); >> +EOF >> + simple_event_num=$((simple_event_num + 1)) >> } >> >> linetoc_end_simple() >> { >> - return >> + cat<> +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