From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58307) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SePJo-0006Dp-IO for qemu-devel@nongnu.org; Tue, 12 Jun 2012 07:31:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SePJg-0002uF-Sy for qemu-devel@nongnu.org; Tue, 12 Jun 2012 07:31:28 -0400 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:39130) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SePJf-0002sS-W9 for qemu-devel@nongnu.org; Tue, 12 Jun 2012 07:31:20 -0400 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 12 Jun 2012 17:01:08 +0530 Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q5CBV5Sw57999524 for ; Tue, 12 Jun 2012 17:01:05 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q5CH1aiT026169 for ; Wed, 13 Jun 2012 03:01:36 +1000 Message-ID: <4FD72875.9040702@linux.vnet.ibm.com> Date: Tue, 12 Jun 2012 17:01:01 +0530 From: Harsh Bora MIME-Version: 1.0 References: <1337853032-32590-1-git-send-email-harsh@linux.vnet.ibm.com> <1337853032-32590-3-git-send-email-harsh@linux.vnet.ibm.com> <4FD5E53E.7020505@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/3] Simpletrace v2: Add support for multiple args, strings. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: aneesh.kumar@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com, qemu-devel@nongnu.org On 06/11/2012 08:25 PM, Stefan Hajnoczi wrote: > On Mon, Jun 11, 2012 at 1:31 PM, Harsh Bora wrote: >> On 06/07/2012 08:02 PM, Stefan Hajnoczi wrote: >>> >>> On Thu, May 24, 2012 at 10:50 AM, Harsh Prateek Bora >>> 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 >