Re: [Qemu-devel] [PATCH 0/6] trace: Add support for trace events grouping

[Stefan Hajnoczi]

On Thu, Oct 13, 2011 at 04:10:56PM +0800, Mark Wu wrote:
> On 10/13/2011 01:14 AM, Mark Wu wrote:
> >This series add support for trace events grouping. The state of a given group
> >of trace events can be queried or changed in bulk by the following monitor
> >commands:
> >
> >* info trace-groups
> >   View available trace event groups and their state.  State 1 means enabled,
> >   state 0 means disabled.
> >
> >* trace-group NAME on|off
> >   Enable/disable a given trace event group.
> >
> >A group of trace events can also be enabled in early running stage through
> >adding its group name prefixed with "group:" to trace events list file
> >which is passed to "-trace events".
> >
> >Mark Wu (6):
> >   trace: Make "tracetool" generate a group list
> >   trace: Add HMP monitor commands for trace events group
> >   trace: Add trace events group implementation in the backend "simple"
> >   trace: Add trace events group implementation in the backend "stderr"
> >   trace: Enable "-trace events" argument to control initial state of
> >     groups
> >   trace: Update doc for trace events group
> >
> >  docs/tracing.txt  |   29 ++++++++++++++--
> >  hmp-commands.hx   |   14 ++++++++
> >  monitor.c         |   22 ++++++++++++
> >  scripts/tracetool |   94 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  trace-events      |   88 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  trace/control.c   |   17 +++++++++
> >  trace/control.h   |    9 +++++
> >  trace/default.c   |   15 ++++++++
> >  trace/simple.c    |   30 +++++++++++++++++
> >  trace/simple.h    |    7 ++++
> >  trace/stderr.c    |   32 ++++++++++++++++++
> >  trace/stderr.h    |    7 ++++
> >  12 files changed, 359 insertions(+), 5 deletions(-)
> >
> Sorry, there're some coding style problems in the patches. I have
> fixed them and will send out later in order to see if there's any
> other problem coming up. :)

Hi Mark,
Please run scripts/checkpatch.pl on the patches if you haven't already.
It will point out many of the coding style issues.

I think we can get the same convenience by adding wildcard trace event
support.  For example, virtio-blk trace events could be enabled using:

  trace-event virtio_blk_* on

This doesn't work for the memory allocation functions because their
names do not share a common unique prefix, e.g. g_malloc and
qemu_memalign.  However, most related trace events do share a common
unique prefix.

Wildcards don't require special comments in trace-events so it
eliminates the problem of people forgetting to add groups when they add
trace events to QEMU.

The other advantage is that wildcards can select fine-grained sets of
trace events, like ecc_mem_writel_mer, ecc_mem_writel_mdr, etc.
ecc_mem_writel_* selects all these related trace events (they are a
subset of the hw/eccmemctl.c trace events).  This is not possible with
the groups patches since each trace-event can only be in one group;
either we have a high-level hw/eccmemctl.c group or a lower-level
ecc_mem_writel group but both is not possible.

I suggest adding wildcard trace event matching instead of adding groups.
The code changes for wildcards should be much smaller than for groups.
What do you think?

Stefan