[PATCH v3 0/8] [RfC] fix tracing for modules

[PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

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

[PATCH v3 1/8] meson: add trace_events_config[]

[Gerd Hoffmann]

It's an array of dicts, where each dict holds the configuration for one
trace-events file.  For now just fill it from trace_events_subdirs.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 meson.build       |  1 +
 trace/meson.build | 21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index af2bc8974111..1f3d48b53a06 100644
--- a/meson.build
+++ b/meson.build
@@ -1677,6 +1677,7 @@ target_softmmu_arch = {}
 
 # TODO: add each directory to the subdirs from its own meson.build, once
 # we have those
+trace_events_config = []
 trace_events_subdirs = [
   'accel/kvm',
   'accel/tcg',
diff --git a/trace/meson.build b/trace/meson.build
index a0be8f9b0db9..3a2b39dd6291 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -2,12 +2,23 @@
 specific_ss.add(files('control-target.c'))
 
 trace_events_files = []
-foreach dir : [ '.' ] + trace_events_subdirs
-  trace_events_file = meson.source_root() / dir / 'trace-events'
+
+trace_events_config += {
+  'file'  : meson.source_root() / 'trace-events',
+  'group' : 'root',
+}
+foreach dir : trace_events_subdirs
+  trace_events_config += {
+    'file'  : meson.source_root() / dir / 'trace-events',
+    'group' : dir.underscorify(),
+  }
+endforeach
+
+foreach c : trace_events_config
+  trace_events_file = c.get('file')
   trace_events_files += [ trace_events_file ]
-  group_name = dir == '.' ? 'root' : dir.underscorify()
-  group = '--group=' + group_name
-  fmt = '@0@-' + group_name + '.@1@'
+  group = '--group=' + c.get('group')
+  fmt = '@0@-' + c.get('group') + '.@1@'
 
   trace_h = custom_target(fmt.format('trace', 'h'),
                           output: fmt.format('trace', 'h'),
-- 
2.29.2

Re: [PATCH v3 1/8] meson: add trace_events_config[]

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Thu, Jan 21, 2021 at 01:50:21PM +0100, Gerd Hoffmann wrote:
> It's an array of dicts, where each dict holds the configuration for one
> trace-events file.  For now just fill it from trace_events_subdirs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  meson.build       |  1 +
>  trace/meson.build | 21 ++++++++++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 2/8] meson: move up hw subdir (specifically before trace subdir)

[Gerd Hoffmann]

Needed so trace/meson.build can see
stuff done in hw/*/meson.build.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 1f3d48b53a06..7462a50b4c36 100644
--- a/meson.build
+++ b/meson.build
@@ -1777,6 +1777,8 @@ if 'CONFIG_VHOST_USER' in config_host
   vhost_user = libvhost_user.get_variable('vhost_user_dep')
 endif
 
+subdir('hw')
+
 subdir('qapi')
 subdir('qobject')
 subdir('stubs')
@@ -1863,7 +1865,6 @@ subdir('migration')
 subdir('monitor')
 subdir('net')
 subdir('replay')
-subdir('hw')
 subdir('accel')
 subdir('plugins')
 subdir('bsd-user')
-- 
2.29.2

Re: [PATCH v3 2/8] meson: move up hw subdir (specifically before trace subdir)

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Thu, Jan 21, 2021 at 01:50:22PM +0100, Gerd Hoffmann wrote:
> Needed so trace/meson.build can see
> stuff done in hw/*/meson.build.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  meson.build | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 1f3d48b53a06..7462a50b4c36 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1777,6 +1777,8 @@ if 'CONFIG_VHOST_USER' in config_host
>    vhost_user = libvhost_user.get_variable('vhost_user_dep')
>  endif
>  
> +subdir('hw')
> +
>  subdir('qapi')

Please add a comment into the meson.build file explaining the ordering
requirement. This will prevent people accidentally breaking this in the
future.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 3/8] meson: add module_trace & module_trace_src

[Gerd Hoffmann]

module_trace is a dict which keeps track of the trace source files for a
module.

module_trace_src collects the trace source files for a given trace-events file,
which then either added to the source set or to a new module_trace dict
depending on whenever they are for a module or core qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 meson.build       |  3 ++-
 trace/meson.build | 16 ++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index 7462a50b4c36..a073fcd301c5 100644
--- a/meson.build
+++ b/meson.build
@@ -1890,7 +1890,8 @@ foreach d, list : modules
   foreach m, module_ss : list
     if enable_modules and targetos != 'windows'
       module_ss = module_ss.apply(config_all, strict: false)
-      sl = static_library(d + '-' + m, [genh, module_ss.sources()],
+      module_trace_src = module_trace.get(d + '-' + m, [])
+      sl = static_library(d + '-' + m, [genh, module_ss.sources(), module_trace_src],
                           dependencies: [modulecommon, module_ss.dependencies()], pic: true)
       if d == 'block'
         block_mods += sl
diff --git a/trace/meson.build b/trace/meson.build
index 3a2b39dd6291..843b472ba943 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -2,6 +2,7 @@
 specific_ss.add(files('control-target.c'))
 
 trace_events_files = []
+module_trace = {}
 
 trace_events_config += {
   'file'  : meson.source_root() / 'trace-events',
@@ -19,6 +20,8 @@ foreach c : trace_events_config
   trace_events_files += [ trace_events_file ]
   group = '--group=' + c.get('group')
   fmt = '@0@-' + c.get('group') + '.@1@'
+  mod = c.get('module', '')
+  module_trace_src = []
 
   trace_h = custom_target(fmt.format('trace', 'h'),
                           output: fmt.format('trace', 'h'),
@@ -34,10 +37,10 @@ foreach c : trace_events_config
                                 output: fmt.format('trace-ust', 'h'),
                                 input: trace_events_file,
                                 command: [ tracetool, group, '--format=ust-events-h', '@INPUT@', '@OUTPUT@' ])
-    trace_ss.add(trace_ust_h, lttng, urcubp)
+    module_trace_src += [ trace_ust_h, lttng, urcubp ]
     genh += trace_ust_h
   endif
-  trace_ss.add(trace_h, trace_c)
+  module_trace_src += [ trace_h, trace_c ]
   if 'CONFIG_TRACE_DTRACE' in config_host
     trace_dtrace = custom_target(fmt.format('trace-dtrace', 'dtrace'),
                                  output: fmt.format('trace-dtrace', 'dtrace'),
@@ -47,17 +50,22 @@ foreach c : trace_events_config
                                    output: fmt.format('trace-dtrace', 'h'),
                                    input: trace_dtrace,
                                    command: [ 'dtrace', '-DSTAP_SDT_V2', '-o', '@OUTPUT@', '-h', '-s', '@INPUT@' ])
-    trace_ss.add(trace_dtrace_h)
+    module_trace_src += trace_dtrace_h
     if host_machine.system() != 'darwin'
       trace_dtrace_o = custom_target(fmt.format('trace-dtrace', 'o'),
                                      output: fmt.format('trace-dtrace', 'o'),
                                      input: trace_dtrace,
                                      command: [ 'dtrace', '-DSTAP_SDT_V2', '-o', '@OUTPUT@', '-G', '-s', '@INPUT@' ])
-      trace_ss.add(trace_dtrace_o)
+      module_trace_src += trace_dtrace_o
     endif
 
     genh += trace_dtrace_h
   endif
+  if enable_modules and mod != ''
+    module_trace += { mod : module_trace_src }
+  else
+    trace_ss.add(module_trace_src)
+  endif
 endforeach
 
 trace_events_all = custom_target('trace-events-all',
-- 
2.29.2

Re: [PATCH v3 3/8] meson: add module_trace & module_trace_src

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Thu, Jan 21, 2021 at 01:50:23PM +0100, Gerd Hoffmann wrote:
> module_trace is a dict which keeps track of the trace source files for a
> module.
> 
> module_trace_src collects the trace source files for a given trace-events file,
> which then either added to the source set or to a new module_trace dict
> depending on whenever they are for a module or core qemu.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  meson.build       |  3 ++-
>  trace/meson.build | 16 ++++++++++++----
>  2 files changed, 14 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 4/8] meson: move qxl trace events to separate file

[Gerd Hoffmann]

Move qxl trace events to separate trace-events-qxl file.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl-render.c     |  1 +
 hw/display/qxl.c            |  1 +
 hw/display/meson.build      |  5 +++
 hw/display/trace-events     | 67 -------------------------------------
 hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++
 5 files changed, 73 insertions(+), 67 deletions(-)
 create mode 100644 hw/display/trace-events-qxl

diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index 3ce2e57b8feb..cc4862e26eb6 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -23,6 +23,7 @@
 #include "qxl.h"
 #include "sysemu/runstate.h"
 #include "trace.h"
+#include "trace/trace-hw_display_qxl.h"
 
 static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
 {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 431c1070967a..4e8d1bb8d77b 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -33,6 +33,7 @@
 #include "migration/blocker.h"
 #include "migration/vmstate.h"
 #include "trace.h"
+#include "trace/trace-hw_display_qxl.h"
 
 #include "qxl.h"
 
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 9d79e3951d9e..945c05ddbe42 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -42,6 +42,11 @@ if config_all_devices.has_key('CONFIG_QXL')
   qxl_ss = ss.source_set()
   qxl_ss.add(when: 'CONFIG_QXL', if_true: [files('qxl.c', 'qxl-logger.c', 'qxl-render.c'),
                                            pixman, spice])
+  trace_events_config += {
+    'file'   : meson.source_root() / 'hw' / 'display' / 'trace-events-qxl',
+    'group'  : 'hw_display_qxl',
+    'module' : 'hw-display-qxl',
+  }
   hw_display_modules += {'qxl': qxl_ss}
 endif
 
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 957b8ba99436..48636149e4b2 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -58,73 +58,6 @@ virtio_gpu_update_cursor(uint32_t scanout, uint32_t x, uint32_t y, const char *t
 virtio_gpu_fence_ctrl(uint64_t fence, uint32_t type) "fence 0x%" PRIx64 ", type 0x%x"
 virtio_gpu_fence_resp(uint64_t fence) "fence 0x%" PRIx64
 
-# qxl.c
-disable qxl_interface_set_mm_time(int qid, uint32_t mm_time) "%d %d"
-disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
-qxl_create_guest_primary(int qid, uint32_t width, uint32_t height, uint64_t mem, uint32_t format, uint32_t position) "%d %ux%u mem=0x%" PRIx64 " %u,%u"
-qxl_create_guest_primary_rest(int qid, int32_t stride, uint32_t type, uint32_t flags) "%d %d,%d,%d"
-qxl_destroy_primary(int qid) "%d"
-qxl_enter_vga_mode(int qid) "%d"
-qxl_exit_vga_mode(int qid) "%d"
-qxl_hard_reset(int qid, int64_t loadvm) "%d loadvm=%"PRId64
-qxl_interface_async_complete_io(int qid, uint32_t current_async, void *cookie) "%d current=%d cookie=%p"
-qxl_interface_attach_worker(int qid) "%d"
-qxl_interface_get_init_info(int qid) "%d"
-qxl_interface_set_compression_level(int qid, int64_t level) "%d %"PRId64
-qxl_interface_update_area_complete(int qid, uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom) "%d surface=%d [%d,%d,%d,%d]"
-qxl_interface_update_area_complete_rest(int qid, uint32_t num_updated_rects) "%d #=%d"
-qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
-qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty) "%d #dirty=%d"
-qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
-qxl_io_log(int qid, const char *log_buf) "%d %s"
-qxl_io_read_unexpected(int qid) "%d"
-qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
-qxl_io_write(int qid, const char *mode, uint64_t addr, const char *aname, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " (%s) val=%"PRIu64" size=%u async=%d"
-qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
-qxl_post_load(int qid, const char *mode) "%d %s"
-qxl_pre_load(int qid) "%d"
-qxl_pre_save(int qid) "%d"
-qxl_reset_surfaces(int qid) "%d"
-qxl_ring_command_check(int qid, const char *mode) "%d %s"
-qxl_ring_command_get(int qid, const char *mode) "%d %s"
-qxl_ring_command_req_notification(int qid) "%d"
-qxl_ring_cursor_check(int qid, const char *mode) "%d %s"
-qxl_ring_cursor_get(int qid, const char *mode) "%d %s"
-qxl_ring_cursor_req_notification(int qid) "%d"
-qxl_ring_res_push(int qid, const char *mode, uint32_t surface_count, uint32_t free_res, void *last_release, const char *notify) "%d %s s#=%d res#=%d last=%p notify=%s"
-qxl_ring_res_push_rest(int qid, uint32_t ring_has, uint32_t ring_size, uint32_t prod, uint32_t cons) "%d ring %d/%d [%d,%d]"
-qxl_ring_res_put(int qid, uint32_t free_res) "%d #res=%d"
-qxl_set_mode(int qid, int modenr, uint32_t x_res, uint32_t y_res, uint32_t bits, uint64_t devmem) "%d mode=%d [ x=%d y=%d @ bpp=%d devmem=0x%" PRIx64 " ]"
-qxl_soft_reset(int qid) "%d"
-qxl_spice_destroy_surfaces_complete(int qid) "%d"
-qxl_spice_destroy_surfaces(int qid, int async) "%d async=%d"
-qxl_spice_destroy_surface_wait_complete(int qid, uint32_t id) "%d sid=%d"
-qxl_spice_destroy_surface_wait(int qid, uint32_t id, int async) "%d sid=%d async=%d"
-qxl_spice_flush_surfaces_async(int qid, uint32_t surface_count, uint32_t num_free_res) "%d s#=%d, res#=%d"
-qxl_spice_monitors_config(int qid) "%d"
-qxl_spice_loadvm_commands(int qid, void *ext, uint32_t count) "%d ext=%p count=%d"
-qxl_spice_oom(int qid) "%d"
-qxl_spice_reset_cursor(int qid) "%d"
-qxl_spice_reset_image_cache(int qid) "%d"
-qxl_spice_reset_memslots(int qid) "%d"
-qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t right, uint32_t top, uint32_t bottom) "%d sid=%d [%d,%d,%d,%d]"
-qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
-qxl_surfaces_dirty(int qid, uint64_t offset, uint64_t size) "%d offset=0x%"PRIx64" size=0x%"PRIx64
-qxl_send_events(int qid, uint32_t events) "%d %d"
-qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
-qxl_set_guest_bug(int qid) "%d"
-qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
-qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d 0x%X %p"
-qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
-qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
-qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
-qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
-
-# qxl-render.c
-qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]"
-qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
-qxl_render_update_area_done(void *cookie) "%p"
-
 # vga.c
 vga_std_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
 vga_std_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
diff --git a/hw/display/trace-events-qxl b/hw/display/trace-events-qxl
new file mode 100644
index 000000000000..1146bd1640d2
--- /dev/null
+++ b/hw/display/trace-events-qxl
@@ -0,0 +1,66 @@
+# qxl.c
+disable qxl_interface_set_mm_time(int qid, uint32_t mm_time) "%d %d"
+disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
+qxl_create_guest_primary(int qid, uint32_t width, uint32_t height, uint64_t mem, uint32_t format, uint32_t position) "%d %ux%u mem=0x%" PRIx64 " %u,%u"
+qxl_create_guest_primary_rest(int qid, int32_t stride, uint32_t type, uint32_t flags) "%d %d,%d,%d"
+qxl_destroy_primary(int qid) "%d"
+qxl_enter_vga_mode(int qid) "%d"
+qxl_exit_vga_mode(int qid) "%d"
+qxl_hard_reset(int qid, int64_t loadvm) "%d loadvm=%"PRId64
+qxl_interface_async_complete_io(int qid, uint32_t current_async, void *cookie) "%d current=%d cookie=%p"
+qxl_interface_attach_worker(int qid) "%d"
+qxl_interface_get_init_info(int qid) "%d"
+qxl_interface_set_compression_level(int qid, int64_t level) "%d %"PRId64
+qxl_interface_update_area_complete(int qid, uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom) "%d surface=%d [%d,%d,%d,%d]"
+qxl_interface_update_area_complete_rest(int qid, uint32_t num_updated_rects) "%d #=%d"
+qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
+qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty) "%d #dirty=%d"
+qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
+qxl_io_log(int qid, const char *log_buf) "%d %s"
+qxl_io_read_unexpected(int qid) "%d"
+qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
+qxl_io_write(int qid, const char *mode, uint64_t addr, const char *aname, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " (%s) val=%"PRIu64" size=%u async=%d"
+qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
+qxl_post_load(int qid, const char *mode) "%d %s"
+qxl_pre_load(int qid) "%d"
+qxl_pre_save(int qid) "%d"
+qxl_reset_surfaces(int qid) "%d"
+qxl_ring_command_check(int qid, const char *mode) "%d %s"
+qxl_ring_command_get(int qid, const char *mode) "%d %s"
+qxl_ring_command_req_notification(int qid) "%d"
+qxl_ring_cursor_check(int qid, const char *mode) "%d %s"
+qxl_ring_cursor_get(int qid, const char *mode) "%d %s"
+qxl_ring_cursor_req_notification(int qid) "%d"
+qxl_ring_res_push(int qid, const char *mode, uint32_t surface_count, uint32_t free_res, void *last_release, const char *notify) "%d %s s#=%d res#=%d last=%p notify=%s"
+qxl_ring_res_push_rest(int qid, uint32_t ring_has, uint32_t ring_size, uint32_t prod, uint32_t cons) "%d ring %d/%d [%d,%d]"
+qxl_ring_res_put(int qid, uint32_t free_res) "%d #res=%d"
+qxl_set_mode(int qid, int modenr, uint32_t x_res, uint32_t y_res, uint32_t bits, uint64_t devmem) "%d mode=%d [ x=%d y=%d @ bpp=%d devmem=0x%" PRIx64 " ]"
+qxl_soft_reset(int qid) "%d"
+qxl_spice_destroy_surfaces_complete(int qid) "%d"
+qxl_spice_destroy_surfaces(int qid, int async) "%d async=%d"
+qxl_spice_destroy_surface_wait_complete(int qid, uint32_t id) "%d sid=%d"
+qxl_spice_destroy_surface_wait(int qid, uint32_t id, int async) "%d sid=%d async=%d"
+qxl_spice_flush_surfaces_async(int qid, uint32_t surface_count, uint32_t num_free_res) "%d s#=%d, res#=%d"
+qxl_spice_monitors_config(int qid) "%d"
+qxl_spice_loadvm_commands(int qid, void *ext, uint32_t count) "%d ext=%p count=%d"
+qxl_spice_oom(int qid) "%d"
+qxl_spice_reset_cursor(int qid) "%d"
+qxl_spice_reset_image_cache(int qid) "%d"
+qxl_spice_reset_memslots(int qid) "%d"
+qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t right, uint32_t top, uint32_t bottom) "%d sid=%d [%d,%d,%d,%d]"
+qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
+qxl_surfaces_dirty(int qid, uint64_t offset, uint64_t size) "%d offset=0x%"PRIx64" size=0x%"PRIx64
+qxl_send_events(int qid, uint32_t events) "%d %d"
+qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
+qxl_set_guest_bug(int qid) "%d"
+qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
+qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d 0x%X %p"
+qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
+qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
+qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
+qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
+
+# qxl-render.c
+qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]"
+qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
+qxl_render_update_area_done(void *cookie) "%p"
-- 
2.29.2

Re: [PATCH v3 4/8] meson: move qxl trace events to separate file

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Thu, Jan 21, 2021 at 01:50:24PM +0100, Gerd Hoffmann wrote:
> Move qxl trace events to separate trace-events-qxl file.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl-render.c     |  1 +
>  hw/display/qxl.c            |  1 +
>  hw/display/meson.build      |  5 +++
>  hw/display/trace-events     | 67 -------------------------------------
>  hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 73 insertions(+), 67 deletions(-)
>  create mode 100644 hw/display/trace-events-qxl

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/8] meson: move qxl trace events to separate file

[Daniel P. Berrangé]

On Thu, Jan 21, 2021 at 01:50:24PM +0100, Gerd Hoffmann wrote:
> Move qxl trace events to separate trace-events-qxl file.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl-render.c     |  1 +
>  hw/display/qxl.c            |  1 +
>  hw/display/meson.build      |  5 +++
>  hw/display/trace-events     | 67 -------------------------------------
>  hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 73 insertions(+), 67 deletions(-)
>  create mode 100644 hw/display/trace-events-qxl
> 
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index 3ce2e57b8feb..cc4862e26eb6 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -23,6 +23,7 @@
>  #include "qxl.h"
>  #include "sysemu/runstate.h"
>  #include "trace.h"
> +#include "trace/trace-hw_display_qxl.h"

I'm not a fan of this, as it feels like we're setting
up a completely different naming convention from the
one that already exists.

The previous "trace.h" header is relative to the source
directory. ie, "trace.h" is pulling in "hw/display/trace.h".

IMHO, we should be generating 'trace-qxl.h', alongside
the existing trace.h, so we can use a bare "trace-qxl.h"
include that refers to "hw/display/trace-qxl.h".

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 :|

[PATCH v3 5/8] trace: iter init tweaks

[Gerd Hoffmann]

Rename trace_event_iter_init() to trace_event_iter_init_pattern(),
add trace_event_iter_init_all() for interating over all events.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 trace/control.h        | 17 +++++++++++++----
 monitor/misc.c         |  4 ++--
 trace/control-target.c |  2 +-
 trace/control.c        | 16 +++++++++++-----
 trace/qmp.c            |  6 +++---
 trace/simple.c         |  2 +-
 6 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/trace/control.h b/trace/control.h
index 9522a7b318e2..ce40bd040574 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -20,15 +20,24 @@ typedef struct TraceEventIter {
 
 
 /**
- * trace_event_iter_init:
+ * trace_event_iter_init_all:
  * @iter: the event iterator struct
- * @pattern: optional pattern to filter events on name
  *
  * Initialize the event iterator struct @iter,
- * optionally using @pattern to filter out events
+ * for all events.
+ */
+void trace_event_iter_init_all(TraceEventIter *iter);
+
+/**
+ * trace_event_iter_init_pattern:
+ * @iter: the event iterator struct
+ * @pattern: pattern to filter events on name
+ *
+ * Initialize the event iterator struct @iter,
+ * using @pattern to filter out events
  * with non-matching names.
  */
-void trace_event_iter_init(TraceEventIter *iter, const char *pattern);
+void trace_event_iter_init_pattern(TraceEventIter *iter, const char *pattern);
 
 /**
  * trace_event_iter_next:
diff --git a/monitor/misc.c b/monitor/misc.c
index a7650ed74702..d62d5d25aa2e 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -2025,7 +2025,7 @@ void info_trace_events_completion(ReadLineState *rs, int nb_args, const char *st
         TraceEventIter iter;
         TraceEvent *ev;
         char *pattern = g_strdup_printf("%s*", str);
-        trace_event_iter_init(&iter, pattern);
+        trace_event_iter_init_pattern(&iter, pattern);
         while ((ev = trace_event_iter_next(&iter)) != NULL) {
             readline_add_completion(rs, trace_event_get_name(ev));
         }
@@ -2043,7 +2043,7 @@ void trace_event_completion(ReadLineState *rs, int nb_args, const char *str)
         TraceEventIter iter;
         TraceEvent *ev;
         char *pattern = g_strdup_printf("%s*", str);
-        trace_event_iter_init(&iter, pattern);
+        trace_event_iter_init_pattern(&iter, pattern);
         while ((ev = trace_event_iter_next(&iter)) != NULL) {
             readline_add_completion(rs, trace_event_get_name(ev));
         }
diff --git a/trace/control-target.c b/trace/control-target.c
index e293eeed7c00..8418673c18cf 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -127,7 +127,7 @@ void trace_init_vcpu(CPUState *vcpu)
 {
     TraceEventIter iter;
     TraceEvent *ev;
-    trace_event_iter_init(&iter, NULL);
+    trace_event_iter_init_all(&iter);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         if (trace_event_is_vcpu(ev) &&
             trace_event_get_state_static(ev) &&
diff --git a/trace/control.c b/trace/control.c
index cd04dd4e0c1c..bd5d79aacd94 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -90,7 +90,7 @@ TraceEvent *trace_event_name(const char *name)
 
     TraceEventIter iter;
     TraceEvent *ev;
-    trace_event_iter_init(&iter, NULL);
+    trace_event_iter_init_all(&iter);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         if (strcmp(trace_event_get_name(ev), name) == 0) {
             return ev;
@@ -99,10 +99,16 @@ TraceEvent *trace_event_name(const char *name)
     return NULL;
 }
 
-void trace_event_iter_init(TraceEventIter *iter, const char *pattern)
+void trace_event_iter_init_all(TraceEventIter *iter)
 {
     iter->event = 0;
     iter->group = 0;
+    iter->pattern = NULL;
+}
+
+void trace_event_iter_init_pattern(TraceEventIter *iter, const char *pattern)
+{
+    trace_event_iter_init_all(iter);
     iter->pattern = pattern;
 }
 
@@ -129,7 +135,7 @@ void trace_list_events(FILE *f)
 {
     TraceEventIter iter;
     TraceEvent *ev;
-    trace_event_iter_init(&iter, NULL);
+    trace_event_iter_init_all(&iter);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         fprintf(f, "%s\n", trace_event_get_name(ev));
     }
@@ -149,7 +155,7 @@ static void do_trace_enable_events(const char *line_buf)
     TraceEvent *ev;
     bool is_pattern = trace_event_is_pattern(line_ptr);
 
-    trace_event_iter_init(&iter, line_ptr);
+    trace_event_iter_init_pattern(&iter, line_ptr);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         if (!trace_event_get_state_static(ev)) {
             if (!is_pattern) {
@@ -257,7 +263,7 @@ void trace_fini_vcpu(CPUState *vcpu)
 
     trace_guest_cpu_exit(vcpu);
 
-    trace_event_iter_init(&iter, NULL);
+    trace_event_iter_init_all(&iter);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         if (trace_event_is_vcpu(ev) &&
             trace_event_get_state_static(ev) &&
diff --git a/trace/qmp.c b/trace/qmp.c
index 85f81e47cc6d..3b4f4702b4f0 100644
--- a/trace/qmp.c
+++ b/trace/qmp.c
@@ -55,7 +55,7 @@ static bool check_events(bool has_vcpu, bool ignore_unavailable, bool is_pattern
         /* error for unavailable events */
         TraceEventIter iter;
         TraceEvent *ev;
-        trace_event_iter_init(&iter, name);
+        trace_event_iter_init_pattern(&iter, name);
         while ((ev = trace_event_iter_next(&iter)) != NULL) {
             if (!ignore_unavailable && !trace_event_get_state_static(ev)) {
                 error_setg(errp, "event \"%s\" is disabled", trace_event_get_name(ev));
@@ -90,7 +90,7 @@ TraceEventInfoList *qmp_trace_event_get_state(const char *name,
     }
 
     /* Get states (all errors checked above) */
-    trace_event_iter_init(&iter, name);
+    trace_event_iter_init_pattern(&iter, name);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         TraceEventInfo *value;
         bool is_vcpu = trace_event_is_vcpu(ev);
@@ -153,7 +153,7 @@ void qmp_trace_event_set_state(const char *name, bool enable,
     }
 
     /* Apply changes (all errors checked above) */
-    trace_event_iter_init(&iter, name);
+    trace_event_iter_init_pattern(&iter, name);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         if (!trace_event_get_state_static(ev) ||
             (has_vcpu && !trace_event_is_vcpu(ev))) {
diff --git a/trace/simple.c b/trace/simple.c
index 9cd2ed1fb3f4..97b6f85168e7 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -286,7 +286,7 @@ static int st_write_event_mapping(void)
     TraceEventIter iter;
     TraceEvent *ev;
 
-    trace_event_iter_init(&iter, NULL);
+    trace_event_iter_init_all(&iter);
     while ((ev = trace_event_iter_next(&iter)) != NULL) {
         uint64_t id = trace_event_get_id(ev);
         const char *name = trace_event_get_name(ev);
-- 
2.29.2

Re: [PATCH v3 5/8] trace: iter init tweaks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Thu, Jan 21, 2021 at 01:50:25PM +0100, Gerd Hoffmann wrote:
> Rename trace_event_iter_init() to trace_event_iter_init_pattern(),
> add trace_event_iter_init_all() for interating over all events.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  trace/control.h        | 17 +++++++++++++----
>  monitor/misc.c         |  4 ++--
>  trace/control-target.c |  2 +-
>  trace/control.c        | 16 +++++++++++-----
>  trace/qmp.c            |  6 +++---
>  trace/simple.c         |  2 +-
>  6 files changed, 31 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 6/8] trace: add trace_event_iter_init_group

[Gerd Hoffmann]

This allows to interate over an event group.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 trace/control.h | 13 +++++++++++++
 trace/control.c | 19 ++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/trace/control.h b/trace/control.h
index ce40bd040574..23b8393b297e 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -13,8 +13,11 @@
 #include "event-internal.h"
 
 typedef struct TraceEventIter {
+    /* iter state */
     size_t event;
     size_t group;
+    /* filter conditions */
+    size_t group_id;
     const char *pattern;
 } TraceEventIter;
 
@@ -39,6 +42,16 @@ void trace_event_iter_init_all(TraceEventIter *iter);
  */
 void trace_event_iter_init_pattern(TraceEventIter *iter, const char *pattern);
 
+/**
+ * trace_event_iter_init_group:
+ * @iter: the event iterator struct
+ * @group_id: group_id to filter events by group.
+ *
+ * Initialize the event iterator struct @iter,
+ * using @group_id to filter for events in the group.
+ */
+void trace_event_iter_init_group(TraceEventIter *iter, size_t group_id);
+
 /**
  * trace_event_iter_next:
  * @iter: the event iterator struct
diff --git a/trace/control.c b/trace/control.c
index bd5d79aacd94..f1cc880b7cd1 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -103,6 +103,7 @@ void trace_event_iter_init_all(TraceEventIter *iter)
 {
     iter->event = 0;
     iter->group = 0;
+    iter->group_id = -1;
     iter->pattern = NULL;
 }
 
@@ -112,20 +113,32 @@ void trace_event_iter_init_pattern(TraceEventIter *iter, const char *pattern)
     iter->pattern = pattern;
 }
 
+void trace_event_iter_init_group(TraceEventIter *iter, size_t group_id)
+{
+    trace_event_iter_init_all(iter);
+    iter->group_id = group_id;
+}
+
 TraceEvent *trace_event_iter_next(TraceEventIter *iter)
 {
     while (iter->group < nevent_groups &&
            event_groups[iter->group].events[iter->event] != NULL) {
         TraceEvent *ev = event_groups[iter->group].events[iter->event];
+        size_t group = iter->group;
         iter->event++;
         if (event_groups[iter->group].events[iter->event] == NULL) {
             iter->event = 0;
             iter->group++;
         }
-        if (!iter->pattern ||
-            g_pattern_match_simple(iter->pattern, trace_event_get_name(ev))) {
-            return ev;
+        if (iter->pattern &&
+            !g_pattern_match_simple(iter->pattern, trace_event_get_name(ev))) {
+            continue;
         }
+        if (iter->group_id != -1 &&
+            iter->group_id != group) {
+            continue;
+        }
+        return ev;
     }
 
     return NULL;
-- 
2.29.2

Re: [PATCH v3 6/8] trace: add trace_event_iter_init_group

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On Thu, Jan 21, 2021 at 01:50:26PM +0100, Gerd Hoffmann wrote:
> This allows to interate over an event group.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  trace/control.h | 13 +++++++++++++
>  trace/control.c | 19 ++++++++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 7/8] trace/simple: pass iter to st_write_event_mapping

[Gerd Hoffmann]

Pass an iter to st_write_event_mapping, so the function can interate
different things depending on how we initialize the iter.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 trace/simple.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/trace/simple.c b/trace/simple.c
index 97b6f85168e7..ec2156d135cb 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -280,14 +280,12 @@ void trace_record_finish(TraceBufferRecord *rec)
     }
 }
 
-static int st_write_event_mapping(void)
+static int st_write_event_mapping(TraceEventIter *iter)
 {
     uint64_t type = TRACE_RECORD_TYPE_MAPPING;
-    TraceEventIter iter;
     TraceEvent *ev;
 
-    trace_event_iter_init_all(&iter);
-    while ((ev = trace_event_iter_next(&iter)) != NULL) {
+    while ((ev = trace_event_iter_next(iter)) != NULL) {
         uint64_t id = trace_event_get_id(ev);
         const char *name = trace_event_get_name(ev);
         uint32_t len = strlen(name);
@@ -309,6 +307,7 @@ static int st_write_event_mapping(void)
  */
 bool st_set_trace_file_enabled(bool enable)
 {
+    TraceEventIter iter;
     bool was_enabled = trace_fp;
 
     if (enable == !!trace_fp) {
@@ -333,8 +332,9 @@ bool st_set_trace_file_enabled(bool enable)
             return was_enabled;
         }
 
+        trace_event_iter_init_all(&iter);
         if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
-            st_write_event_mapping() < 0) {
+            st_write_event_mapping(&iter) < 0) {
             fclose(trace_fp);
             trace_fp = NULL;
             return was_enabled;
-- 
2.29.2

Re: [PATCH v3 7/8] trace/simple: pass iter to st_write_event_mapping

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On Thu, Jan 21, 2021 at 01:50:27PM +0100, Gerd Hoffmann wrote:
> Pass an iter to st_write_event_mapping, so the function can interate
> different things depending on how we initialize the iter.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  trace/simple.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 8/8] trace/simple: add st_init_group

[Gerd Hoffmann]

Add helper function and call it for each trace event group added.
Makes sure that events added at module load time are initialized
properly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 trace/simple.h  |  1 +
 trace/control.c |  4 ++++
 trace/simple.c  | 12 ++++++++++++
 3 files changed, 17 insertions(+)

diff --git a/trace/simple.h b/trace/simple.h
index 26ccbc8b8ae3..ee1983ce5617 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -15,6 +15,7 @@ void st_print_trace_file_status(void);
 bool st_set_trace_file_enabled(bool enable);
 void st_set_trace_file(const char *file);
 bool st_init(void);
+void st_init_group(size_t group);
 void st_flush_trace_buffer(void);
 
 typedef struct {
diff --git a/trace/control.c b/trace/control.c
index f1cc880b7cd1..9649d0979e70 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -81,6 +81,10 @@ void trace_event_register_group(TraceEvent **events)
     event_groups = g_renew(TraceEventGroup, event_groups, nevent_groups + 1);
     event_groups[nevent_groups].events = events;
     nevent_groups++;
+
+#ifdef CONFIG_TRACE_SIMPLE
+    st_init_group(nevent_groups - 1);
+#endif
 }
 
 
diff --git a/trace/simple.c b/trace/simple.c
index ec2156d135cb..ac499edee0d5 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -422,3 +422,15 @@ bool st_init(void)
     atexit(st_flush_trace_buffer);
     return true;
 }
+
+void st_init_group(size_t group)
+{
+    TraceEventIter iter;
+
+    if (!trace_writeout_enabled) {
+        return;
+    }
+
+    trace_event_iter_init_group(&iter, group);
+    st_write_event_mapping(&iter);
+}
-- 
2.29.2

Re: [PATCH v3 8/8] trace/simple: add st_init_group

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Thu, Jan 21, 2021 at 01:50:28PM +0100, Gerd Hoffmann wrote:
> Add helper function and call it for each trace event group added.
> Makes sure that events added at module load time are initialized
> properly.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  trace/simple.h  |  1 +
>  trace/control.c |  4 ++++
>  trace/simple.c  | 12 ++++++++++++
>  3 files changed, 17 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

  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

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Daniel P. Berrangé]

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 :|

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3413 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Daniel P. Berrangé]

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

The prefix is used by the systemtap backend. It lets us define
friendly probes, scoped for each QEMU emulator target.

eg a trace point "dma_map_wait" gets mapped to probes in many
.stp files, once per target, because we need to match based on
the executable path:

  probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
  probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-ppc64").mark("dma_map_wait")
  probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-aarch64").mark("dma_map_wait")

there is nothing here that reqiures us to change the prefix for
a module - we can carry on using the same systemtap probe name
whether modules are used or not.


Currently we kind of unofficially have a convention that the name
of a trace point should reflect its functional area. So all QXL
related probes have a name prefix "qxl_".

We could make that explicit, by having the trace-events files
support a group, IOW instead of

  qxl_destroy_primary(int qid) "%d"
  qxl_enter_vga_mode(int qid) "%d"
  qxl_exit_vga_mode(int qid) "%d"

We could say that a dot separates the group from the probe
name, and thus have


  qxl.destroy_primary(int qid) "%d"
  qxl.enter_vga_mode(int qid) "%d"
  qxl.exit_vga_mode(int qid) "%d"

this would be backwards compatible, as we can turn the "." back into
a "_" for all existing trace backends, except stp.

NB this is all tagential to use of modules at a build system /runtime
level. I think this explicit grouping of probes would make sense for
everything, to make the naming convention explicit instead of implicit.

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 :|

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

  Hi,

> eg a trace point "dma_map_wait" gets mapped to probes in many
> .stp files, once per target, because we need to match based on
> the executable path:
> 
>   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
>   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-ppc64").mark("dma_map_wait")

Probe qemu.system.ppc64.dma_map_wait = ...

Can I trace qemu started from build directory?
Seems scripts/qemu-trace-stap doesn't support that.

>   qxl.destroy_primary(int qid) "%d"
>   qxl.enter_vga_mode(int qid) "%d"
>   qxl.exit_vga_mode(int qid) "%d"
> 
> this would be backwards compatible, as we can turn the "." back into
> a "_" for all existing trace backends, except stp.

Hmm, not sure I like this inconsistency.  I think names should be the
same no matter which trace backend is used.

take care,
  Gerd

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Daniel P. Berrangé]

On Fri, Mar 26, 2021 at 01:47:00PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > eg a trace point "dma_map_wait" gets mapped to probes in many
> > .stp files, once per target, because we need to match based on
> > the executable path:
> > 
> >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
> >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-ppc64").mark("dma_map_wait")
> 
> Probe qemu.system.ppc64.dma_map_wait = ...
> 
> Can I trace qemu started from build directory?
> Seems scripts/qemu-trace-stap doesn't support that.

We should really generate extra equiv .stp files just for running from
the build.

> >   qxl.destroy_primary(int qid) "%d"
> >   qxl.enter_vga_mode(int qid) "%d"
> >   qxl.exit_vga_mode(int qid) "%d"
> > 
> > this would be backwards compatible, as we can turn the "." back into
> > a "_" for all existing trace backends, except stp.
> 
> Hmm, not sure I like this inconsistency.  I think names should be the
> same no matter which trace backend is used.

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 :|

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

On Mon, Mar 29, 2021 at 10:23:42AM +0100, Daniel P. Berrangé wrote:
> On Fri, Mar 26, 2021 at 01:47:00PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > eg a trace point "dma_map_wait" gets mapped to probes in many
> > > .stp files, once per target, because we need to match based on
> > > the executable path:
> > > 
> > >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
> > >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-ppc64").mark("dma_map_wait")
> > 
> > Probe qemu.system.ppc64.dma_map_wait = ...
> > 
> > Can I trace qemu started from build directory?
> > Seems scripts/qemu-trace-stap doesn't support that.
> 
> We should really generate extra equiv .stp files just for running from
> the build.

Well, "make install" with --prefix=$HOME/qemu-install fixed that for the time
being.

Now I have this:

kraxel@sirius ~/qemu-install/bin# sudo ./qemu-trace-stap -v run ./qemu-system-x86_64 "qxl_soft_reset"
Using tapset dir '/home/kraxel/qemu-install/share/systemtap/tapset' for binary './qemu-system-x86_64'
Compiling script 'probe qemu.system.x86_64.log.qxl_soft_reset {}'
semantic error: unresolved function pid: identifier 'pid' at /home/kraxel/qemu-install/share/systemtap/tapset/qemu-system-x86_64-log.stp:5451:41
        source:     printf("%d@%d qxl_soft_reset %d\n", pid(), gettimeofday_ns(), qid)
                                                        ^

Pass 2: analysis failed.  [man error::pass2]

Any clue why pid() isn't known?

thanks,
  Gerd

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Daniel P. Berrangé]

On Mon, Mar 29, 2021 at 11:48:18AM +0200, Gerd Hoffmann wrote:
> On Mon, Mar 29, 2021 at 10:23:42AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Mar 26, 2021 at 01:47:00PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > eg a trace point "dma_map_wait" gets mapped to probes in many
> > > > .stp files, once per target, because we need to match based on
> > > > the executable path:
> > > > 
> > > >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
> > > >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-ppc64").mark("dma_map_wait")
> > > 
> > > Probe qemu.system.ppc64.dma_map_wait = ...
> > > 
> > > Can I trace qemu started from build directory?
> > > Seems scripts/qemu-trace-stap doesn't support that.
> > 
> > We should really generate extra equiv .stp files just for running from
> > the build.
> 
> Well, "make install" with --prefix=$HOME/qemu-install fixed that for the time
> being.
> 
> Now I have this:
> 
> kraxel@sirius ~/qemu-install/bin# sudo ./qemu-trace-stap -v run ./qemu-system-x86_64 "qxl_soft_reset"
> Using tapset dir '/home/kraxel/qemu-install/share/systemtap/tapset' for binary './qemu-system-x86_64'
> Compiling script 'probe qemu.system.x86_64.log.qxl_soft_reset {}'
> semantic error: unresolved function pid: identifier 'pid' at /home/kraxel/qemu-install/share/systemtap/tapset/qemu-system-x86_64-log.stp:5451:41
>         source:     printf("%d@%d qxl_soft_reset %d\n", pid(), gettimeofday_ns(), qid)
>                                                         ^
> 
> Pass 2: analysis failed.  [man error::pass2]
> 
> Any clue why pid() isn't known?

Hmm, strange, makes me think we have a bug causing it to not pull in
global functions.

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 :|

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

  Hi,

> > Well, "make install" with --prefix=$HOME/qemu-install fixed that for the time
> > being.
> > 
> > Now I have this:
> > 
> > kraxel@sirius ~/qemu-install/bin# sudo ./qemu-trace-stap -v run ./qemu-system-x86_64 "qxl_soft_reset"
> > Using tapset dir '/home/kraxel/qemu-install/share/systemtap/tapset' for binary './qemu-system-x86_64'
> > Compiling script 'probe qemu.system.x86_64.log.qxl_soft_reset {}'
> > semantic error: unresolved function pid: identifier 'pid' at /home/kraxel/qemu-install/share/systemtap/tapset/qemu-system-x86_64-log.stp:5451:41
> >         source:     printf("%d@%d qxl_soft_reset %d\n", pid(), gettimeofday_ns(), qid)
> >                                                         ^
> > 
> > Pass 2: analysis failed.  [man error::pass2]
> > 
> > Any clue why pid() isn't known?
> 
> Hmm, strange, makes me think we have a bug causing it to not pull in
> global functions.

Hmm.  5.1.0 fails the same way.  5.2.0 fails in a different way.  Seems
we had temporary breakage in 5.2.0 (was that the module thing which
needed some workaround?).  Given 5.1.0 fails too I suspect this is a
systemtap change (/me runs fedora 33).  Googling didn't found much,
other than indicating pid() is used frequently in examples and
tutorials.

