| Message ID | 20100616181206.65186bc4@zephyr |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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 */"
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(-)