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

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

On 06/11/2012 08:25 PM, Stefan Hajnoczi wrote:
> On Mon, Jun 11, 2012 at 1:31 PM, Harsh Bora<harsh@linux.vnet.ibm.com>  wrote:
>> On 06/07/2012 08:02 PM, Stefan Hajnoczi wrote:
>>>
>>> On Thu, May 24, 2012 at 10:50 AM, Harsh Prateek Bora
>>> <harsh@linux.vnet.ibm.com>    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
>