First version that actually works.  Only qxl covered for this RfC,
other modules will follow once the basics are hashed out.

v3:
 - handle initialization of modular tracepoints.

TODO:
Enabling modular tracepoints via -trace cmd line doesn't work yet.
Guess we need to store the list somewhere for later re-processing.
Error handling is tricky, specifically the "tracepoint doesn't exist"
error.  Suggestions / ideas are welcome.

More context:
  https://bugzilla.redhat.com/show_bug.cgi?id=1898700
  https://bugzilla.redhat.com/show_bug.cgi?id=1869642

take care,
  Gerd

Gerd Hoffmann (8):
  meson: add trace_events_config[]
  meson: move up hw subdir (specifically before trace subdir)
  meson: add module_trace & module_trace_src
  meson: move qxl trace events to separate file
  trace: iter init tweaks
  trace: add trace_event_iter_init_group
  trace/simple: pass iter to st_write_event_mapping
  trace/simple: add st_init_group

 trace/control.h             | 30 ++++++++++++++---
 trace/simple.h              |  1 +
 hw/display/qxl-render.c     |  1 +
 hw/display/qxl.c            |  1 +
 monitor/misc.c              |  4 +--
 trace/control-target.c      |  2 +-
 trace/control.c             | 39 ++++++++++++++++-----
 trace/qmp.c                 |  6 ++--
 trace/simple.c              | 22 +++++++++---
 hw/display/meson.build      |  5 +++
 hw/display/trace-events     | 67 -------------------------------------
 hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++
 meson.build                 |  7 ++--
 trace/meson.build           | 37 +++++++++++++++-----
 14 files changed, 187 insertions(+), 101 deletions(-)
 create mode 100644 hw/display/trace-events-qxl

-- 
2.29.2

On Thu, Jan 21, 2021 at 01:50:20PM +0100, Gerd Hoffmann wrote:
> First version that actually works.  Only qxl covered for this RfC,
> other modules will follow once the basics are hashed out.
> 
> v3:
>  - handle initialization of modular tracepoints.

Cool, this looks promising!

> TODO:
> Enabling modular tracepoints via -trace cmd line doesn't work yet.
> Guess we need to store the list somewhere for later re-processing.
> Error handling is tricky, specifically the "tracepoint doesn't exist"
> error.  Suggestions / ideas are welcome.

Two ideas:

Global trace event name list
----------------------------
Build *some* global information about all trace events, including
modules, into the main QEMU binary. For example, generate an array of
all trace event names so QEMU can always print an error if a
non-existent trace event name is used. (This is similar to the
trace-events-all file, which is a global list of all trace events.)

Module name prefixes
--------------------
Allow an optional module/group prefix like qxl:my_trace_event. When the
user says:

  --trace qxl:my_trace_event

QEMU knows that this trace event belongs to the "qxl" module/group. It
will not attempt to load it until the qxl module registers itself.

If "my_trace_event" doesn't exist in the qxl module:
1. If the qxl module is not loaded we don't hit an error. Nevermind.
2. When the qxl module is loaded pending events are resolved and an
   error is printed.

  Hi,

> > TODO:
> > Enabling modular tracepoints via -trace cmd line doesn't work yet.
> > Guess we need to store the list somewhere for later re-processing.
> > Error handling is tricky, specifically the "tracepoint doesn't exist"
> > error.  Suggestions / ideas are welcome.
> 
> Two ideas:
> 
> Global trace event name list
> ----------------------------
> Build *some* global information about all trace events, including
> modules, into the main QEMU binary. For example, generate an array of
> all trace event names so QEMU can always print an error if a
> non-existent trace event name is used. (This is similar to the
> trace-events-all file, which is a global list of all trace events.)
> 
> Module name prefixes
> --------------------
> Allow an optional module/group prefix like qxl:my_trace_event. When the
> user says:
> 
>   --trace qxl:my_trace_event
> 
> QEMU knows that this trace event belongs to the "qxl" module/group. It
> will not attempt to load it until the qxl module registers itself.
> 
> If "my_trace_event" doesn't exist in the qxl module:
> 1. If the qxl module is not loaded we don't hit an error. Nevermind.
> 2. When the qxl module is loaded pending events are resolved and an
>    error is printed.

Finally found the time to look at this again... 

