Re: [Qemu-devel] [PATCH v2 2/3] Simpletrace v2: Add support for multiple args, strings.

[Harsh Bora]

On 06/11/2012 08:25 PM, Stefan Hajnoczi wrote:
> On Mon, Jun 11, 2012 at 1:31 PM, Harsh Bora<address@hidden>  wrote:
> > On 06/07/2012 08:02 PM, Stefan Hajnoczi wrote:
> > > On Thu, May 24, 2012 at 10:50 AM, Harsh Prateek Bora
> > > <address@hidden>    wrote:
> > > > @@ -75,16 +96,22 @@ static char *trace_file_name = NULL;
> > > >   *
> > > >   * Returns false if the record is not valid.
> > > >   */
> > > > -static bool get_trace_record(unsigned int idx, TraceRecord *record)
> > > > +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
> > > >   {
> > > > -    if (!(trace_buf[idx].event&    TRACE_RECORD_VALID)) {
> > > >
> > > > +    uint8_t temp_rec[ST_REC_HDR_LEN];
> > > > +    TraceRecord *record = (TraceRecord *) temp_rec;
> > > > +    read_from_buffer(idx, temp_rec, ST_REC_HDR_LEN);
> > > > +
> > > > +    if (!(record->event&    TRACE_RECORD_VALID)) {
> > > >
> > > >          return false;
> > > >      }
> > > >
> > > >      __sync_synchronize(); /* read memory barrier before accessing record
> > > > */
> > >
> > >
> > > The need for the memory barrier is no longer clear.  Previously we
> > > were directly accessing the trace ring buffer, and therefore needed to
> > > ensure fields were settled before accessing them.  Now we use
> > > read_from_buffer() which copies the data into our temporary struct on
> > > the stack.
> > >
> > > I think the best way of doing it is to read the event field first in a
> > > separate step, then do the read memory barrier, and then read the rest
> > > of the record.  This ensures that the event field is at least as "old"
> > > as the other fields we read.
> >
> >
> > Currently, it does a two step read:
> >
> > read header (which includes event field and length of record as well)
> > sync
> > read rest of record (using length from header)
> >
> > Are you suggesting this:
> >
> > read event field only
> > sync (if event valid)
> > read header (for length)
> > sync
> > read rest of record (using length)
> >
> > or
> >
> > read event field only
> > sync (if event valid)
> > read header (for length)
> > read rest of record
>
> header, sync, rest of header + payload

I guess, we should read complete header after sync as well.

> The reason for the read barrier is to ensure that we don't read stale
> header fields (e.g. length) together with an up-to-date "valid" event
> field.
>
> > > > -    *record = trace_buf[idx];
> > > > -    record->event&= ~TRACE_RECORD_VALID;
> > > >
> > > > +    *recordptr = g_malloc(record->length);
> > > > +    /* make a copy of record to avoid being overwritten */
> > > > +    read_from_buffer(idx, (uint8_t *)*recordptr, record->length);
> > > > +    (*recordptr)->event&= ~TRACE_RECORD_VALID;
> > > >      return true;
> > > >   }
> >
> > [....]

[...]

> > Are you suggesting to use locking here ? I couldnt find a test-and-set
> > alternative in glib2. Does qemu have access to any such interface ?
>
> How about:
> http://developer.gnome.org/glib/2.32/glib-Atomic-Operations.html#g-atomic-int-compare-and-exchange
>
> I think this becomes:
>
> while True:
>      old_idx = trace_idx
>      rmb()
>      new_idx = old_idx + rec_len
>
>      if new_idx - write_idx>  TRACE_BUF_LEN:
>          dropped_events++
>          return
>
>      if g_atomic_int_compare_and_exchange((gint *)&trace_idx, old_idx, new_idx):
>          break
>
> Now we've reserved [new_idx, new_idx + rec_len).

Hmm :) I realized that after sending mail, however thanks for the algo.

> > > > +void trace_record_finish(TraceBufferRecord *rec);
> > > > +uint32_t safe_strlen(const char* str);
> > >
> > >
> > > Given that safe_strlen() is only used once and a generic utility (not
> > > specific to the simple trace backend), I suggest dropping it an just
> > > open coding the tristate operator: s ? strlen(s) : 0.
> >
> > safe_strlen is used multiple times in auto-generated code by tracetool in
> > expression for calculating the sum of size of trace arguments which as of
> > now looks like:
> >
> > 8 + 8 + 4 + safe_strlen(str1) + 8 + 4 + safe_strlen(str2) ...
> >
> > for tracing events with string arguments. For trace events with multiple
> > strings, this expression is more readable as compared to having an
> > expression with tristate operator like this:
> >
> > 8 + 8 + 4 + (str1 ? strlen(str1) : 0) + 8  + 4 + (str2 ? strlen(str2) : 0)
> > ...
> >
> >
> > I agree that its a generic function and need not be placed in tracing code,
> > let me know the right place where to put it if you think its worth keeping
> > it.
>
> The generic place to put it is cutils.c.  It's up to you if you want to keep it.

Hmm, I gave it a second thought and realized that having a generic 
qemu_strlen would require to return a negative value when string is null 
as non-null strings can return 0 as well, however tracing code required 
value 0 when string is NULL, so I dropped the idea of qemu_strlen 
(safe_strlen) and using tristate operators only.

Sending the updated patch series in a while.

regards,
Harsh

> Stefan