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