Re: [Qemu-devel] [PATCH v6 2/3] Simpletrace v2: Support multiple arguments, strings.

[Harsh Bora <harsh@linux.vnet.ibm.com>]

On 06/15/2012 12:09 AM, Harsh Prateek Bora wrote:
>   static gpointer writeout_thread(gpointer opaque)
>   {
> -    TraceRecord record;
> -    unsigned int writeout_idx = 0;
> -    unsigned int num_available, idx;
> +    TraceRecord *recordptr, *dropped_ptr;
> +    union {
> +        TraceRecord rec;
> +        uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
> +    } dropped;
> +    dropped_ptr = (TraceRecord *)dropped.bytes;
> +    unsigned int idx = 0;
> +    uint64_t dropped_count;
>       size_t unused __attribute__ ((unused));
>
>       for (;;) {
>           wait_for_trace_records_available();
>
> -        num_available = trace_idx - writeout_idx;
> -        if (num_available>  TRACE_BUF_LEN) {
> -            record = (TraceRecord){
> -                .event = DROPPED_EVENT_ID,
> -                .x1 = num_available,
> -            };
> -            unused = fwrite(&record, sizeof(record), 1, trace_fp);
> -            writeout_idx += num_available;
> +        if (dropped_events) {
> +            dropped_ptr->event = DROPPED_EVENT_ID,
> +            dropped_ptr->timestamp_ns = get_clock();
> +            dropped_ptr->length = sizeof(TraceRecord) + sizeof(dropped_events),
> +            dropped_ptr->reserved = 0;
> +            while (1) {
> +                dropped_count = dropped_events;
> +                if (g_atomic_int_compare_and_exchange((gint *)&dropped_events,
> +                                                      dropped_count, 0)) {
> +                    break;
> +                }
> +            }
> +            memcpy(dropped_ptr->arguments,&dropped_count, sizeof(uint64_t));
> +            unused = fwrite(dropped_ptr, dropped_ptr->length, 1, trace_fp);
>           }
>
> -        idx = writeout_idx % TRACE_BUF_LEN;
> -        while (get_trace_record(idx,&record)) {
> -            trace_buf[idx].event = 0; /* clear valid bit */
> -            unused = fwrite(&record, sizeof(record), 1, trace_fp);
> -            idx = ++writeout_idx % TRACE_BUF_LEN;
> +        while (get_trace_record(idx,&recordptr)) {
> +            unused = fwrite(recordptr, recordptr->length, 1, trace_fp);
> +            writeout_idx += recordptr->length;
> +            g_free(recordptr);
>           }
> +        /* Note: The idx assignment expression below,
> +         * if kept in while-loop above, results in tracelog
> +         * corruption, possibly due to compiler-reordering of
> +         * statements. Keeping it out of loop saves a memory barrier.
> +         */

Hmm, I tried moving this assignment to while-loop using memory barrier 
before it, to ensure the loop can read multiple records without coming 
out of loop, however its still giving me corrupted records. However, 
keeping the assignment out of loop always works for me. Not sure what 
could be the reason for such a behaviour, above comment thus needs an 
update.

~ Harsh

> +        idx = writeout_idx % TRACE_BUF_LEN;
>
>           fflush(trace_fp);
>       }
>       return NULL;
>   }