[PATCH 0/3] Monitor support QEMU trace framework

[PATCH 0/3] Monitor support QEMU trace framework

[Prerna Saxena]

This is v3 of a set of patches to enable trace visualization & control 
via the QEMU monitor. It is based on trace infrastructure posted by 
Stefan :
( http://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02407.html )


This patchset adds monitor commands :
- info trace : to view current contents of the trace buffer
- info tracepoints : to view all available tracepoints and their state.
- tracepoint NAME on|off : to enable/disable the logging of data
 from tracepoint 'NAME' to on/off.

Changelog :
- Clean-ups from v2, particularly relating to export of tdb_hash()

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] [PATCH 0/3] Monitor support QEMU trace framework

[Prerna Saxena]

This is v3 of a set of patches to enable trace visualization & control 
via the QEMU monitor. It is based on trace infrastructure posted by 
Stefan :
( http://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02407.html )


This patchset adds monitor commands :
- info trace : to view current contents of the trace buffer
- info tracepoints : to view all available tracepoints and their state.
- tracepoint NAME on|off : to enable/disable the logging of data
 from tracepoint 'NAME' to on/off.

Changelog :
- Clean-ups from v2, particularly relating to export of tdb_hash()

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[PATCH 1/3] Export hash function

[Prerna Saxena]

Rename tdb_hash() to qemu_hash().
Move definition from qdict.c to a new file qemu-misc.c for use by tracing
infrastructure.

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---

 Makefile.objs |    2 +-
 qdict.c       |   24 ++++--------------------
 qemu-common.h |    3 +++
 qemu-misc.c   |   24 ++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 21 deletions(-)
 create mode 100644 qemu-misc.c


diff --git a/Makefile.objs b/Makefile.objs
index 7cb40ac..53e3a65 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -1,6 +1,6 @@
 #######################################################################
 # QObject
-qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
+qobject-obj-y = qint.o qstring.o qdict.o qemu-misc.o qlist.o qfloat.o qbool.o
 qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
 qobject-obj-y += qerror.o
 
diff --git a/qdict.c b/qdict.c
index 175bc17..d4588ef 100644
--- a/qdict.c
+++ b/qdict.c
@@ -53,22 +53,6 @@ QDict *qobject_to_qdict(const QObject *obj)
 }
 
 /**
- * tdb_hash(): based on the hash agorithm from gdbm, via tdb
- * (from module-init-tools)
- */
-static unsigned int tdb_hash(const char *name)
-{
-    unsigned value;	/* Used to compute the hash value.  */
-    unsigned   i;	/* Used to cycle through random values. */
-
-    /* Set the initial value from the key size. */
-    for (value = 0x238F13AF * strlen(name), i=0; name[i]; i++)
-        value = (value + (((const unsigned char *)name)[i] << (i*5 % 24)));
-
-    return (1103515243 * value + 12345);
-}
-
-/**
  * alloc_entry(): allocate a new QDictEntry
  */
 static QDictEntry *alloc_entry(const char *key, QObject *value)
@@ -113,7 +97,7 @@ void qdict_put_obj(QDict *qdict, const char *key, QObject *value)
     unsigned int hash;
     QDictEntry *entry;
 
-    hash = tdb_hash(key) % QDICT_HASH_SIZE;
+    hash = qemu_hash(key) % QDICT_HASH_SIZE;
     entry = qdict_find(qdict, key, hash);
     if (entry) {
         /* replace key's value */
@@ -137,7 +121,7 @@ QObject *qdict_get(const QDict *qdict, const char *key)
 {
     QDictEntry *entry;
 
-    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_HASH_SIZE);
+    entry = qdict_find(qdict, key, qemu_hash(key) % QDICT_HASH_SIZE);
     return (entry == NULL ? NULL : entry->value);
 }
 
@@ -148,7 +132,7 @@ QObject *qdict_get(const QDict *qdict, const char *key)
  */
 int qdict_haskey(const QDict *qdict, const char *key)
 {
-    unsigned int hash = tdb_hash(key) % QDICT_HASH_SIZE;
+    unsigned int hash = qemu_hash(key) % QDICT_HASH_SIZE;
     return (qdict_find(qdict, key, hash) == NULL ? 0 : 1);
 }
 
