[Qemu-devel] [PULL 0/4] Tracing patches

[Qemu-devel] [PULL 0/4] Tracing patches

[Stefan Hajnoczi]

The following changes since commit 0169c511554cb0014a00290b0d3d26c31a49818f:

  Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2014-01-24 15:52:44 -0800)

are available in the git repository at:


  git://github.com/stefanha/qemu.git tags/tracing-pull-request

for you to fetch changes up to 736ec1677f1ae7e64f2f3436ca3775c48f79678c:

  trace: fix simple trace "disable" keyword (2014-01-27 15:49:39 +0100)

----------------------------------------------------------------
Tracing pull request

----------------------------------------------------------------
Lluís Vilanova (1):
      trace: [simple] Do not include "trace/simple.h" in generated tracer headers

Michael Mueller (1):
      tracing: start trace processing thread in final child process

Stefan Hajnoczi (2):
      trace: add glib 2.32+ static GMutex support
      trace: fix simple trace "disable" keyword

 scripts/tracetool/backend/simple.py |  6 ++----
 trace/simple.c                      | 24 +++++++++++++++++-------
 vl.c                                | 12 ++++++++++--
 3 files changed, 29 insertions(+), 13 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PULL 1/4] tracing: start trace processing thread in final child process

[Stefan Hajnoczi]

From: Michael Mueller <mimu@linux.vnet.ibm.com>

When running with trace backend e.g. "simple" the writer thread needs to be
implemented in the same process context as the trace points that will be
processed. Under libvirtd control, qemu gets first started in daemonized
mode to privide its capabilities. Creating the writer thread in the initial
process context then leads to a dead lock because the thread gets termined
together with the initial parent. (-daemonize)

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[minor whitespace fixes]
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 vl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 7f4fe0d..63f4d55 100644
--- a/vl.c
+++ b/vl.c
@@ -3879,8 +3879,10 @@ int main(int argc, char **argv, char **envp)
         qemu_set_log(mask);
     }
 