So, we already have a "group".  Which is basically the sub-directory of
the trace-events file right now, and it seems to be mostly a build system
thing.  We get many small lists instead of one huge, but there seems to
be no other effect.  We could change that though, by giving each group
an (optional?) prefix.

There also is a probe prefix, apparently used by dtrace only.  Not sure
how to deal with that.  It prefix is qemu-<target-type>-<target-name>.
Giving qemu modules its own dtrace prefix looks sensible to me.  That
would probably something like "qemu-module-<name>".

take care,
  Gerd

On Mon, Feb 22, 2021 at 04:13:32PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > TODO:
> > > Enabling modular tracepoints via -trace cmd line doesn't work yet.
> > > Guess we need to store the list somewhere for later re-processing.
> > > Error handling is tricky, specifically the "tracepoint doesn't exist"
> > > error.  Suggestions / ideas are welcome.
> > 
> > Two ideas:
> > 
> > Global trace event name list
> > ----------------------------
> > Build *some* global information about all trace events, including
> > modules, into the main QEMU binary. For example, generate an array of
> > all trace event names so QEMU can always print an error if a
> > non-existent trace event name is used. (This is similar to the
> > trace-events-all file, which is a global list of all trace events.)
> > 
> > Module name prefixes
> > --------------------
> > Allow an optional module/group prefix like qxl:my_trace_event. When the
> > user says:
> > 
> >   --trace qxl:my_trace_event
> > 
> > QEMU knows that this trace event belongs to the "qxl" module/group. It
> > will not attempt to load it until the qxl module registers itself.
> > 
> > If "my_trace_event" doesn't exist in the qxl module:
> > 1. If the qxl module is not loaded we don't hit an error. Nevermind.
> > 2. When the qxl module is loaded pending events are resolved and an
> >    error is printed.
> 
> Finally found the time to look at this again... 
> 
> So, we already have a "group".  Which is basically the sub-directory of
> the trace-events file right now, and it seems to be mostly a build system
> thing.  We get many small lists instead of one huge, but there seems to
> be no other effect.  We could change that though, by giving each group
> an (optional?) prefix.

Yes. This reminds me of an idea that was mentioned at the beginning of
this effort: maybe QEMU modules should always have their own directory
in the source tree instead of being alongside other source files that
are built into the main binary.

> There also is a probe prefix, apparently used by dtrace only.  Not sure
> how to deal with that.  It prefix is qemu-<target-type>-<target-name>.
> Giving qemu modules its own dtrace prefix looks sensible to me.  That
> would probably something like "qemu-module-<name>".

I think the DTrace prefix needs to be the same as the executable name,
but I'm not sure. I also don't know how that extends to shared libraries
like QEMU modules. I'm afraid you would need to investigate the DTrace
prefix.

Stefan

On Mon, Mar 22, 2021 at 11:36:39AM +0000, Stefan Hajnoczi wrote:
> On Mon, Feb 22, 2021 at 04:13:32PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > TODO:
> > > > Enabling modular tracepoints via -trace cmd line doesn't work yet.
> > > > Guess we need to store the list somewhere for later re-processing.
> > > > Error handling is tricky, specifically the "tracepoint doesn't exist"
> > > > error.  Suggestions / ideas are welcome.
> > > 
> > > Two ideas:
> > > 
> > > Global trace event name list
> > > ----------------------------
> > > Build *some* global information about all trace events, including
> > > modules, into the main QEMU binary. For example, generate an array of
> > > all trace event names so QEMU can always print an error if a
> > > non-existent trace event name is used. (This is similar to the
> > > trace-events-all file, which is a global list of all trace events.)
> > > 
> > > Module name prefixes
> > > --------------------
> > > Allow an optional module/group prefix like qxl:my_trace_event. When the
> > > user says:
> > > 
> > >   --trace qxl:my_trace_event
> > > 
> > > QEMU knows that this trace event belongs to the "qxl" module/group. It
> > > will not attempt to load it until the qxl module registers itself.
> > > 
> > > If "my_trace_event" doesn't exist in the qxl module:
> > > 1. If the qxl module is not loaded we don't hit an error. Nevermind.
> > > 2. When the qxl module is loaded pending events are resolved and an
> > >    error is printed.
> > 
> > Finally found the time to look at this again... 
> > 
> > So, we already have a "group".  Which is basically the sub-directory of
> > the trace-events file right now, and it seems to be mostly a build system
> > thing.  We get many small lists instead of one huge, but there seems to
> > be no other effect.  We could change that though, by giving each group
> > an (optional?) prefix.
> 
> Yes. This reminds me of an idea that was mentioned at the beginning of
> this effort: maybe QEMU modules should always have their own directory
> in the source tree instead of being alongside other source files that
> are built into the main binary.