@@ -347,7 +331,7 @@ void qdict_del(QDict *qdict, const char *key)
 {
     QDictEntry *entry;
 
-    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_HASH_SIZE);
+    entry = qdict_find(qdict, key, qemu_hash(key) % QDICT_HASH_SIZE);
     if (entry) {
         QLIST_REMOVE(entry, next);
         qentry_destroy(entry);
diff --git a/qemu-common.h b/qemu-common.h
index a4888e5..d225e45 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -17,6 +17,9 @@ typedef struct QEMUTimer QEMUTimer;
 typedef struct QEMUFile QEMUFile;
 typedef struct QEMUBH QEMUBH;
 
+/* Hash function definition */
+unsigned int qemu_hash(const char *name);
+
 /* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN in files that
    cannot include the following headers without conflicts. This condition has
    to be removed once dyngen is gone. */
diff --git a/qemu-misc.c b/qemu-misc.c
new file mode 100644
index 0000000..a69196a
--- /dev/null
+++ b/qemu-misc.c
@@ -0,0 +1,24 @@
+/*
+ * Definition of tdb_hash() moved here, from qdict.c. Renamed to qemu_hash()
+ * 
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+#include "qemu-common.h"
+
+/**
+ * tdb_hash(): based on the hash agorithm from gdbm, via tdb
+ * (from module-init-tools). Renamed to qemu_hash(). 
+ */
+unsigned int qemu_hash(const char *name)
+{
+    unsigned value;     /* Used to compute the hash value.  */
+    unsigned   i;       /* Used to cycle through random values. */
+
+    /* Set the initial value from the key size. */
+    for (value = 0x238F13AF * strlen(name), i=0; name[i]; i++)
+        value = (value + (((const unsigned char *)name)[i] << (i*5 % 24)));
+
+    return (1103515243 * value + 12345);
+}


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] [PATCH 1/3] Export hash function

[Prerna Saxena]

Rename tdb_hash() to qemu_hash().
Move definition from qdict.c to a new file qemu-misc.c for use by tracing
infrastructure.

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---

 Makefile.objs |    2 +-
 qdict.c       |   24 ++++--------------------
 qemu-common.h |    3 +++
 qemu-misc.c   |   24 ++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 21 deletions(-)
 create mode 100644 qemu-misc.c


diff --git a/Makefile.objs b/Makefile.objs
index 7cb40ac..53e3a65 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -1,6 +1,6 @@
 #######################################################################
 # QObject
-qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
+qobject-obj-y = qint.o qstring.o qdict.o qemu-misc.o qlist.o qfloat.o qbool.o
 qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
 qobject-obj-y += qerror.o
 
diff --git a/qdict.c b/qdict.c
index 175bc17..d4588ef 100644
--- a/qdict.c
+++ b/qdict.c
@@ -53,22 +53,6 @@ QDict *qobject_to_qdict(const QObject *obj)
 }
 
 /**
- * tdb_hash(): based on the hash agorithm from gdbm, via tdb
- * (from module-init-tools)
- */
-static unsigned int tdb_hash(const char *name)
-{
-    unsigned value;	/* Used to compute the hash value.  */
-    unsigned   i;	/* Used to cycle through random values. */
-
-    /* Set the initial value from the key size. */
-    for (value = 0x238F13AF * strlen(name), i=0; name[i]; i++)
-        value = (value + (((const unsigned char *)name)[i] << (i*5 % 24)));
-
-    return (1103515243 * value + 12345);
-}
-
-/**
  * alloc_entry(): allocate a new QDictEntry
  */
 static QDictEntry *alloc_entry(const char *key, QObject *value)
@@ -113,7 +97,7 @@ void qdict_put_obj(QDict *qdict, const char *key, QObject *value)
     unsigned int hash;
     QDictEntry *entry;
 
