Re: [Qemu-devel] [PATCH 1/4] trace: Fix simple trace dropped event record for big endian

[Harsh Bora]

On 01/25/2013 09:13 PM, Markus Armbruster wrote:
> We use atomic operations to keep track of dropped events.
>
> Inconveniently, GLib supports only int and void * atomics, but the
> counter dropped_events is uint64_t.  Can't stop commit 62bab732: a
> quick (gint *)&dropped_events bludgeons the compiler into submission.
>
> That cast is okay only when int is exactly 64 bits wide, which it
> commonly isn't.
>
> If int is even wider, we clobber whatever follows dropped_events.  Not
> worth worrying about, as none of the machines that interest us have
> such morbidly obese ints.
>
> That leaves the common case: int narrower than 64 bits.
>
> Harmless on little endian hosts: we just don't access the most
> significant bits of dropped_events.  They remain zero.
>
> On big endian hosts, we use only the most significant bits of
> dropped_events as counter.  The least significant bits remain zero.
> However, we write out the full value, which is the correct counter
> shifted left a bunch of places.
>
> Fix by changing the variables involved to int.
>
> There's another, equally suspicious-looking (gint *)&trace_idx
> argument to g_atomic_int_compare_and_exchange(), but that one casts
> unsigned *, so it's okay.  But it's also superfluous, because GLib's
> atomic int operations work just fine for unsigned.  Drop it.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>   trace/simple.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/trace/simple.c b/trace/simple.c
> index ce17d64..ccbdb6a 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -53,7 +53,7 @@ enum {
>   uint8_t trace_buf[TRACE_BUF_LEN];
>   static unsigned int trace_idx;
>   static unsigned int writeout_idx;
> -static uint64_t dropped_events;
> +static int dropped_events;
>   static FILE *trace_fp;
>   static char *trace_file_name;
>
> @@ -63,7 +63,7 @@ typedef struct {
>       uint64_t timestamp_ns;
>       uint32_t length;   /*    in bytes */
>       uint32_t reserved; /*    unused */
> -    uint8_t arguments[];
> +    uint64_t arguments[];

I am just paranoid about this change, however if the patch has gone 
through a stress testing, I have no objections.

>   } TraceRecord;
>
>   typedef struct {
> @@ -160,7 +160,7 @@ static gpointer writeout_thread(gpointer opaque)
>           uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
>       } dropped;
>       unsigned int idx = 0;
> -    uint64_t dropped_count;
> +    int dropped_count;

Just because of Glib's limitations, we are limiting our counting 
ability, that's bad.

>       size_t unused __attribute__ ((unused));
>
>       for (;;) {
> @@ -169,16 +169,16 @@ static gpointer writeout_thread(gpointer opaque)
>           if (dropped_events) {
>               dropped.rec.event = DROPPED_EVENT_ID,
>               dropped.rec.timestamp_ns = get_clock();
> -            dropped.rec.length = sizeof(TraceRecord) + sizeof(dropped_events),
> +            dropped.rec.length = sizeof(TraceRecord) + sizeof(uint64_t),
>               dropped.rec.reserved = 0;
>               while (1) {
>                   dropped_count = dropped_events;
> -                if (g_atomic_int_compare_and_exchange((gint *)&dropped_events,
> +                if (g_atomic_int_compare_and_exchange(&dropped_events,
>                                                         dropped_count, 0)) {
>                       break;
>                   }
>               }
> -            memcpy(dropped.rec.arguments, &dropped_count, sizeof(uint64_t));
> +            dropped.rec.arguments[0] = dropped_count;

I like the above improvement for the price paid.

Lets see what Stefan has to say about this patch.

thanks,
Harsh

>               unused = fwrite(&dropped.rec, dropped.rec.length, 1, trace_fp);
>           }
>
> @@ -220,11 +220,11 @@ int trace_record_start(TraceBufferRecord *rec, 
> TraceEventID event, size_t datasi
>
>           if (new_idx - writeout_idx > TRACE_BUF_LEN) {
>               /* Trace Buffer Full, Event dropped ! */
> -            g_atomic_int_inc((gint *)&dropped_events);
> +            g_atomic_int_inc(&dropped_events);
>               return -ENOSPC;
>           }
>
> -        if (g_atomic_int_compare_and_exchange((gint *)&trace_idx,
> +        if (g_atomic_int_compare_and_exchange(&trace_idx,
>                                                 old_idx, new_idx)) {
>               break;
>           }