From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50821 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OWmZF-0002vL-OI for qemu-devel@nongnu.org; Thu, 08 Jul 2010 04:34:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OWmZB-0007vh-H4 for qemu-devel@nongnu.org; Thu, 08 Jul 2010 04:34:49 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:56427) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OWmZB-0007vU-Cr for qemu-devel@nongnu.org; Thu, 08 Jul 2010 04:34:45 -0400 Received: by vws2 with SMTP id 2so802982vws.4 for ; Thu, 08 Jul 2010 01:34:44 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20100708082827.GA2174@stefan-thinkpad.transitives.com> References: <1278447241-2709-1-git-send-email-stefanha@linux.vnet.ibm.com> <1278447241-2709-3-git-send-email-stefanha@linux.vnet.ibm.com> <4C357342.3080708@linux.vnet.ibm.com> <20100708082827.GA2174@stefan-thinkpad.transitives.com> Date: Thu, 8 Jul 2010 09:34:02 +0100 Message-ID: Subject: Re: [Qemu-devel] [PATCH 3/3] trace: Flush trace buffer on exit From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Maneesh Soni , sripathik@in.ibm.com, qemu-devel@nongnu.org, Ananth , Prerna Saxena On Thu, Jul 8, 2010 at 9:28 AM, Stefan Hajnoczi wrote: > On Thu, Jul 08, 2010 at 12:12:10PM +0530, Prerna Saxena wrote: >> Hi Stefan, >> On 07/07/2010 01:44 AM, Stefan Hajnoczi wrote: >> >Signed-off-by: Stefan Hajnoczi >> >--- >> >This applies to the tracing branch at: >> > >> > =A0 http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/tracing-= dev >> > >> > =A0simpletrace.c | =A0 23 +++++++++++++++-------- >> > =A01 files changed, 15 insertions(+), 8 deletions(-) >> > >> >diff --git a/simpletrace.c b/simpletrace.c >> >index ace009f..9604ea6 100644 >> >--- a/simpletrace.c >> >+++ b/simpletrace.c >> >@@ -22,6 +22,20 @@ static TraceRecord trace_buf[TRACE_BUF_LEN]; >> > =A0static unsigned int trace_idx; >> > =A0static FILE *trace_fp; >> > >> >+static void flush_trace_buffer(void) >> >+{ >> >+ =A0 =A0if (!trace_fp) { >> >+ =A0 =A0 =A0 =A0trace_fp =3D fopen("/tmp/trace.log", "w"); >> >+ =A0 =A0 =A0 =A0if (trace_fp) { >> >+ =A0 =A0 =A0 =A0 =A0 =A0atexit(flush_trace_buffer); >> >+ =A0 =A0 =A0 =A0} >> >+ =A0 =A0} >> >+ =A0 =A0if (trace_fp) { >> >+ =A0 =A0 =A0 =A0size_t unused; /* for when fwrite(3) is declared warn_= unused_result */ >> >+ =A0 =A0 =A0 =A0unused =3D fwrite(trace_buf, trace_idx * sizeof(trace_= buf[0]), 1, trace_fp); >> >> I think this would be better denoted as : >> unused =3D fwrite(trace_buf, trace_idx * sizeof(TraceRecord), 1, trace_f= p); > > The sizeof(trace_buf[0]) idiom prevents bugs creeping in when the type > of trace_buf[] is changed. =A0It is easy to forget to update all relevant > uses of sizeof(TraceRecord) across the codebase and end up with > incorrect memset/memcpy() or read/write(). =A0It also means that a patch > changing the type of trace_buf[] doesn't need to touch as many places. > > Why repeat the trace_buf[]'s throughout the codebase? Should read: Why repeat trace_buf[]'s type throughout the codebase? :) > >> >+ =A0 =A0} >> >+} >> >+ >> > =A0static void trace(TraceEventID event, unsigned long x1, >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long x2, unsigned long= x3, >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long x4, unsigned long= x5) >> >@@ -44,15 +58,8 @@ static void trace(TraceEventID event, unsigned long = x1, >> > =A0 =A0 =A0rec->x5 =3D x5; >> > >> > =A0 =A0 =A0if (++trace_idx =3D=3D TRACE_BUF_LEN) { >> >+ =A0 =A0 =A0 =A0flush_trace_buffer(); >> > =A0 =A0 =A0 =A0 =A0trace_idx =3D 0; >> >- >> >- =A0 =A0 =A0 =A0if (!trace_fp) { >> >- =A0 =A0 =A0 =A0 =A0 =A0trace_fp =3D fopen("/tmp/trace.log", "w"); >> >- =A0 =A0 =A0 =A0} >> >- =A0 =A0 =A0 =A0if (trace_fp) { >> >- =A0 =A0 =A0 =A0 =A0 =A0size_t result =3D fwrite(trace_buf, sizeof tra= ce_buf, 1, trace_fp); >> >- =A0 =A0 =A0 =A0 =A0 =A0result =3D result; >> >- =A0 =A0 =A0 =A0} >> > =A0 =A0 =A0} >> > =A0} >> > >> >> I was wondering if we can extend this. One can have a monitor >> command such as "dump-trace" which would write a partly-filled >> buffer to file using a call to flush_trace_buffer(). > > Definitely! =A0I'd like that to be part of the trace file management > command patch. =A0This patch just adds the functionality to flush the > trace buffer. > >> But this has a few caveats. flush_trace_buffer() must reset >> trace_idx to 0 to prevent duplicate traces to be written once the >> buffer is filled up. >> Also, I'm wondering what happens in case qemu is started with -smp 2 >> or more. We might need to enforce some kind of synchronisation so >> that threads on other cpus do not log traces while the buffer is >> being sync'ed. ( For now, I have not been able to get upstream qemu >> run with -smp. Going forward, this is something that might need to >> be looked into.) > > The simple trace backend performs no synchronization - it is only safe > under the global QEMU iothread mutex. =A0This actually covers the majorit= y > of the code when using KVM, but we need to be explicit about this > limitation and explore it under non-KVM scenarios. > >> >> Regards, >> >> -- >> Prerna Saxena >> >> Linux Technology Centre, >> IBM Systems and Technology Lab, >> Bangalore, India > >