-    hash = tdb_hash(key) % QDICT_HASH_SIZE;
+    hash = qemu_hash(key) % QDICT_HASH_SIZE;
     entry = qdict_find(qdict, key, hash);
     if (entry) {
         /* replace key's value */
@@ -137,7 +121,7 @@ QObject *qdict_get(const QDict *qdict, const char *key)
 {
     QDictEntry *entry;
 
-    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_HASH_SIZE);
+    entry = qdict_find(qdict, key, qemu_hash(key) % QDICT_HASH_SIZE);
     return (entry == NULL ? NULL : entry->value);
 }
 
@@ -148,7 +132,7 @@ QObject *qdict_get(const QDict *qdict, const char *key)
  */
 int qdict_haskey(const QDict *qdict, const char *key)
 {
-    unsigned int hash = tdb_hash(key) % QDICT_HASH_SIZE;
+    unsigned int hash = qemu_hash(key) % QDICT_HASH_SIZE;
     return (qdict_find(qdict, key, hash) == NULL ? 0 : 1);
 }
 
@@ -347,7 +331,7 @@ void qdict_del(QDict *qdict, const char *key)
 {
     QDictEntry *entry;
 
-    entry = qdict_find(qdict, key, tdb_hash(key) % QDICT_HASH_SIZE);
+    entry = qdict_find(qdict, key, qemu_hash(key) % QDICT_HASH_SIZE);
     if (entry) {
         QLIST_REMOVE(entry, next);
         qentry_destroy(entry);
diff --git a/qemu-common.h b/qemu-common.h
index a4888e5..d225e45 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -17,6 +17,9 @@ typedef struct QEMUTimer QEMUTimer;
 typedef struct QEMUFile QEMUFile;
 typedef struct QEMUBH QEMUBH;
 
+/* Hash function definition */
+unsigned int qemu_hash(const char *name);
+
 /* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN in files that
    cannot include the following headers without conflicts. This condition has
    to be removed once dyngen is gone. */
diff --git a/qemu-misc.c b/qemu-misc.c
new file mode 100644
index 0000000..a69196a
--- /dev/null
+++ b/qemu-misc.c
@@ -0,0 +1,24 @@
+/*
+ * Definition of tdb_hash() moved here, from qdict.c. Renamed to qemu_hash()
+ * 
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+#include "qemu-common.h"
+
+/**
+ * tdb_hash(): based on the hash agorithm from gdbm, via tdb
+ * (from module-init-tools). Renamed to qemu_hash(). 
+ */
+unsigned int qemu_hash(const char *name)
+{
+    unsigned value;     /* Used to compute the hash value.  */
+    unsigned   i;       /* Used to cycle through random values. */
+
+    /* Set the initial value from the key size. */
+    for (value = 0x238F13AF * strlen(name), i=0; name[i]; i++)
+        value = (value + (((const unsigned char *)name)[i] << (i*5 % 24)));
+
+    return (1103515243 * value + 12345);
+}


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[PATCH 2/3] Monitor command 'info trace'

[Prerna Saxena]

Monitor command 'info trace' to display contents of trace buffer

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---

 configure       |    3 +++
 monitor.c       |   12 ++++++++++++
 qemu-monitor.hx |    4 ++++
 simpletrace.c   |   13 +++++++++++++
 tracetool       |    2 ++
 5 files changed, 34 insertions(+), 0 deletions(-)


diff --git a/configure b/configure
index 675d0fc..56af8dd 100755
--- a/configure
+++ b/configure
@@ -2302,6 +2302,9 @@ bsd)
 esac
 
 echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
+if test "$trace_backend" = "simple"; then
+  echo "CONFIG_SIMPLE_TRACE=y" >> $config_host_mak
+fi
 if test "$trace_backend" = "ust"; then
   LIBS="-lust $LIBS"
 fi
diff --git a/monitor.c b/monitor.c
index ad50f12..8b60830 100644
--- a/monitor.c
+++ b/monitor.c
@@ -55,6 +55,9 @@
 #include "json-streamer.h"
 #include "json-parser.h"
 #include "osdep.h"
+#ifdef CONFIG_SIMPLE_TRACE
+#include "trace.h"
+#endif
 
 //#define DEBUG
 //#define DEBUG_COMPLETION
@@ -2780,6 +2783,15 @@ static const mon_cmd_t info_cmds[] = {
         .help       = "show roms",
         .mhandler.info = do_info_roms,
     },
+#if defined(CONFIG_SIMPLE_TRACE)
+    {
+        .name       = "trace",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show current contents of trace buffer",
+        .mhandler.info = do_info_trace,
+    },
+#endif
     {
         .name       = NULL,
     },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index b6e3467..766c30f 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -114,6 +114,10 @@ show migration status
 show balloon information
 @item info qtree
 show device tree
+#ifdef CONFIG_SIMPLE_TRACE
+@item info trace
+show contents of trace buffer
+#endif
 @end table
 ETEXI
 
diff --git a/simpletrace.c b/simpletrace.c
index 2fec4d3..239ae3f 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long
 void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5) {
     trace(event, x1, x2, x3, x4, x5);
 }
