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.
>
> Monitor commands added :
> 1) info tracepoints : to view all available tracepoints and
> their state.
> 2) tracepoint NAME on|off : to enable/disable data logging from a
> given tracepoint.
> Eg, tracepoint paio_submit off
> disables logging of data when
> paio_submit is hit.
>
> Signed-off-by: Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>
> ---
>
> monitor.c | 16 ++++++++++++++
> qemu-monitor.hx | 18 ++++++++++++++++
> simpletrace.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tracetool | 30 +++++++++++++++++++++++---
> vl.c | 6 +++++
> 5 files changed, 129 insertions(+), 4 deletions(-)
>
>
> diff --git a/monitor.c b/monitor.c
> index 8b60830..238bdf0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -548,6 +548,15 @@ static void do_commit(Monitor *mon, const QDict *qdict)
> }
> }
>
> +#ifdef CONFIG_SIMPLE_TRACE
> +static void do_change_tracepoint_state(Monitor *mon, const QDict *qdict)
> +{
> + const char *tp_name = qdict_get_str(qdict, "name");
> + bool new_state = qdict_get_bool(qdict, "option");
> + change_tracepoint_state(tp_name, new_state);
> +}
> +#endif
> +
> static void user_monitor_complete(void *opaque, QObject *ret_data)
> {
> MonitorCompletionData *data = (MonitorCompletionData *)opaque;
> @@ -2791,6 +2800,13 @@ static const mon_cmd_t info_cmds[] = {
> .help = "show current contents of trace buffer",
> .mhandler.info = do_info_trace,
> },
> + {
> + .name = "tracepoints",
> + .args_type = "",
> + .params = "",
> + .help = "show available tracepoints & their state",
> + .mhandler.info = do_info_all_tracepoints,
> + },
> #endif
> {
> .name = NULL,
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 766c30f..8540b8f 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -117,6 +117,8 @@ show device tree
> #ifdef CONFIG_SIMPLE_TRACE
> @item info trace
> show contents of trace buffer
> +@item info tracepoints
> +show available tracepoints and their state
> #endif
> @end table
> ETEXI
> @@ -225,6 +227,22 @@ STEXI
> @item logfile @var{filename}
> @findex logfile
> Output logs to @var{filename}.
> +#ifdef CONFIG_SIMPLE_TRACE
> +ETEXI
> +
> + {
> + .name = "tracepoint",
> + .args_type = "name:s,option:b",
> + .params = "name on|off",
> + .help = "changes status of a specific tracepoint",
> + .mhandler.cmd = do_change_tracepoint_state,
> + },
> +
> +STEXI
> +@item tracepoint
> +@findex tracepoint
> +changes status of a tracepoint
> +#endif
> ETEXI
>
> {
> diff --git a/simpletrace.c b/simpletrace.c
> index 239ae3f..4221a8f 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -3,6 +3,12 @@
> #include "trace.h"
>
> 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.
> +
> +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.
> + 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.
> + trace_list[tevent].hash = qemu_hash(tname);
> + trace_list[tevent].state = 1; /* Enable all by default */
> +}
>
> static void trace(TraceEvent event, unsigned long x1,
> unsigned long x2, unsigned long x3,
> unsigned long x4, unsigned long x5) {
> TraceRecord *rec = &trace_buf[trace_idx];
> +
> + if (!trace_list[event].state) {
> + return;
> + }
> +
> rec->event = event;
> rec->x1 = x1;
> rec->x2 = x2;
> @@ -75,3 +99,42 @@ void do_info_trace(Monitor *mon)
> trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);
> }
> }
> +
> +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.
> +}
> +
> +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.
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().
> +}
> +
> +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.
> + 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.
> +}
> +EOF
> }
>
> linetoh_begin_ust()
> diff --git a/vl.c b/vl.c
> index 328395e..dd07904 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -163,6 +163,9 @@ int main(int argc, char **argv)
> #include "cpus.h"
> #include "arch_init.h"
>
> +#ifdef CONFIG_SIMPLE_TRACE
> +#include "trace.h"
> +#endif
> //#define DEBUG_NET
> //#define DEBUG_SLIRP
>
> @@ -3604,6 +3607,9 @@ int main(int argc, char **argv, char **envp)
> if (net_init_clients() < 0) {
> exit(1);
> }
> +#ifdef CONFIG_SIMPLE_TRACE
> + init_tracepoint_table();
> +#endif
>
> /* init the bluetooth world */
> if (foreach_device_config(DEV_BT, bt_parse))
>
>
> --
> Prerna Saxena
>
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
