Re: [Qemu-devel] [PATCH] simpletrace: Thread-safe tracing

[Alexander Graf]

On 23.03.2011, at 08:59, Stefan Hajnoczi wrote:

> On Wed, Mar 23, 2011 at 7:58 AM, Stefan Hajnoczi <address@hidden> wrote:
>> On Wed, Mar 23, 2011 at 7:39 AM, Stefan Hajnoczi <address@hidden> wrote:
>>> On Tue, Mar 22, 2011 at 11:52 PM, Andreas Färber <address@hidden> wrote:
>>>> Am 28.02.2011 um 10:38 schrieb Stefan Hajnoczi:
>>>> 
>>>>> Trace events outside the global mutex cannot be used with the simple
>>>>> trace backend since it is not thread-safe.  There is no check to prevent
>>>>> them being enabled so people sometimes learn this the hard way.
>>>>> 
>>>>> This patch restructures the simple trace backend with a ring buffer
>>>>> suitable for multiple concurrent writers.  A writeout thread empties the
>>>>> trace buffer when threshold fill levels are reached.  Should the
>>>>> writeout thread be unable to keep up with trace generation, records will
>>>>> simply be dropped.
>>>>> 
>>>>> Each time events are dropped a special record is written to the trace
>>>>> file indicating how many events were dropped.  The event ID is
>>>>> 0xfffffffffffffffe and its signature is dropped(uint32_t count).
>>>>> 
>>>>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>>>>> ---
>>>>> v2:
>>>>> * Add 'dropped' event so we know when events were lost.
>>>> 
>>>> [...]
>>>>> 
>>>>> +    __sync_synchronize(); /* read memory barrier before accessing record
>>>>> */
>>>> 
>>>> Getting this at HEAD on Darwin/ppc64:
>>>> 
>>>>  CC    simpletrace.o
>>>> /Users/andreas/QEMU/qemu/simpletrace.c: In function ‘get_trace_record’:
>>>> /Users/andreas/QEMU/qemu/simpletrace.c:81: warning: implicit declaration of
>>>> function ‘__sync_synchronize’
>>>> /Users/andreas/QEMU/qemu/simpletrace.c:81: warning: nested extern
>>>> declaration of ‘__sync_synchronize’
>>>> /Users/andreas/QEMU/qemu/simpletrace.c: In function ‘trace’:
>>>> /Users/andreas/QEMU/qemu/simpletrace.c:161: warning: implicit declaration 
>>>> of
>>>> function ‘__sync_fetch_and_add’
>>>> /Users/andreas/QEMU/qemu/simpletrace.c:161: warning: nested extern
>>>> declaration of ‘__sync_fetch_and_add’
>>>> [...]
>>>>  LINK  qemu-nbd
>>>> Undefined symbols:
>>>>  "___sync_fetch_and_add", referenced from:
>>>>      _trace in simpletrace.o
>>>>  "___sync_synchronize", referenced from:
>>>>      _get_trace_record in simpletrace.o
>>>>      _trace in simpletrace.o
>>>> ld: symbol(s) not found
>>>> collect2: ld returned 1 exit status
>>>> make: *** [qemu-nbd] Error 1
>>>> 
>>>> Haven't investigated further yet.
>>> 
>>> /me shakes his fist at Apple gcc!
>>> 
>>> These are gcc builtins, I believe the were added in gcc 4.1:
>>> http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html#Atomic-Builtins
>>> 
>>> Which version of gcc are you running?
>>> 
>>> We can replace them with equivalent library functions or inline
>>> assembly code.  Here's what we need:
>>> Read memory barrier
>>> Write memory barrier
>>> Atomic load and increment
>>> 
>>> CCed Alex and Anthony who may have thoughts on adding these atomic ops to 
>>> QEMU.
>> 
>> Thinking about it more, the way I'd like to solve this (and make
>> simpletrace work on Windows too!) is to go ahead and use glib threads
>> and atomics.  I don't want to be in the business of writing
>> portability wrappers for different OSes and architectures, and glib
>> already does this:
>> file:///usr/share/doc/libglib2.0-doc/glib/glib-Atomic-Operations.html#g-atomic-int-exchange-and-add
> 
> Corrected URI:
> http://library.gnome.org/devel/glib/2.28/glib-Atomic-Operations.html#g-atomic-int-exchange-and-add

Yeah, either that or adding a configure check for the availability of atomic 
operations. If the glib folks did go through the work already, I agree that 
it'd be nice to reuse that work though.


Alex