+
+void do_info_trace(Monitor *mon)
+{
+    unsigned int i, max_idx;
+
+    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
+
+    for (i=0; i<max_idx ;i++) {
+        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
+                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
+                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);
+    }
+}
diff --git a/tracetool b/tracetool
index 9ea9c08..2c73bab 100755
--- a/tracetool
+++ b/tracetool
@@ -130,6 +130,7 @@ 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);
 EOF
 
     simple_event_num=0
@@ -289,6 +290,7 @@ tracetoh()
 #define TRACE_H
 
 #include "qemu-common.h"
+#include "monitor.h"
 EOF
     convert h
     echo "#endif /* TRACE_H */"


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] [PATCH 2/3] Monitor command 'info trace'

[Prerna Saxena]

Monitor command 'info trace' to display contents of trace buffer

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---

 configure       |    3 +++
 monitor.c       |   12 ++++++++++++
 qemu-monitor.hx |    4 ++++
 simpletrace.c   |   13 +++++++++++++
 tracetool       |    2 ++
 5 files changed, 34 insertions(+), 0 deletions(-)


diff --git a/configure b/configure
index 675d0fc..56af8dd 100755
--- a/configure
+++ b/configure
@@ -2302,6 +2302,9 @@ bsd)
 esac
 
 echo "TRACE_BACKEND=$trace_backend" >> $config_host_mak
+if test "$trace_backend" = "simple"; then
+  echo "CONFIG_SIMPLE_TRACE=y" >> $config_host_mak
+fi
 if test "$trace_backend" = "ust"; then
   LIBS="-lust $LIBS"
 fi
diff --git a/monitor.c b/monitor.c
index ad50f12..8b60830 100644
--- a/monitor.c
+++ b/monitor.c
@@ -55,6 +55,9 @@
 #include "json-streamer.h"
 #include "json-parser.h"
 #include "osdep.h"
+#ifdef CONFIG_SIMPLE_TRACE
+#include "trace.h"
+#endif
 
 //#define DEBUG
 //#define DEBUG_COMPLETION
@@ -2780,6 +2783,15 @@ static const mon_cmd_t info_cmds[] = {
         .help       = "show roms",
         .mhandler.info = do_info_roms,
     },
+#if defined(CONFIG_SIMPLE_TRACE)
+    {
+        .name       = "trace",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show current contents of trace buffer",
+        .mhandler.info = do_info_trace,
+    },
+#endif
     {
         .name       = NULL,
     },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index b6e3467..766c30f 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -114,6 +114,10 @@ show migration status
 show balloon information
 @item info qtree
 show device tree
+#ifdef CONFIG_SIMPLE_TRACE
+@item info trace
+show contents of trace buffer
+#endif
 @end table
 ETEXI
 
diff --git a/simpletrace.c b/simpletrace.c
index 2fec4d3..239ae3f 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long
 void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5) {
     trace(event, x1, x2, x3, x4, x5);
 }
+
+void do_info_trace(Monitor *mon)
+{
+    unsigned int i, max_idx;
+
+    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
+
+    for (i=0; i<max_idx ;i++) {
+        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
+                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
+                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);
+    }
+}
diff --git a/tracetool b/tracetool
index 9ea9c08..2c73bab 100755
--- a/tracetool
+++ b/tracetool
@@ -130,6 +130,7 @@ 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);
 EOF
 
     simple_event_num=0
