[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/3] Tracepoint, buffer & monitor framework

From: Prerna Saxena
Subject: Re: [Qemu-devel] [PATCH 2/3] Tracepoint, buffer & monitor framework
Date: Tue, 25 May 2010 23:50:16 +0530
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20100330 Fedora/3.0.4-1.fc11 Thunderbird/3.0.4

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

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.


(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.)


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().


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]