Patchwork [1/2] trace: portable simple trace backend using glib

login
register
mail settings
Submitter Stefan Hajnoczi
Date Sept. 9, 2011, 9:37 a.m.
Message ID <1315561022-25386-2-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/114034/
State New
Headers show

Comments

Stefan Hajnoczi - Sept. 9, 2011, 9:37 a.m.
Convert the simple trace backend to glib so that it works under Windows.
We cannot use pthread directly but glib provides portable abstractions.
Also use glib atomics instead of newish gcc builtins which may not be
supported on Windows toolchains.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 trace/simple.c |   56 ++++++++++++++++++++++++++------------------------------
 1 files changed, 26 insertions(+), 30 deletions(-)
Jan Kiszka - Sept. 20, 2011, 10:31 a.m.
On 2011-09-09 11:37, Stefan Hajnoczi wrote:
> Convert the simple trace backend to glib so that it works under Windows.
> We cannot use pthread directly but glib provides portable abstractions.
> Also use glib atomics instead of newish gcc builtins which may not be
> supported on Windows toolchains.

Please avoid restrictive glib thread services. We have qemu_thread
abstractions that allow central tuning (will be needed e.g. to adjust
scheduling parameters).

I'm currently on the way to eliminate remaining pthread users and add
some missing bits to qemu_thread/cond.

Jan
Paolo Bonzini - Sept. 20, 2011, 10:52 a.m.
On 09/20/2011 12:31 PM, Jan Kiszka wrote:
> Please avoid restrictive glib thread services. We have qemu_thread
> abstractions that allow central tuning (will be needed e.g. to adjust
> scheduling parameters).

I think the rationale here was to allow tracing the qemu_thread 
routines.  For your application you can still use ust or systemtap backends.

Paolo
Jan Kiszka - Sept. 20, 2011, 10:58 a.m.
On 2011-09-20 12:52, Paolo Bonzini wrote:
> On 09/20/2011 12:31 PM, Jan Kiszka wrote:
>> Please avoid restrictive glib thread services. We have qemu_thread
>> abstractions that allow central tuning (will be needed e.g. to adjust
>> scheduling parameters).
> 
> I think the rationale here was to allow tracing the qemu_thread 
> routines.  For your application you can still use ust or systemtap backends.

OK, if that's a chick-egg thing, this makes sense. Should be documented
then (to avoid someone using this as a general example).

Jan
Stefan Hajnoczi - Sept. 20, 2011, 12:01 p.m.
On Tue, Sep 20, 2011 at 11:58 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-09-20 12:52, Paolo Bonzini wrote:
>> On 09/20/2011 12:31 PM, Jan Kiszka wrote:
>>> Please avoid restrictive glib thread services. We have qemu_thread
>>> abstractions that allow central tuning (will be needed e.g. to adjust
>>> scheduling parameters).
>>
>> I think the rationale here was to allow tracing the qemu_thread
>> routines.  For your application you can still use ust or systemtap backends.
>
> OK, if that's a chick-egg thing, this makes sense. Should be documented
> then (to avoid someone using this as a general example).

I'll add a comment.

Stefan

Patch

diff --git a/trace/simple.c b/trace/simple.c
index a609368..92c315a 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -12,8 +12,6 @@ 
 #include <stdint.h>
 #include <stdio.h>
 #include <time.h>
-#include <signal.h>
-#include <pthread.h>
 #include "qemu-timer.h"
 #include "trace.h"
 #include "trace/control.h"
@@ -54,9 +52,9 @@  enum {
  * Trace records are written out by a dedicated thread.  The thread waits for
  * records to become available, writes them out, and then waits again.
  */
-static pthread_mutex_t trace_lock = PTHREAD_MUTEX_INITIALIZER;
-static pthread_cond_t trace_available_cond = PTHREAD_COND_INITIALIZER;
-static pthread_cond_t trace_empty_cond = PTHREAD_COND_INITIALIZER;
+static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
+static GCond *trace_available_cond;
+static GCond *trace_empty_cond;
 static bool trace_available;
 static bool trace_writeout_enabled;
 
@@ -93,29 +91,30 @@  static bool get_trace_record(unsigned int idx, TraceRecord *record)
  */
 static void flush_trace_file(bool wait)
 {
-    pthread_mutex_lock(&trace_lock);
+    g_static_mutex_lock(&trace_lock);
     trace_available = true;
-    pthread_cond_signal(&trace_available_cond);
+    g_cond_signal(trace_available_cond);
 
     if (wait) {
-        pthread_cond_wait(&trace_empty_cond, &trace_lock);
+        g_cond_wait(trace_empty_cond, g_static_mutex_get_mutex(&trace_lock));
     }
 
-    pthread_mutex_unlock(&trace_lock);
+    g_static_mutex_unlock(&trace_lock);
 }
 
 static void wait_for_trace_records_available(void)
 {
-    pthread_mutex_lock(&trace_lock);
+    g_static_mutex_lock(&trace_lock);
     while (!(trace_available && trace_writeout_enabled)) {
-        pthread_cond_signal(&trace_empty_cond);
-        pthread_cond_wait(&trace_available_cond, &trace_lock);
+        g_cond_signal(trace_empty_cond);
+        g_cond_wait(trace_available_cond,
+                    g_static_mutex_get_mutex(&trace_lock));
     }
     trace_available = false;
-    pthread_mutex_unlock(&trace_lock);
+    g_static_mutex_unlock(&trace_lock);
 }
 
-static void *writeout_thread(void *opaque)
+static gpointer writeout_thread(gpointer opaque)
 {
     TraceRecord record;
     unsigned int writeout_idx = 0;
@@ -159,7 +158,7 @@  static void trace(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3,
 
     timestamp = get_clock();
 
-    idx = __sync_fetch_and_add(&trace_idx, 1) % TRACE_BUF_LEN;
+    idx = g_atomic_int_exchange_and_add((gint *)&trace_idx, 1) % TRACE_BUF_LEN;
     trace_buf[idx] = (TraceRecord){
         .event = event,
         .timestamp_ns = timestamp,
@@ -333,26 +332,23 @@  bool trace_event_set_state(const char *name, bool state)
 
 bool trace_backend_init(const char *events, const char *file)
 {
-    pthread_t thread;
-    pthread_attr_t attr;
-    sigset_t set, oldset;
-    int ret;
+    GThread *thread;
 
-    pthread_attr_init(&attr);
-    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
+    if (!g_thread_supported()) {
+        g_thread_init(NULL);
+    }
 
-    sigfillset(&set);
-    pthread_sigmask(SIG_SETMASK, &set, &oldset);
-    ret = pthread_create(&thread, &attr, writeout_thread, NULL);
-    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
+    trace_available_cond = g_cond_new();
+    trace_empty_cond = g_cond_new();
 
-    if (ret != 0) {
+    thread = g_thread_create(writeout_thread, NULL, FALSE, NULL);
+    if (!thread) {
         fprintf(stderr, "warning: unable to initialize simple trace backend\n");
-    } else {
-        atexit(st_flush_trace_buffer);
-        trace_backend_init_events(events);
-        st_set_trace_file(file);
+        return false;
     }
 
+    atexit(st_flush_trace_buffer);
+    trace_backend_init_events(events);
+    st_set_trace_file(file);
     return true;
 }