@@ -289,6 +290,7 @@ tracetoh()
 #define TRACE_H
 
 #include "qemu-common.h"
+#include "monitor.h"
 EOF
     convert h
     echo "#endif /* TRACE_H */"


-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

Re: [PATCH 2/3] Monitor command 'info trace'

[Stefan Hajnoczi]

On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
> diff --git a/simpletrace.c b/simpletrace.c
> index 2fec4d3..239ae3f 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long
>  void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5) {
>      trace(event, x1, x2, x3, x4, x5);
>  }
> +
> +void do_info_trace(Monitor *mon)
> +{
> +    unsigned int i, max_idx;
> +
> +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;

trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
to perform this test.

> +
> +    for (i=0; i<max_idx ;i++) {

Whitespace "i=0; i<max_idx ;i++".  "i = 0; i < max_idx; i++" is pretty
common across QEMU.

> +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
> +                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
> +                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);

Getting only numeric output is the limitation of a binary trace.  It
would probably be possible to pretty-print without much additional code
by using the format strings from the trace-events file.

I think the numeric dump is good for now though.  Hex is more compact
than decimal and would make pointers easier to spot.  Want to change
this?

> +    }
> +}
> diff --git a/tracetool b/tracetool
> index 9ea9c08..2c73bab 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -130,6 +130,7 @@ 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);
>  EOF
> 
>      simple_event_num=0
> @@ -289,6 +290,7 @@ tracetoh()
>  #define TRACE_H
> 
>  #include "qemu-common.h"
> +#include "monitor.h"

qemu-common.h forward-declares Monitor, I don't think you need
monitor.h.

Stefan

[Qemu-devel] Re: [PATCH 2/3] Monitor command 'info trace'

[Stefan Hajnoczi]

On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
> diff --git a/simpletrace.c b/simpletrace.c
> index 2fec4d3..239ae3f 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long
>  void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5) {
>      trace(event, x1, x2, x3, x4, x5);
>  }
> +
> +void do_info_trace(Monitor *mon)
> +{
> +    unsigned int i, max_idx;
> +
> +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;

trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
to perform this test.

> +
> +    for (i=0; i<max_idx ;i++) {

Whitespace "i=0; i<max_idx ;i++".  "i = 0; i < max_idx; i++" is pretty
common across QEMU.

> +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
> +                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
> +                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);

Getting only numeric output is the limitation of a binary trace.  It
would probably be possible to pretty-print without much additional code
by using the format strings from the trace-events file.

I think the numeric dump is good for now though.  Hex is more compact
than decimal and would make pointers easier to spot.  Want to change
this?

> +    }
> +}
> diff --git a/tracetool b/tracetool
> index 9ea9c08..2c73bab 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -130,6 +130,7 @@ 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);
>  EOF
> 
>      simple_event_num=0
> @@ -289,6 +290,7 @@ tracetoh()
>  #define TRACE_H
> 
>  #include "qemu-common.h"
> +#include "monitor.h"

qemu-common.h forward-declares Monitor, I don't think you need
monitor.h.

Stefan

Re: [PATCH 2/3] Monitor command 'info trace'

[Prerna Saxena]

Hi Stefan, Jan,
Thanks for taking a look.

