From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Se62F-0001I6-QP for qemu-devel@nongnu.org; Mon, 11 Jun 2012 10:56:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Se626-0001id-MR for qemu-devel@nongnu.org; Mon, 11 Jun 2012 10:56:03 -0400 Received: from mail-lb0-f173.google.com ([209.85.217.173]:61181) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Se626-0001iS-8C for qemu-devel@nongnu.org; Mon, 11 Jun 2012 10:55:54 -0400 Received: by lbok6 with SMTP id k6so3288718lbo.4 for ; Mon, 11 Jun 2012 07:55:51 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4FD5E53E.7020505@linux.vnet.ibm.com> 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> Date: Mon, 11 Jun 2012 15:55:51 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: Harsh Bora Cc: aneesh.kumar@linux.vnet.ibm.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com On Mon, Jun 11, 2012 at 1:31 PM, Harsh Bora wrot= e: > On 06/07/2012 08:02 PM, Stefan Hajnoczi wrote: >> >> On Thu, May 24, 2012 at 10:50 AM, Harsh Prateek Bora >> =A0wrote: >>> @@ -75,16 +96,22 @@ static char *trace_file_name =3D NULL; >>> =A0* >>> =A0* Returns false if the record is not valid. >>> =A0*/ >>> -static bool get_trace_record(unsigned int idx, TraceRecord *record) >>> +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr= ) >>> =A0{ >>> - =A0 =A0if (!(trace_buf[idx].event& =A0TRACE_RECORD_VALID)) { >>> >>> + =A0 =A0uint8_t temp_rec[ST_REC_HDR_LEN]; >>> + =A0 =A0TraceRecord *record =3D (TraceRecord *) temp_rec; >>> + =A0 =A0read_from_buffer(idx, temp_rec, ST_REC_HDR_LEN); >>> + >>> + =A0 =A0if (!(record->event& =A0TRACE_RECORD_VALID)) { >>> >>> =A0 =A0 =A0 =A0 return false; >>> =A0 =A0 } >>> >>> =A0 =A0 __sync_synchronize(); /* read memory barrier before accessing r= ecord >>> */ >> >> >> The need for the memory barrier is no longer clear. =A0Previously we >> were directly accessing the trace ring buffer, and therefore needed to >> ensure fields were settled before accessing them. =A0Now 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. =A0This 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 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. >>> - =A0 =A0*record =3D trace_buf[idx]; >>> - =A0 =A0record->event&=3D ~TRACE_RECORD_VALID; >>> >>> + =A0 =A0*recordptr =3D g_malloc(record->length); >>> + =A0 =A0/* make a copy of record to avoid being overwritten */ >>> + =A0 =A0read_from_buffer(idx, (uint8_t *)*recordptr, record->length); >>> + =A0 =A0(*recordptr)->event&=3D ~TRACE_RECORD_VALID; >>> =A0 =A0 return true; >>> =A0} >>> > > [....] > > >>> >>> -void trace3(TraceEventID event, uint64_t x1, uint64_t x2, uint64_t x3) >>> +int trace_alloc_record(TraceBufferRecord *rec, TraceEventID event, >>> uint32_t datasize) >>> =A0{ >>> - =A0 =A0trace(event, x1, x2, x3, 0, 0, 0); >>> + =A0 =A0unsigned int idx, rec_off; >>> + =A0 =A0uint32_t rec_len =3D ST_REC_HDR_LEN + datasize; >>> + =A0 =A0uint64_t timestamp_ns =3D get_clock(); >>> + >>> + =A0 =A0if ((rec_len + trace_idx - writeout_idx)> =A0TRACE_BUF_LEN) { >>> + =A0 =A0 =A0 =A0/* Trace Buffer Full, Event dropped ! */ >>> + =A0 =A0 =A0 =A0dropped_events++; >>> + =A0 =A0 =A0 =A0return 1; >> >> >> -ENOSPC? =A0A negative errno is clearer than 0 - success, 1 - failure. >> >>> + =A0 =A0} >>> + =A0 =A0idx =3D g_atomic_int_exchange_and_add((gint *)&trace_idx, rec_= len) % >>> TRACE_BUF_LEN; >> >> >> How does this deal with the race condition of two or more threads >> running trace_alloc_record() concurrently when the buffer reaches max >> size? =A0I think two or more threads could think they have enough space >> because trace_idx hasn't been updated yet between the buffer length >> check and the atomic update. > > > 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-i= nt-compare-and-exchange I think this becomes: while True: old_idx =3D trace_idx rmb() new_idx =3D 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_i= dx): break Now we've reserved [new_idx, new_idx + rec_len). >>> +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 =A0+ 4 + (str2 ? strlen(str2) := 0) > ... > > > I agree that its a generic function and need not be placed in tracing cod= e, > let me know the right place where to put it if you think its worth keepin= g > it. The generic place to put it is cutils.c. It's up to you if you want to kee= p it. Stefan