This implies that each time we modularize something, we have to move
its source code. It is possible,  but it wouldn't be by preferred
choice.

> > There also is a probe prefix, apparently used by dtrace only.  Not sure
> > how to deal with that.  It prefix is qemu-<target-type>-<target-name>.
> > Giving qemu modules its own dtrace prefix looks sensible to me.  That
> > would probably something like "qemu-module-<name>".
> 
> I think the DTrace prefix needs to be the same as the executable name,
> but I'm not sure. I also don't know how that extends to shared libraries
> like QEMU modules. I'm afraid you would need to investigate the DTrace
> prefix.

I'm not aware of any requirement for dtrace prefix to match the
executable name. We don't even have an executable called "qemu"
so we're not matching even today.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Mar 22, 2021 at 11:46:55AM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 22, 2021 at 11:36:39AM +0000, Stefan Hajnoczi wrote:
> > On Mon, Feb 22, 2021 at 04:13:32PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > TODO:
> > > > > Enabling modular tracepoints via -trace cmd line doesn't work yet.
> > > > > Guess we need to store the list somewhere for later re-processing.
> > > > > Error handling is tricky, specifically the "tracepoint doesn't exist"
> > > > > error.  Suggestions / ideas are welcome.
> > > > 
> > > > Two ideas:
> > > > 
> > > > Global trace event name list
> > > > ----------------------------
> > > > Build *some* global information about all trace events, including
> > > > modules, into the main QEMU binary. For example, generate an array of
> > > > all trace event names so QEMU can always print an error if a
> > > > non-existent trace event name is used. (This is similar to the
> > > > trace-events-all file, which is a global list of all trace events.)
> > > > 
> > > > Module name prefixes
> > > > --------------------
> > > > Allow an optional module/group prefix like qxl:my_trace_event. When the
> > > > user says:
> > > > 
> > > >   --trace qxl:my_trace_event
> > > > 
> > > > QEMU knows that this trace event belongs to the "qxl" module/group. It
> > > > will not attempt to load it until the qxl module registers itself.
> > > > 
> > > > If "my_trace_event" doesn't exist in the qxl module:
> > > > 1. If the qxl module is not loaded we don't hit an error. Nevermind.
> > > > 2. When the qxl module is loaded pending events are resolved and an
> > > >    error is printed.
> > > 
> > > Finally found the time to look at this again... 
> > > 
> > > So, we already have a "group".  Which is basically the sub-directory of
> > > the trace-events file right now, and it seems to be mostly a build system
> > > thing.  We get many small lists instead of one huge, but there seems to
> > > be no other effect.  We could change that though, by giving each group
> > > an (optional?) prefix.
> > 
> > Yes. This reminds me of an idea that was mentioned at the beginning of
> > this effort: maybe QEMU modules should always have their own directory
> > in the source tree instead of being alongside other source files that
> > are built into the main binary.
> 
> This implies that each time we modularize something, we have to move
> its source code. It is possible,  but it wouldn't be by preferred
> choice.

If it bothers you then it probably bothers others too. Let's leave it.

> > > There also is a probe prefix, apparently used by dtrace only.  Not sure
> > > how to deal with that.  It prefix is qemu-<target-type>-<target-name>.
> > > Giving qemu modules its own dtrace prefix looks sensible to me.  That
> > > would probably something like "qemu-module-<name>".
> > 
> > I think the DTrace prefix needs to be the same as the executable name,
> > but I'm not sure. I also don't know how that extends to shared libraries
> > like QEMU modules. I'm afraid you would need to investigate the DTrace
> > prefix.
> 
> I'm not aware of any requirement for dtrace prefix to match the
> executable name. We don't even have an executable called "qemu"
> so we're not matching even today.

Great, thanks for the other email reply where you showed how the
SystemTap tapsetsuse the prefix!

Stefan