On 06/17/2010 08:38 PM, Stefan Hajnoczi wrote:
> On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
>> diff --git a/simpletrace.c b/simpletrace.c
>> index 2fec4d3..239ae3f 100644
>> --- a/simpletrace.c
>> +++ b/simpletrace.c
>> @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long
>>   void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5) {
>>       trace(event, x1, x2, x3, x4, x5);
>>   }
>> +
>> +void do_info_trace(Monitor *mon)
>> +{
>> +    unsigned int i, max_idx;
>> +
>> +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
>
> trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
> to perform this test.

I only display the logged contents in the trace buffer (till trace_idx) 
, and not the entire trace buffer. Only when the index is full that the 
entire buffer is displayed.

>
>> +
>> +    for (i=0; i<max_idx ;i++) {
>
> Whitespace "i=0; i<max_idx ;i++".  "i = 0; i<  max_idx; i++" is pretty
> common across QEMU.
>

I'll fix this.

>> +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
>> +                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
>> +                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);
>
> Getting only numeric output is the limitation of a binary trace.  It
> would probably be possible to pretty-print without much additional code
> by using the format strings from the trace-events file.
>
> I think the numeric dump is good for now though.  Hex is more compact
> than decimal and would make pointers easier to spot.  Want to change
> this?
>

I agree, but we can let this be a todo till after the first prototype 
goes upstream ?

>> +    }
>> +}
>> diff --git a/tracetool b/tracetool
>> index 9ea9c08..2c73bab 100755
>> --- a/tracetool
>> +++ b/tracetool
>> @@ -130,6 +130,7 @@ 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);
>>   EOF
>>
>>       simple_event_num=0
>> @@ -289,6 +290,7 @@ tracetoh()
>>   #define TRACE_H
>>
>>   #include "qemu-common.h"
>> +#include "monitor.h"
>
> qemu-common.h forward-declares Monitor, I don't think you need
> monitor.h.
>

Right.

> Stefan

I'll post patches by Monday that addresses your suggestions, and try to 
get it integrated with QMP.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] Re: [PATCH 2/3] Monitor command 'info trace'

[Prerna Saxena]

Hi Stefan, Jan,
Thanks for taking a look.

On 06/17/2010 08:38 PM, Stefan Hajnoczi wrote:
> On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
>> diff --git a/simpletrace.c b/simpletrace.c
>> index 2fec4d3..239ae3f 100644
>> --- a/simpletrace.c
>> +++ b/simpletrace.c
>> @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long
>>   void trace5(TraceEvent event, unsigned long x1, unsigned long x2, unsigned long x3, unsigned long x4, unsigned long x5) {
>>       trace(event, x1, x2, x3, x4, x5);
>>   }
>> +
>> +void do_info_trace(Monitor *mon)
>> +{
>> +    unsigned int i, max_idx;
>> +
>> +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
>
> trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
> to perform this test.

I only display the logged contents in the trace buffer (till trace_idx) 
, and not the entire trace buffer. Only when the index is full that the 
entire buffer is displayed.

>
>> +
>> +    for (i=0; i<max_idx ;i++) {
>
> Whitespace "i=0; i<max_idx ;i++".  "i = 0; i<  max_idx; i++" is pretty
> common across QEMU.
>

I'll fix this.

>> +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
>> +                          trace_buf[i].event, trace_buf[i].x1, trace_buf[i].x2,
>> +                            trace_buf[i].x3, trace_buf[i].x4, trace_buf[i].x5);
>
> Getting only numeric output is the limitation of a binary trace.  It
> would probably be possible to pretty-print without much additional code
> by using the format strings from the trace-events file.
>
> I think the numeric dump is good for now though.  Hex is more compact
> than decimal and would make pointers easier to spot.  Want to change
> this?
>

I agree, but we can let this be a todo till after the first prototype 
goes upstream ?

>> +    }
>> +}
>> diff --git a/tracetool b/tracetool
>> index 9ea9c08..2c73bab 100755
>> --- a/tracetool
>> +++ b/tracetool
>> @@ -130,6 +130,7 @@ 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);
>>   EOF
>>
>>       simple_event_num=0
>> @@ -289,6 +290,7 @@ tracetoh()
>>   #define TRACE_H
>>
>>   #include "qemu-common.h"
>> +#include "monitor.h"
>
> qemu-common.h forward-declares Monitor, I don't think you need
> monitor.h.
>

Right.

> Stefan

I'll post patches by Monday that addresses your suggestions, and try to 
get it integrated with QMP.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

Re: [PATCH 2/3] Monitor command 'info trace'

[Stefan Hajnoczi]