-    if (!trace_backend_init(trace_events, trace_file)) {
-        exit(1);
+    if (!is_daemonized()) {
+        if (!trace_backend_init(trace_events, trace_file)) {
+            exit(1);
+        }
     }
 
     /* If no data_dir is specified then try to find it relative to the
@@ -4379,6 +4381,12 @@ int main(int argc, char **argv, char **envp)
 
     os_setup_post();
 
+    if (is_daemonized()) {
+        if (!trace_backend_init(trace_events, trace_file)) {
+            exit(1);
+        }
+    }
+
     main_loop();
     bdrv_close_all();
     pause_all_vcpus();
-- 
1.8.4.2

[Qemu-devel] [PULL 2/4] trace: [simple] Do not include "trace/simple.h" in generated tracer headers

[Stefan Hajnoczi]

From: Lluís Vilanova <vilanova@ac.upc.edu>

The header is not necessary, given that the simple backend does not define any
inlined tracing routines.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/backend/simple.py | 3 ---
 trace/simple.c                      | 1 +
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index 37ef599..30faac9 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -93,9 +93,6 @@ def c(events):
 
 
 def h(events):
-    out('#include "trace/simple.h"',
-        '')
-
     for event in events:
         out('void trace_%(name)s(%(args)s);',
             name = event.name,
diff --git a/trace/simple.c b/trace/simple.c
index 1e3f691..410172e 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -19,6 +19,7 @@
 #include "qemu/timer.h"
 #include "trace.h"
 #include "trace/control.h"
+#include "trace/simple.h"
 
 /** Trace file header event ID */
 #define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */
-- 
1.8.4.2

[Qemu-devel] [PULL 3/4] trace: add glib 2.32+ static GMutex support

[Stefan Hajnoczi]

The GStaticMutex API was deprecated in glib 2.32.  We cannot switch over
to GMutex unconditionally since we would drop support for older glib
versions.  But the deprecated API warnings during build are annoying so
use static GMutex when possible.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 trace/simple.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index 410172e..57572c4 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -40,7 +40,17 @@
  * Trace records are written out by a dedicated thread.  The thread waits for
  * records to become available, writes them out, and then waits again.
  */
+#if GLIB_CHECK_VERSION(2, 32, 0)
+static GMutex trace_lock;
+#define lock_trace_lock() g_mutex_lock(&trace_lock)
+#define unlock_trace_lock() g_mutex_unlock(&trace_lock)
+#define get_trace_lock_mutex() (&trace_lock)
+#else
 static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
+#define lock_trace_lock() g_static_mutex_lock(&trace_lock)
+#define unlock_trace_lock() g_static_mutex_unlock(&trace_lock)
+#define get_trace_lock_mutex() g_static_mutex_get_mutex(&trace_lock)
+#endif
 
 /* g_cond_new() was deprecated in glib 2.31 but we still need to support it */
 #if GLIB_CHECK_VERSION(2, 31, 0)
@@ -140,27 +150,26 @@ static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
  */
 static void flush_trace_file(bool wait)
 {
-    g_static_mutex_lock(&trace_lock);
+    lock_trace_lock();
     trace_available = true;
     g_cond_signal(trace_available_cond);
 
     if (wait) {
-        g_cond_wait(trace_empty_cond, g_static_mutex_get_mutex(&trace_lock));
+        g_cond_wait(trace_empty_cond, get_trace_lock_mutex());
     }
 
-    g_static_mutex_unlock(&trace_lock);
+    unlock_trace_lock();
 }
 
 static void wait_for_trace_records_available(void)
 {
-    g_static_mutex_lock(&trace_lock);
+    lock_trace_lock();
     while (!(trace_available && trace_writeout_enabled)) {
         g_cond_signal(trace_empty_cond);
-        g_cond_wait(trace_available_cond,
-                    g_static_mutex_get_mutex(&trace_lock));
+        g_cond_wait(trace_available_cond, get_trace_lock_mutex());
     }
     trace_available = false;
-    g_static_mutex_unlock(&trace_lock);
+    unlock_trace_lock();
 }
 
 static gpointer writeout_thread(gpointer opaque)
-- 
1.8.4.2

[Qemu-devel] [PULL 4/4] trace: fix simple trace "disable" keyword

[Stefan Hajnoczi]

The trace-events "disable" keyword turns an event into a nop at
compile-time.  This is important for high-frequency events that can
impact performance.

The "disable" keyword is currently broken in the simple trace backend.
This patch fixes the problem as follows:

Trace events are identified by their TraceEventID number.  When events
are disabled there are two options for assigning TraceEventID numbers:
1. Skip disabled events and don't assign them a number.
2. Assign numbers for all events regardless of the disabled keyword.

The simple trace backend and its binary file format uses approach #1.

The tracetool infrastructure has been using approach #2 for a while.

The result is that the numbers used in simple trace files do not
correspond with TraceEventIDs.  In trace/simple.c we assumed that they
are identical and therefore emitted bogus numbers.

This patch fixes the bug by using TraceEventID for trace_event_id()
while sticking to approach #1 for simple trace file numbers.  This
preserves simple trace file format compatibility.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/backend/simple.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index 30faac9..3dde372 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -56,7 +56,7 @@ def c(events):
 
 
         out('',
-            '    TraceEvent *eventp = trace_event_id(%(event_id)s);',
+            '    TraceEvent *eventp = trace_event_id(%(event_enum)s);',
             '    bool _state = trace_event_get_state_dynamic(eventp);',
             '    if (!_state) {',
             '        return;',
@@ -65,6 +65,7 @@ def c(events):
             '    if (trace_record_start(&rec, %(event_id)s, %(size_str)s)) {',
             '        return; /* Trace Buffer Full, Event Dropped ! */',
             '    }',
+            event_enum = 'TRACE_' + event.name.upper(),
             event_id = num,
             size_str = sizestr,
             )
-- 
1.8.4.2

Re: [Qemu-devel] [PULL 3/4] trace: add glib 2.32+ static GMutex support

[Daniel P. Berrange]

On Mon, Jan 27, 2014 at 03:53:05PM +0100, Stefan Hajnoczi wrote:
> The GStaticMutex API was deprecated in glib 2.32.  We cannot switch over
> to GMutex unconditionally since we would drop support for older glib
> versions.  But the deprecated API warnings during build are annoying so
> use static GMutex when possible.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  trace/simple.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/trace/simple.c b/trace/simple.c
> index 410172e..57572c4 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -40,7 +40,17 @@
>   * Trace records are written out by a dedicated thread.  The thread waits for
>   * records to become available, writes them out, and then waits again.
>   */
> +#if GLIB_CHECK_VERSION(2, 32, 0)
> +static GMutex trace_lock;
> +#define lock_trace_lock() g_mutex_lock(&trace_lock)
> +#define unlock_trace_lock() g_mutex_unlock(&trace_lock)
> +#define get_trace_lock_mutex() (&trace_lock)
> +#else
>  static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
> +#define lock_trace_lock() g_static_mutex_lock(&trace_lock)
> +#define unlock_trace_lock() g_static_mutex_unlock(&trace_lock)
> +#define get_trace_lock_mutex() g_static_mutex_get_mutex(&trace_lock)
> +#endif

coroutine-gthread.c also uses GStaticMutex - is there somewhere you
could put some compat calls tobe shared. Perhaps some hack like

  #define GStaticMutex GMutex
  #define g_static_mutex_lock(m) g_mutex_lock(m) 

?

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL 1/4] tracing: start trace processing thread in final child process

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On 01/27/2014 07:53 AM, Stefan Hajnoczi wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> When running with trace backend e.g. "simple" the writer thread needs to be
> implemented in the same process context as the trace points that will be
> processed. Under libvirtd control, qemu gets first started in daemonized
> mode to privide its capabilities. Creating the writer thread in the initial

