Patchwork [v11,0/7] trace: Generic event state description

login
register
mail settings
Submitter Stefan Hajnoczi
Date March 5, 2013, 10:33 a.m.
Message ID <20130305103357.GD1938@stefanha-thinkpad.redhat.com>
Download mbox | patch
Permalink /patch/224980/
State New
Headers show

Comments

Stefan Hajnoczi - March 5, 2013, 10:33 a.m.
On Mon, Mar 04, 2013 at 10:01:43PM +0100, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
> > On Fri, Mar 01, 2013 at 04:29:35PM +0100, Lluís Vilanova wrote:
> >> Provides a generic event state description structure (TraceEvent) and a more
> >> detailed event control and query interface.
> >> 
> >> This is achieved by creating a new "non-public" tracing backend (i.e., not
> >> selectable by the user at configure time) that will generate the appropriate
> >> event description information.
> >> 
> >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> >> ---
> >> 
> >> Changes in v11:
> >> 
> >> * Rebase on a4bcea3 from master.
> 
> > Hi Lluís,
> > Thanks for rebasing.  Unfortunately I still hit the hang on shutdown
> > with the simple trace backend.
> 
> > Steps to reproduce:
> > $ ./configure --target-list=x86_64-softmmu --enable-trace-backend=simple
> > $ make
> > $ cat my-events
> > bdrv_open_common
> > $ x86_64-softmmu/qemu-system-x86_64 \
> >       -enable-kvm -m 1024 \
> >       -trace events=my-events \
> >       -drive if=virtio,cache=none,file=test.img
> > Ctrl-Alt-2
> > (qemu) quit
> > ...hang...
> 
> > Please let me know if you are able to reproduce it.  qemu.git/master
> > does not behave this way.
> 
> I've been unable to reproduce it with v11, but was able to using v10 (although
> only a couple of times).

Found it!

The problem is that trace_record_start() is writing a 4-byte event field
instead of an 8-byte event field.  TraceEventID used to be typedef
uint64_t.  In your series TraceEventID becomes an enum and breaks
trace_record_start().

Try this:
Lluís Vilanova - March 5, 2013, 1 p.m.
Stefan Hajnoczi writes:

> On Mon, Mar 04, 2013 at 10:01:43PM +0100, Lluís Vilanova wrote:
>> Stefan Hajnoczi writes:
>> 
>> > On Fri, Mar 01, 2013 at 04:29:35PM +0100, Lluís Vilanova wrote:
>> >> Provides a generic event state description structure (TraceEvent) and a more
>> >> detailed event control and query interface.
>> >> 
>> >> This is achieved by creating a new "non-public" tracing backend (i.e., not
>> >> selectable by the user at configure time) that will generate the appropriate
>> >> event description information.
>> >> 
>> >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> >> ---
>> >> 
>> >> Changes in v11:
>> >> 
>> >> * Rebase on a4bcea3 from master.
>> 
>> > Hi Lluís,
>> > Thanks for rebasing.  Unfortunately I still hit the hang on shutdown
>> > with the simple trace backend.
>> 
>> > Steps to reproduce:
>> > $ ./configure --target-list=x86_64-softmmu --enable-trace-backend=simple
>> > $ make
>> > $ cat my-events
>> > bdrv_open_common
>> > $ x86_64-softmmu/qemu-system-x86_64 \
>> >       -enable-kvm -m 1024 \
>> >       -trace events=my-events \
>> >       -drive if=virtio,cache=none,file=test.img
>> > Ctrl-Alt-2
>> > (qemu) quit
>> > ...hang...
>> 
>> > Please let me know if you are able to reproduce it.  qemu.git/master
>> > does not behave this way.
>> 
>> I've been unable to reproduce it with v11, but was able to using v10 (although
>> only a couple of times).

> Found it!

> The problem is that trace_record_start() is writing a 4-byte event field
> instead of an 8-byte event field.  TraceEventID used to be typedef
> uint64_t.  In your series TraceEventID becomes an enum and breaks
> trace_record_start().

> Try this:

> diff --git a/trace/simple.c b/trace/simple.c
> index 5bb905c..1e3f691 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -218,6 +218,7 @@ int trace_record_start(TraceBufferRecord *rec, TraceEv
>  {
>      unsigned int idx, rec_off, old_idx, new_idx;
>      uint32_t rec_len = sizeof(TraceRecord) + datasize;
> +    uint64_t event_u64 = event;
>      uint64_t timestamp_ns = get_clock();
 
>      do {
> @@ -235,7 +236,7 @@ int trace_record_start(TraceBufferRecord *rec, TraceEv
>      idx = old_idx % TRACE_BUF_LEN;
 
>      rec_off = idx;
> -    rec_off = write_to_buffer(rec_off, &event, sizeof(event));
> +    rec_off = write_to_buffer(rec_off, &event_u64, sizeof(event_u64));
>      rec_off = write_to_buffer(rec_off, &timestamp_ns, sizeof(timestamp_ns
>      rec_off = write_to_buffer(rec_off, &rec_len, sizeof(rec_len));

Works for me, thx! I'll send v12 in a couple of minutes.


Lluis

Patch

diff --git a/trace/simple.c b/trace/simple.c
index 5bb905c..1e3f691 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -218,6 +218,7 @@  int trace_record_start(TraceBufferRecord *rec, TraceEv
 {
     unsigned int idx, rec_off, old_idx, new_idx;
     uint32_t rec_len = sizeof(TraceRecord) + datasize;
+    uint64_t event_u64 = event;
     uint64_t timestamp_ns = get_clock();
 
     do {
@@ -235,7 +236,7 @@  int trace_record_start(TraceBufferRecord *rec, TraceEv
     idx = old_idx % TRACE_BUF_LEN;
 
     rec_off = idx;
-    rec_off = write_to_buffer(rec_off, &event, sizeof(event));
+    rec_off = write_to_buffer(rec_off, &event_u64, sizeof(event_u64));
     rec_off = write_to_buffer(rec_off, &timestamp_ns, sizeof(timestamp_ns
     rec_off = write_to_buffer(rec_off, &rec_len, sizeof(rec_len));