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

login
register
mail settings
Submitter Prerna Saxena
Date June 16, 2010, 12:42 p.m.
Message ID <20100616181206.65186bc4@zephyr>
Download mbox | patch
Permalink /patch/55881/
State New
Headers show

Comments

Prerna Saxena - June 16, 2010, 12:42 p.m.
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(-)
Stefan Hajnoczi - June 17, 2010, 3:08 p.m.
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
Prerna Saxena - June 18, 2010, 11:58 a.m.
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.
Stefan Hajnoczi - June 18, 2010, 12:34 p.m.
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

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 */"