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

[Prerna Saxena <prerna@linux.vnet.ibm.com>]

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