Re: [Qemu-devel] [PATCH for-1.4 1/2] trace: use glib atomic int types

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> Juan reported that RHEL 6.4 hosts give compiler warnings because we use
> unsigned int while glib prototypes use volatile gint in trace/simple.c.
>
>   trace/simple.c:223: error: pointer targets in passing argument 1 of 
> 'g_atomic_int_compare_and_exchange' differ in signedness

Meh.  Contrary to documentation and how current GLib versions behave, in
other words a bug in need of a workaround.

> These variables are only accessed with glib atomic int functions so
> let's play it by the book and use volatile gint.

gint is a silly alias for int, and used completely interchangeably, even
within GLib APIs.  Any pretentions of treating it as something more
abstract break down at the first printf(), if not earlier.  But if you
think it helps...

I doubt volatile has any effect here.

> Reported-by: Juan Quintela <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  trace/simple.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/trace/simple.c b/trace/simple.c
> index 74701e3..1d5d8e4 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -51,9 +51,9 @@ enum {
>  };
>  
>  uint8_t trace_buf[TRACE_BUF_LEN];
> -static unsigned int trace_idx;
> +static volatile gint trace_idx;
>  static unsigned int writeout_idx;
> -static int dropped_events;
> +static volatile gint dropped_events;
>  static FILE *trace_fp;
>  static char *trace_file_name;
>  
> @@ -267,7 +267,7 @@ void trace_record_finish(TraceBufferRecord *rec)
>      record.event |= TRACE_RECORD_VALID;
>      write_to_buffer(rec->tbuf_idx, &record, sizeof(TraceRecord));
>  
> -    if ((g_atomic_int_get(&trace_idx) - writeout_idx)
> +    if (((unsigned int)g_atomic_int_get(&trace_idx) - writeout_idx)
>          > TRACE_BUF_FLUSH_THRESHOLD) {
>          flush_trace_file(false);
>      }

Unchanged:

int trace_record_start(TraceBufferRecord *rec, TraceEventID event, size_t 
datasize)
{
    unsigned int idx, rec_off, old_idx, new_idx;
    uint32_t rec_len = sizeof(TraceRecord) + datasize;
    uint64_t timestamp_ns = get_clock();

    do {
        old_idx = g_atomic_int_get(&trace_idx);
        smp_rmb();
        new_idx = old_idx + rec_len;

        if (new_idx - writeout_idx > TRACE_BUF_LEN) {
            /* Trace Buffer Full, Event dropped ! */
            g_atomic_int_inc(&dropped_events);
            return -ENOSPC;
        }
    } while (!g_atomic_int_compare_and_exchange(&trace_idx, old_idx, new_idx));
[...]
}

Now mixes int trace_idx with unsigned old_idx, new_idx.  No biggie, but
you might want to clean it up anyway.