On Fri, Jun 18, 2010 at 12:58 PM, Prerna Saxena
<prerna@linux.vnet.ibm.com> wrote:
> Hi Stefan, Jan,
> Thanks for taking a look.
>
> On 06/17/2010 08:38 PM, Stefan Hajnoczi wrote:
>>
>> On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
>>>
>>> diff --git a/simpletrace.c b/simpletrace.c
>>> index 2fec4d3..239ae3f 100644
>>> --- a/simpletrace.c
>>> +++ b/simpletrace.c
>>> @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1,
>>> unsigned long x2, unsigned long
>>>  void trace5(TraceEvent event, unsigned long x1, unsigned long x2,
>>> unsigned long x3, unsigned long x4, unsigned long x5) {
>>>      trace(event, x1, x2, x3, x4, x5);
>>>  }
>>> +
>>> +void do_info_trace(Monitor *mon)
>>> +{
>>> +    unsigned int i, max_idx;
>>> +
>>> +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
>>
>> trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
>> to perform this test.
>
> I only display the logged contents in the trace buffer (till trace_idx) ,
> and not the entire trace buffer. Only when the index is full that the entire
> buffer is displayed.

Thanks for explaining, I understand what you are doing now.  Due to
this special case, the code will dump out the empty trace buffer if
used before anything has been traced (trace_idx=0).

>>> +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
>>> +                          trace_buf[i].event, trace_buf[i].x1,
>>> trace_buf[i].x2,
>>> +                            trace_buf[i].x3, trace_buf[i].x4,
>>> trace_buf[i].x5);
>>
>> Getting only numeric output is the limitation of a binary trace.  It
>> would probably be possible to pretty-print without much additional code
>> by using the format strings from the trace-events file.
>>
>> I think the numeric dump is good for now though.  Hex is more compact
>> than decimal and would make pointers easier to spot.  Want to change
>> this?
>>
>
> I agree, but we can let this be a todo till after the first prototype goes
> upstream ?

I still vote for hex instead of decimal :).  Since you're already
spinning a new patch it would be nice to put that change in, but no
worries.

> I'll post patches by Monday that addresses your suggestions, and try to get
> it integrated with QMP.

Excellent, thanks.  I'd like to put your patches onto my tracing
branch soon and test out the overall workflow of tracing QEMU.

Stefan

[Qemu-devel] Re: [PATCH 2/3] Monitor command 'info trace'

[Stefan Hajnoczi]

On Fri, Jun 18, 2010 at 12:58 PM, Prerna Saxena
<prerna@linux.vnet.ibm.com> wrote:
> Hi Stefan, Jan,
> Thanks for taking a look.
>
> On 06/17/2010 08:38 PM, Stefan Hajnoczi wrote:
>>
>> On Wed, Jun 16, 2010 at 06:12:06PM +0530, Prerna Saxena wrote:
>>>
>>> diff --git a/simpletrace.c b/simpletrace.c
>>> index 2fec4d3..239ae3f 100644
>>> --- a/simpletrace.c
>>> +++ b/simpletrace.c
>>> @@ -62,3 +62,16 @@ void trace4(TraceEvent event, unsigned long x1,
>>> unsigned long x2, unsigned long
>>>  void trace5(TraceEvent event, unsigned long x1, unsigned long x2,
>>> unsigned long x3, unsigned long x4, unsigned long x5) {
>>>      trace(event, x1, x2, x3, x4, x5);
>>>  }
>>> +
>>> +void do_info_trace(Monitor *mon)
>>> +{
>>> +    unsigned int i, max_idx;
>>> +
>>> +    max_idx = trace_idx ? trace_idx : TRACE_BUF_LEN;
>>
>> trace_idx is always in the range [0, TRACE_BUF_LEN).  There is no need
>> to perform this test.
>
> I only display the logged contents in the trace buffer (till trace_idx) ,
> and not the entire trace buffer. Only when the index is full that the entire
> buffer is displayed.

Thanks for explaining, I understand what you are doing now.  Due to
this special case, the code will dump out the empty trace buffer if
used before anything has been traced (trace_idx=0).

>>> +        monitor_printf(mon, "Event %ld : %ld %ld %ld %ld %ld\n",
>>> +                          trace_buf[i].event, trace_buf[i].x1,
>>> trace_buf[i].x2,
>>> +                            trace_buf[i].x3, trace_buf[i].x4,
>>> trace_buf[i].x5);
>>
>> Getting only numeric output is the limitation of a binary trace.  It
>> would probably be possible to pretty-print without much additional code
>> by using the format strings from the trace-events file.
>>
>> I think the numeric dump is good for now though.  Hex is more compact
>> than decimal and would make pointers easier to spot.  Want to change
>> this?
>>
>
> I agree, but we can let this be a todo till after the first prototype goes
> upstream ?

I still vote for hex instead of decimal :).  Since you're already
spinning a new patch it would be nice to put that change in, but no
worries.

> I'll post patches by Monday that addresses your suggestions, and try to get
> it integrated with QMP.

Excellent, thanks.  I'd like to put your patches onto my tracing
branch soon and test out the overall workflow of tracing QEMU.

Stefan

[PATCH 3/3] Toggle tracepoint state

[Prerna Saxena]

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@linux.vnet.ibm.com>
---

 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;
+
+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;
+    }
+
+    trace_list[tevent].tp_name = (char*)qemu_malloc(strlen(tname)+1);
+    strncpy(trace_list[tevent].tp_name, tname, strlen(tname));
+    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);
+        }
+}
+
+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 */
+}
+
+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
+    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;
+}
+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

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

