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

[Stefan Hajnoczi]

+#define DEFINE_TRACE(name, tproto, tassign, tprint)    \
+       void trace_##name(tproto)                               \
+       {                                                       \
+         unsigned int hash;                                    \
+         char tpname[] = __stringify(name);                    \
+         struct tracepoint *tp;                                \
+         struct __trace_struct_##name var, *entry;             \
+                                                               \
+         hash = tdb_hash(tpname);              \
+         tp = find_tracepoint_by_name(tpname);                 \
+         if (tp == NULL || !tp->state)                         \
+               return;                                         \
+                                                               \
+         entry = &var;                                         \
+         tassign                                               \
+                                                               \
+         write_trace_to_buffer(&qemu_buf, hash, tp->trace_id,  \
+               entry, sizeof(struct __trace_struct_##name));   \
+       }                                                       \

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.

+#define DECLARE_TRACE(name, tproto, tstruct)                   \
+       struct __trace_struct_##name {                          \
+               tstruct                                         \
+       };                                                      \

Should this struct be packed so more fields can fit?

+    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.

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

Nitpicks:

Some added lines of code use tabs for indentation, 4 space indentation should
be used.

+    {
+        .name       = "tracepoint",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show contents of trace buffer",

Copy-pasted, .help not updated.

@@ -145,6 +147,10 @@ struct Monitor {
 #ifdef CONFIG_DEBUG_MONITOR
     int print_calls_nr;
 #endif
+#ifdef CONFIG_QEMU_TRACE
+    struct DebugBuffer *qemu_buf_ptr;
+#endif
+
     QError *error;
     QLIST_HEAD(,mon_fd_t) fds;
     QLIST_ENTRY(Monitor) entry;

Would TraceBuffer be a more appropriate name for DebugBuffer?  qemu_buf_ptr is
vague, perhaps trace_buf is more clear?

I'm not sure I understand the reason for qemu_buf_ptr.  There is already a
global qemu_buf and qemu_buf_ptr is a pointer to that?

+    if(!strncmp(tp_state, "on", 3))
[...]
+           if(!strncmp(tp_state, "off", 4))

"on" with 3 and "off" with 4 are equivalent to strcmp().  "on" with 2 and
"off" with 3 would allow for any suffix after the matched string.

+#else /* CONFIG_QEMU_TRACE */
+static void do_tracepoint_status(Monitor *mon, const QDict *qdict)
+{
+    monitor_printf(mon, "Internal tracing not compiled\n");
+}
+#endif

"tracepoint" has this !CONFIG_QEMU_TRACE function but "trace" doesn't.

+#define INCREMENT_INDEX(HEAD,IDX) (HEAD->IDX++) % HEAD->buf_size
[...]
+    if ((trace_queue->last + 1) % trace_queue->buf_size
+                                        == trace_queue->first)
+       trace_queue->first = INCREMENT_INDEX(trace_queue, first);
+    trace_queue->last  = INCREMENT_INDEX(trace_queue, last);

Slightly safer macro:
#define NEXT_INDEX(HEAD,IDX) (((HEAD)->IDX + 1) % (HEAD)->buf_size)
[...]
if (NEXT_INDEX(trace_queue, last) == trace_queue->first)
    trace_queue->first = NEXT_INDEX(trace_queue, first);
trace_queue->last  = NEXT_INDEX(trace_queue, last);

+    tmp = trace_queue->last;
Instead of using tmp:
DebugEntry *entry = &trace_queue->trace_buffer[trace_queue->last];

This will make the code to fill out the trace entry easier to read.
"trace_queue->trace_buffer[tmp]." can be replaced with "entry->".

+#ifndef __TRACE_BUFFER_H__
+#define __TRACE_BUFFER_H__

QEMU doesn't use __ for include guards, just TRACE_BUFFER_H.

+#define EFFECTIVE_SLOT_SIZE     SLOT_SIZE-9
+
+struct DebugEntry{
+    char data[EFFECTIVE_SLOT_SIZE];
+    uint64_t timestamp;
+    struct {
+       unsigned write_complete :1;
+       unsigned reserved :7;
+       } metadata;
+};

There will be padding after data[] and before timestamp.  Perhaps data[]
should be the last field of the struct.  Also, QEMU coding style typedefs
structs and doesn't using the explicit "struct DebugEntry" tag.

I think this could be called a TraceEntry.

+struct DebugBuffer {
[...]
+       struct DebugEntry trace_buffer[BUFFER_SIZE];
+};

I think the struct could be called TraceBuffer and the array would be called
entries[].

@@ -158,7 +162,7 @@ ETEXI

 STEXI
 @item change @var{device} @var{setting}
address@hidden change
address@hidden :change

 Change the configuration of a device.

Not sure what this hunk is doing :).

+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.

+       void __trace_print_##name(Monitor *mon, void *data)     \
+       {                                                       \
+         struct __trace_struct_##name *entry;                  \
+                                                               \
+         if(!entry)                                            \
+               return;                                         \

This does not work, entry is not initialized.

+#define DO_TRACE(name, args...)                                        \
+           trace_##name(args);

This macro is unused?

+/* 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.

+    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