From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=35253 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OGyjp-0003lu-AH for qemu-devel@nongnu.org; Tue, 25 May 2010 14:20:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OGyjn-0000wm-O4 for qemu-devel@nongnu.org; Tue, 25 May 2010 14:20:25 -0400 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:41482) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OGyjm-0000w6-Uz for qemu-devel@nongnu.org; Tue, 25 May 2010 14:20:23 -0400 Received: from d28relay01.in.ibm.com (d28relay01.in.ibm.com [9.184.220.58]) by e28smtp06.in.ibm.com (8.14.3/8.13.1) with ESMTP id o4PIKIdN022270 for ; Tue, 25 May 2010 23:50:18 +0530 Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o4PIKHNM3149874 for ; Tue, 25 May 2010 23:50:18 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.3/8.13.1/NCO v10.0 AVout) with ESMTP id o4PIKHFB010003 for ; Wed, 26 May 2010 04:20:17 +1000 Message-ID: <4BFC14E0.1030906@linux.vnet.ibm.com> Date: Tue, 25 May 2010 23:50:16 +0530 From: Prerna Saxena MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/3] Tracepoint, buffer & monitor framework References: <4BFAA3BB.5030207@linux.vnet.ibm.com> <4BFAA60D.6080308@linux.vnet.ibm.com> <4BFAA673.4050809@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Maneesh Soni , Anthony Liguori , Ananth , qemu-devel@nongnu.org Hi Stefan, Thanks for having a look. As I'd mentioned, this patchset is *work in progress*, which explains the dummy comments and coding style violations at places :) I was merely sharing a draft of what my approach is -- so that we can work together on how much of it can add to the trace framework you've proposed. On 05/25/2010 05:10 PM, Stefan Hajnoczi wrote: > I think this is too much work. Let each tracepoint have its own global struct > tracepoint so it can directly reference it using tracepoint_##name - no hash > lookup needed. Add the QLIST_ENTRY directly to struct tracepoint so the > tracepoint register/unregister code can assign ids and look up tracepoints by > name. No critical path code needs to do name lookups and the hash table can > disappear. I had employed a combination of hash (derived from name) and an ID (which is the offset within a hash bucket where the tracepoint details are stored) to determine tracepoint information for a given name. Your suggestion to eliminate name queries is good, let me see how much of this can be scaled down. > > +#define DECLARE_TRACE(name, tproto, tstruct) \ > + struct __trace_struct_##name { \ > + tstruct \ > + }; \ > > Should this struct be packed so more fields can fit? > Yes, indeed. Thanks for reminding ! > + trace_queue->trace_buffer[tmp].metadata.write_complete = 0; > > This is not guaranteed to work without memory barriers. There is no way for > the trace consumer to block until there is more data available. The > synchronization needs to consider writing traces to a file, which has different > constraints than dumping the current contents of the trace buffer. > > We're missing a way to trace to a file. That could be done in binary or text. > It would be easier in text because we already have the format strings and don't > need a unique ID mapping in an external binary parsing tool. > OK, at the time of working on this I hadnt really thought of dumping traces in a file. It meant too much of IO latency that such tracing would bring in. My idea of a tracer entailed buffer based logging with a simple reader(see last) > Making data available after crash is also useful. The easiest way is to dump > the trace buffer from the core dump using gdb. However, we'd need some way of > making sense of the bytes. That could be done by reading the tracepoint_lib > structures from the core dump. > Agree. > (The way I do trace recovery from a core dump in my simple tracer is to binary > dump the trace buffer from the core dump. Since the trace buffer contents are > normally written out to file unchanged anyway, the simpletrace.py script can > read the dumped trace buffer like a normal trace file.) > > Nitpicks: > As I mentioned, this is work in progress so you'd have seen quite a lot of violations. Thanks for pointing those out, I'll clean those up for whatever approach we choose to use :) > Some added lines of code use tabs for indentation, 4 space indentation should > be used. > > +struct tracepoint { > + char *name; /* Tracepoint name */ > + uint8_t trace_id; /* numerical ID */ > > Maximum 256 tracepoints in QEMU? A limit of 65536 is less likely to > be an issue in the future. > No, this field describes the maximum tracepoints for a given hash queue. > + void __trace_print_##name(Monitor *mon, void *data) \ > + { \ > + struct __trace_struct_##name *entry; \ > + \ > + if(!entry) \ > + return; \ > > This does not work, entry is not initialized. Typo ! should've been : if(!data) > > +#define DO_TRACE(name, args...) \ > + trace_##name(args); > > This macro is unused? A relic that needs to be cleaned :) > > +/* In essence, the structure becomes : > + * struct tracepoint_lib { > > This comment will get out of sync easily. > > + qemu_malloc(sizeof(struct tracepoint_lib)); > + > + if (!new_entry) > + return NULL; /* No memory */ > > qemu_malloc() does not return NULL on out-of-memory, it aborts the program. > Same for allocating new_entry->entry.name. > Wondering how I forgot that ! thanks for reminding. > + new_entry->entry.name = (char*)qemu_malloc(strlen(name)+1); > + if(!new_entry->entry.name) > + return NULL; > + > + strncpy(new_entry->entry.name, name, strlen(name)+1); > > Perhaps just strdup() instead of manual qemu_malloc()/strncpy(). > > Stefan > I'll work on merging this circular buffer + monitor-based reader as a backend for your proposed tracer. Would it be a good idea to have two trace buffers -- when one is full, it gets written to disk ; while the second is used to log traces. I think the monitor interface for reading traces can be retained as is. Also, I'd implemented the monitor interface for enabling/disabling data logging for a given tracepoint (for a running guest) Not sure if this is supported in the set of patches you've posted ? It might be a good to have feature. -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India