[Prerna Saxena]

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@linux.vnet.ibm.com>
---

 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;
+
+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;
+    }
+
+    trace_list[tevent].tp_name = (char*)qemu_malloc(strlen(tname)+1);
+    strncpy(trace_list[tevent].tp_name, tname, strlen(tname));
+    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);
+        }
+}
+
+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 */
+}
+
+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
+    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;
+}
+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

Re: [PATCH 3/3] Toggle tracepoint state

[Stefan Hajnoczi]

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@linux.vnet.ibm.com>
> ---
> 
>  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
> 

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

[Stefan Hajnoczi]

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@linux.vnet.ibm.com>
> ---
> 
>  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
> 

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

[Prerna Saxena]

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

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

[Stefan Hajnoczi]

On Fri, Jun 18, 2010 at 1:24 PM, Prerna Saxena
<prerna@linux.vnet.ibm.com> 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

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

[Stefan Hajnoczi]

On Fri, Jun 18, 2010 at 1:24 PM, Prerna Saxena
<prerna@linux.vnet.ibm.com> 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

Re: [PATCH 0/3] Monitor support QEMU trace framework

[Jan Kiszka]

Prerna Saxena wrote:
> This is v3 of a set of patches to enable trace visualization & control 
> via the QEMU monitor. It is based on trace infrastructure posted by 
> Stefan :
> ( http://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02407.html )
> 
> 
> This patchset adds monitor commands :
> - info trace : to view current contents of the trace buffer
> - info tracepoints : to view all available tracepoints and their state.
> - tracepoint NAME on|off : to enable/disable the logging of data
>  from tracepoint 'NAME' to on/off.
> 
> Changelog :
> - Clean-ups from v2, particularly relating to export of tdb_hash()
> 

But it's still using the legacy monitor command interface. See [1]+[2]
for a mechanism that allows you to convert to the new interface without
automatically exposing your commands via QMP.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/74356
[2] http://thread.gmane.org/gmane.comp.emulators.qemu/74372

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 0/3] Monitor support QEMU trace framework

[Jan Kiszka]

Prerna Saxena wrote:
> This is v3 of a set of patches to enable trace visualization & control 
> via the QEMU monitor. It is based on trace infrastructure posted by 
> Stefan :
> ( http://lists.gnu.org/archive/html/qemu-devel/2010-05/msg02407.html )
> 
> 
> This patchset adds monitor commands :
> - info trace : to view current contents of the trace buffer
> - info tracepoints : to view all available tracepoints and their state.
> - tracepoint NAME on|off : to enable/disable the logging of data
>  from tracepoint 'NAME' to on/off.
> 
> Changelog :
> - Clean-ups from v2, particularly relating to export of tdb_hash()
> 

But it's still using the legacy monitor command interface. See [1]+[2]
for a mechanism that allows you to convert to the new interface without
automatically exposing your commands via QMP.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/74356
[2] http://thread.gmane.org/gmane.comp.emulators.qemu/74372

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux