[PATCH 0/3] Use only trace_event_get_state_backends() to check trace enablement

[PATCH 0/3] Use only trace_event_get_state_backends() to check trace enablement

[Peter Maydell]

Currently docs/devel/tracing.txt recommends checking the
TRACE_FOO_ENABLED macro to see if the FOO event is enabled,
for the purpose of skipping expensive computations that
are only used in trace output.

This is no longer the best way to do this; instead we
should prefer trace_event_get_state_backends(), which
checks both whether the event is compile-time disabled
and also whether it is run-time disabled.

Update the documentation, and also the 5 uses of the
old style in the tree.

thanks
-- PMM

Peter Maydell (3):
  docs/devel/tracing.txt: Recommend only
    trace_event_get_state_backends()
  memory.c: Use trace_event_get_state_backends()
  hw/display/qxl.c: Use trace_event_get_state_backends()

 docs/devel/tracing.txt | 12 +++++-------
 hw/display/qxl.c       |  2 +-
 memory.c               |  8 ++++----
 3 files changed, 10 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/3] docs/devel/tracing.txt: Recommend only trace_event_get_state_backends()

[Peter Maydell]

Instead of recommending checking the TRACE_FOO_ENABLED macro to
skip expensive computations needed only for tracing, recommend
only using trace_event_get_state_backends(). This works for both
compile-time and run-time disabling of events, and has no extra
performance impact if the event is compile-time disabled.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/tracing.txt | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 8c0376fefa4..cb5f685de9f 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -342,8 +342,10 @@ edit the "trace-events-all" file).
 
 In addition, there might be cases where relatively complex computations must be
 performed to generate values that are only used as arguments for a trace
-function. In these cases you can use the macro 'TRACE_${EVENT_NAME}_ENABLED' to
-guard such computations and avoid its compilation when the event is disabled:
+function. In these cases you can use 'trace_event_get_state_backends()' to
+guard such computations, so they are skipped if the event has been either
+compile-time disabled or run-time disabled. If the event is compile-time
+disabled, this check will have no performance impact.
 
     #include "trace.h"  /* needed for trace event prototype */
     
@@ -356,7 +358,7 @@ guard such computations and avoid its compilation when the event is disabled:
             align = getpagesize();
         }
         ptr = qemu_memalign(align, size);
-        if (TRACE_QEMU_VMALLOC_ENABLED) { /* preprocessor macro */
+        if (trace_event_get_state_backends(TRACE_QEMU_VMALLOC)) {
             void *complex;
             /* some complex computations to produce the 'complex' value */
             trace_qemu_vmalloc(size, ptr, complex);
@@ -364,10 +366,6 @@ guard such computations and avoid its compilation when the event is disabled:
         return ptr;
     }
 
-You can check both if the event has been disabled and is dynamically enabled at
-the same time using the 'trace_event_get_state_backends' routine (see header
-"trace/control.h" for more information).
-
 === "tcg" ===
 
 Guest code generated by TCG can be traced by defining an event with the "tcg"
-- 
2.20.1

[PATCH 2/3] memory.c: Use trace_event_get_state_backends()

[Peter Maydell]

The preferred way to test whether a trace event is enabled is to
use trace_event_get_state_backends(), because this will give the
correct answer (allowing expensive computations to be skipped)
whether the trace event is compile-time or run-time disabled.
Convert the four old-style direct uses of TRACE_FOO_ENABLED in
memory.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 memory.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/memory.c b/memory.c
index d7b9bb6951f..0baafe1fa90 100644
--- a/memory.c
+++ b/memory.c
@@ -434,7 +434,7 @@ static MemTxResult  memory_region_read_accessor(MemoryRegion *mr,
     tmp = mr->ops->read(mr->opaque, addr, size);
     if (mr->subpage) {
         trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
-    } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
+    } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
         trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size);
     }
@@ -456,7 +456,7 @@ static MemTxResult memory_region_read_with_attrs_accessor(MemoryRegion *mr,
     r = mr->ops->read_with_attrs(mr->opaque, addr, &tmp, size, attrs);
     if (mr->subpage) {
         trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
-    } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
+    } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
         trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, size);
     }
@@ -476,7 +476,7 @@ static MemTxResult memory_region_write_accessor(MemoryRegion *mr,
 
     if (mr->subpage) {
         trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size);
-    } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
+    } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
         trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size);
     }
@@ -496,7 +496,7 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr,
 
     if (mr->subpage) {
         trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, size);
-    } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
+    } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) {
         hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
         trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, size);
     }
-- 
2.20.1

[PATCH 3/3] hw/display/qxl.c: Use trace_event_get_state_backends()

[Peter Maydell]

The preferred way to test whether a trace event is enabled is to
use trace_event_get_state_backends(), because this will give the
correct answer (allowing expensive computations to be skipped)
whether the trace event is compile-time or run-time disabled.
Convert the old-style direct use of TRACE_FOO_ENABLED.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/qxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6d43b7433cf..80a4dcc40e4 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1764,7 +1764,7 @@ async_common:
         qxl_set_mode(d, val, 0);
         break;
     case QXL_IO_LOG:
-        if (TRACE_QXL_IO_LOG_ENABLED || d->guestdebug) {
+        if (trace_event_get_state_backends(TRACE_QXL_IO_LOG) || d->guestdebug) {
             /* We cannot trust the guest to NUL terminate d->ram->log_buf */
             char *log_buf = g_strndup((const char *)d->ram->log_buf,
                                       sizeof(d->ram->log_buf));
-- 
2.20.1

Re: [PATCH 3/3] hw/display/qxl.c: Use trace_event_get_state_backends()

[Gerd Hoffmann]

On Mon, Jan 20, 2020 at 03:11:42PM +0000, Peter Maydell wrote:
> The preferred way to test whether a trace event is enabled is to
> use trace_event_get_state_backends(), because this will give the
> correct answer (allowing expensive computations to be skipped)
> whether the trace event is compile-time or run-time disabled.
> Convert the old-style direct use of TRACE_FOO_ENABLED.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/display/qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 6d43b7433cf..80a4dcc40e4 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1764,7 +1764,7 @@ async_common:
>          qxl_set_mode(d, val, 0);
>          break;
>      case QXL_IO_LOG:
> -        if (TRACE_QXL_IO_LOG_ENABLED || d->guestdebug) {
> +        if (trace_event_get_state_backends(TRACE_QXL_IO_LOG) || d->guestdebug) {

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

>              /* We cannot trust the guest to NUL terminate d->ram->log_buf */
>              char *log_buf = g_strndup((const char *)d->ram->log_buf,
>                                        sizeof(d->ram->log_buf));
> -- 
> 2.20.1
> 

Re: [PATCH 0/3] Use only trace_event_get_state_backends() to check trace enablement

[Stefan Hajnoczi]

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

On Mon, Jan 20, 2020 at 03:11:39PM +0000, Peter Maydell wrote:
> Currently docs/devel/tracing.txt recommends checking the
> TRACE_FOO_ENABLED macro to see if the FOO event is enabled,
> for the purpose of skipping expensive computations that
> are only used in trace output.
> 
> This is no longer the best way to do this; instead we
> should prefer trace_event_get_state_backends(), which
> checks both whether the event is compile-time disabled
> and also whether it is run-time disabled.
> 
> Update the documentation, and also the 5 uses of the
> old style in the tree.
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   docs/devel/tracing.txt: Recommend only
>     trace_event_get_state_backends()
>   memory.c: Use trace_event_get_state_backends()
>   hw/display/qxl.c: Use trace_event_get_state_backends()
> 
>  docs/devel/tracing.txt | 12 +++++-------
>  hw/display/qxl.c       |  2 +-
>  memory.c               |  8 ++++----
>  3 files changed, 10 insertions(+), 12 deletions(-)
> 
> -- 
> 2.20.1
> 
> 

Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing

Stefan

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