s/privide/provide/

> process context then leads to a dead lock because the thread gets termined

s/dead lock/deadlock/
s/termined/terminated/

> together with the initial parent. (-daemonize)
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [minor whitespace fixes]
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PULL 3/4] trace: add glib 2.32+ static GMutex support

[Stefan Hajnoczi]

On Mon, Jan 27, 2014 at 03:15:47PM +0000, Daniel P. Berrange wrote:
> On Mon, Jan 27, 2014 at 03:53:05PM +0100, Stefan Hajnoczi wrote:
> > The GStaticMutex API was deprecated in glib 2.32.  We cannot switch over
> > to GMutex unconditionally since we would drop support for older glib
> > versions.  But the deprecated API warnings during build are annoying so
> > use static GMutex when possible.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  trace/simple.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/trace/simple.c b/trace/simple.c
> > index 410172e..57572c4 100644
> > --- a/trace/simple.c
> > +++ b/trace/simple.c
> > @@ -40,7 +40,17 @@
> >   * Trace records are written out by a dedicated thread.  The thread waits for
> >   * records to become available, writes them out, and then waits again.
> >   */
> > +#if GLIB_CHECK_VERSION(2, 32, 0)
> > +static GMutex trace_lock;
> > +#define lock_trace_lock() g_mutex_lock(&trace_lock)
> > +#define unlock_trace_lock() g_mutex_unlock(&trace_lock)
> > +#define get_trace_lock_mutex() (&trace_lock)
> > +#else
> >  static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
> > +#define lock_trace_lock() g_static_mutex_lock(&trace_lock)
> > +#define unlock_trace_lock() g_static_mutex_unlock(&trace_lock)
> > +#define get_trace_lock_mutex() g_static_mutex_get_mutex(&trace_lock)
> > +#endif
> 
> coroutine-gthread.c also uses GStaticMutex - is there somewhere you
> could put some compat calls tobe shared. Perhaps some hack like
> 
>   #define GStaticMutex GMutex
>   #define g_static_mutex_lock(m) g_mutex_lock(m) 
> 
> ?

I'll do a follow-up patch to pull together all the glib version
abstraction hacks we have in QEMU.

Stefan

Re: [Qemu-devel] [PULL 0/4] Tracing patches

[Peter Maydell]

On 27 January 2014 14:53, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit 0169c511554cb0014a00290b0d3d26c31a49818f:
>
>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2014-01-24 15:52:44 -0800)
>
> are available in the git repository at:
>
>
>   git://github.com/stefanha/qemu.git tags/tracing-pull-request
>
> for you to fetch changes up to 736ec1677f1ae7e64f2f3436ca3775c48f79678c:
>
>   trace: fix simple trace "disable" keyword (2014-01-27 15:49:39 +0100)
>
> ----------------------------------------------------------------
> Tracing pull request
>

Applied, thanks.

-- PMM