stap-prep asked for kernel-debuginfo, but installing that (huge package
with 3.5G(!) installed size) hasn't changed the situation.  Guess I only
actually need that if I want trace the kernel not userspace?

take care,
  Gerd

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

  Hi,

> > semantic error: unresolved function pid: identifier 'pid' at /home/kraxel/qemu-install/share/systemtap/tapset/qemu-system-x86_64-log.stp:5451:41
> >         source:     printf("%d@%d qxl_soft_reset %d\n", pid(), gettimeofday_ns(), qid)

> Hmm, strange, makes me think we have a bug causing it to not pull in
> global functions.

Yep.  qemu-trace-stap sets SYSTEMTAP_TAPSET to
/home/kraxel/qemu-install/share/systemtap/tapset.  Which makes stap use
that exclusively and ignore /usr/share/systemtap/tapset.  Which in turn
makes pid() disappear because that apparently is defined somewhere in
those tapsets.

IOW: It's broken for any $prefix != "/usr".

Using stap -I /home/kraxel/qemu-install/share/systemtap/tapset seems to
work fine, I guess qemu-trace-stap should use that instead ...

take care,
  Gerd

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

  Hi,

> eg a trace point "dma_map_wait" gets mapped to probes in many
> .stp files, once per target, because we need to match based on
> the executable path:
> 
>   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")

So, that changes with modules, we need the module name now, i.e.

    probe qemu.system.x86_64.qxl_soft_reset = \
	process("/home/kraxel/qemu-install/lib/qemu/hw-display-qxl.so").mark("qxl_soft_reset")

We could repeat that in every qemu-system-$arch.stp file.

We could also have one stp file per module, with probes like this:

    probe qemu.modules.qxl_soft_reset = \
        process("/home/kraxel/qemu-install/lib/qemu/hw-display-qxl.so").mark("qxl_soft_reset")

The later looks like a better fit to me, but has the drawback that the
tracepoints have different names in modular and non-modular builds ...

take care,
  Gerd

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Daniel P. Berrangé]

On Fri, Apr 09, 2021 at 03:12:45PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > eg a trace point "dma_map_wait" gets mapped to probes in many
> > .stp files, once per target, because we need to match based on
> > the executable path:
> > 
> >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
> 
> So, that changes with modules, we need the module name now, i.e.
> 
>     probe qemu.system.x86_64.qxl_soft_reset = \
> 	process("/home/kraxel/qemu-install/lib/qemu/hw-display-qxl.so").mark("qxl_soft_reset")
> 
> We could repeat that in every qemu-system-$arch.stp file.

This would have the surprise the 'qemu.system.x86_64.qxl_soft_reset'
probes will fire even for qemu-system-ppc64 / qemu-system-xxxxx etc
because we've not restricted the scope as the original probe did.

If we can't fix that, then we must use the second option to avoid
the surprise IMHO

> We could also have one stp file per module, with probes like this:
> 
>     probe qemu.modules.qxl_soft_reset = \
>         process("/home/kraxel/qemu-install/lib/qemu/hw-display-qxl.so").mark("qxl_soft_reset")
> 
> The later looks like a better fit to me, but has the drawback that the
> tracepoints have different names in modular and non-modular builds ...

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 :|

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Gerd Hoffmann]

On Fri, Apr 09, 2021 at 02:17:13PM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 09, 2021 at 03:12:45PM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > eg a trace point "dma_map_wait" gets mapped to probes in many
> > > .stp files, once per target, because we need to match based on
> > > the executable path:
> > > 
> > >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
> > 
> > So, that changes with modules, we need the module name now, i.e.
> > 
> >     probe qemu.system.x86_64.qxl_soft_reset = \
> > 	process("/home/kraxel/qemu-install/lib/qemu/hw-display-qxl.so").mark("qxl_soft_reset")
> > 
> > We could repeat that in every qemu-system-$arch.stp file.
> 
> This would have the surprise the 'qemu.system.x86_64.qxl_soft_reset'
> probes will fire even for qemu-system-ppc64 / qemu-system-xxxxx etc
> because we've not restricted the scope as the original probe did.

Oh, right.

> If we can't fix that, then we must use the second option to avoid
> the surprise IMHO

Yep.  Got that working.  Only problem is qemu-trace-stap is broken now
and it seems there is no easy way out.  Right now qemu-trace-stap can
simply work with a constant prefix, with that change the prefix can be
either qemu.system.$arch or qemu.system.modules and I suspect there is
no way around listing tracepoints to figure the correct name ...

take care,
  Gerd

PS: https://git.kraxel.org/cgit/qemu/log/?h=sirius/trace-modules

Re: [PATCH v3 0/8] [RfC] fix tracing for modules

[Daniel P. Berrangé]

On Mon, Apr 12, 2021 at 03:07:59PM +0200, Gerd Hoffmann wrote:
> On Fri, Apr 09, 2021 at 02:17:13PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Apr 09, 2021 at 03:12:45PM +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > eg a trace point "dma_map_wait" gets mapped to probes in many
> > > > .stp files, once per target, because we need to match based on
> > > > the executable path:
> > > > 
> > > >   probe qemu.system.x86_64.dma_map_wait = process("/usr/libexec/qemu-system-x86_64").mark("dma_map_wait")
> > > 
> > > So, that changes with modules, we need the module name now, i.e.
> > > 
> > >     probe qemu.system.x86_64.qxl_soft_reset = \
> > > 	process("/home/kraxel/qemu-install/lib/qemu/hw-display-qxl.so").mark("qxl_soft_reset")
> > > 
> > > We could repeat that in every qemu-system-$arch.stp file.
> > 
> > This would have the surprise the 'qemu.system.x86_64.qxl_soft_reset'
> > probes will fire even for qemu-system-ppc64 / qemu-system-xxxxx etc
> > because we've not restricted the scope as the original probe did.
> 
> Oh, right.
> 
> > If we can't fix that, then we must use the second option to avoid
> > the surprise IMHO
> 
> Yep.  Got that working.  Only problem is qemu-trace-stap is broken now
> and it seems there is no easy way out.  Right now qemu-trace-stap can
> simply work with a constant prefix, with that change the prefix can be
> either qemu.system.$arch or qemu.system.modules and I suspect there is
> no way around listing tracepoints to figure the correct name ...

Maybe just don't change the probe names, and declare that people have
to tell stap to filter by pid if you want exact matches

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 :|