[PATCH] eal: add build-time option to omit trace

[PATCH] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_trace.c                      | 11 ++++++++++-
 config/rte_config.h                        |  1 +
 lib/eal/include/rte_trace_point.h          |  6 +++++-
 lib/eal/include/rte_trace_point_register.h |  8 ++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..7918cc865d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -12,7 +12,16 @@
 
 int app_dpdk_test_tp_count;
 
-#ifdef RTE_EXEC_ENV_WINDOWS
+#if !defined(RTE_TRACE)
+
+static int
+test_trace(void)
+{
+	printf("trace omitted at build-time, skipping test\n");
+	return TEST_SKIPPED;
+}
+
+#elif defined(RTE_EXEC_ENV_WINDOWS)
 
 static int
 test_trace(void)
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..1b60bba043 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+#ifdef RTE_TRACE
 /**
  * @internal
  *
@@ -230,6 +231,7 @@ __rte_trace_point_fp_is_enabled(void)
 	return false;
 #endif
 }
+#endif /* RTE_TRACE */
 
 /**
  * @internal
@@ -356,6 +358,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 	return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
 }
 
+#ifdef RTE_TRACE
+
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
@@ -411,7 +415,7 @@ do { \
 	RTE_SET_USED(len); \
 } while (0)
 
-
+#endif /* RTE_TRACE */
 #endif /* ALLOW_EXPERIMENTAL_API */
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..78c0ede5f1 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -18,6 +18,8 @@ extern "C" {
 
 RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
+#ifdef RTE_TRACE
+
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
@@ -27,6 +29,12 @@ RTE_INIT(trace##_init) \
 		(void (*)(void)) trace); \
 }
 
+#else
+
+#define RTE_TRACE_POINT_REGISTER(trace, name)
+
+#endif /* RTE_TRACE */
+
 #define __rte_trace_point_emit_header_generic(t) \
 	RTE_PER_LCORE(trace_point_sz) = __RTE_TRACE_EVENT_HEADER_SZ
 
-- 
2.43.0

Re: [PATCH] eal: add build-time option to omit trace

[Jerin Jacob]

On Wed, Sep 18, 2024 at 2:39 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  app/test/test_trace.c                      | 11 ++++++++++-
>  config/rte_config.h                        |  1 +
>  lib/eal/include/rte_trace_point.h          |  6 +++++-
>  lib/eal/include/rte_trace_point_register.h |  8 ++++++++
>  4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> index 00809f433b..7918cc865d 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -12,7 +12,16 @@
>
>  int app_dpdk_test_tp_count;
>
> -#ifdef RTE_EXEC_ENV_WINDOWS
> +#if !defined(RTE_TRACE)
> +
> +static int
> +test_trace(void)
> +{
> +       printf("trace omitted at build-time, skipping test\n");
> +       return TEST_SKIPPED;
> +}
> +
> +#elif defined(RTE_EXEC_ENV_WINDOWS)
>
>  static int
>  test_trace(void)
> diff --git a/config/rte_config.h b/config/rte_config.h
> index dd7bb0d35b..fd6f8a2f1a 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -49,6 +49,7 @@
>  #define RTE_MAX_TAILQ 32
>  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
>  #define RTE_MAX_VFIO_CONTAINERS 64
> +#define RTE_TRACE 1
>
>  /* bsd module defines */
>  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
> index 41e2a7f99e..1b60bba043 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -212,6 +212,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
>  __rte_experimental
>  rte_trace_point_t *rte_trace_point_lookup(const char *name);
>
> +#ifdef RTE_TRACE
>  /**
>   * @internal
>   *
> @@ -230,6 +231,7 @@ __rte_trace_point_fp_is_enabled(void)
>         return false;
>  #endif
>  }
> +#endif /* RTE_TRACE */
>
>  /**
>   * @internal
> @@ -356,6 +358,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
>         return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
>  }
>
> +#ifdef RTE_TRACE


Please change to 1.4.5 style _if possible_ in
https://doc.dpdk.org/guides/contributing/coding_style.html.
Assuming linker will remove the memory from the image if it is not
using by stubbing out.

Untested.

#define __rte_trace_point_emit_header_generic(t) \
void *mem; \
do { \
  +      if (RTE_TRACE_ENABLED == 0) \
  +             return \
        const uint64_t val = rte_atomic_load_explicit(t,
rte_memory_order_acquire); \
        if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
                return; \
        mem = __rte_trace_mem_get(val); \
        if (unlikely(mem == NULL)) \
                return; \
        mem = __rte_trace_point_emit_ev_header(mem, val); \
} while (0)



> +
>  #define __rte_trace_point_emit_header_generic(t) \
>  void *mem; \
>  do { \
> @@ -411,7 +415,7 @@ do { \
>         RTE_SET_USED(len); \
>  } while (0)
>
> -
> +#endif /* RTE_TRACE */
>  #endif /* ALLOW_EXPERIMENTAL_API */
>  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>
> diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
> index 41260e5964..78c0ede5f1 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -18,6 +18,8 @@ extern "C" {
>
>  RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
>
> +#ifdef RTE_TRACE
> +
>  #define RTE_TRACE_POINT_REGISTER(trace, name) \
>  rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
>  static const char __##trace##_name[] = RTE_STR(name); \
> @@ -27,6 +29,12 @@ RTE_INIT(trace##_init) \
>                 (void (*)(void)) trace); \
>  }
>
> +#else
> +
> +#define RTE_TRACE_POINT_REGISTER(trace, name)
> +
> +#endif /* RTE_TRACE */
> +
>  #define __rte_trace_point_emit_header_generic(t) \
>         RTE_PER_LCORE(trace_point_sz) = __RTE_TRACE_EVENT_HEADER_SZ
>
> --
> 2.43.0
>

RE: [PATCH] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Wednesday, 18 September 2024 11.50
to omit trace
> 
> On Wed, Sep 18, 2024 at 2:39 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Some applications want to omit the trace feature.
> > Either to reduce the memory footprint, to reduce the exposed attack
> > surface, or for other reasons.
> >
> > This patch adds an option in rte_config.h to include or omit trace in the
> > build. Trace is included by default.
> >
> > Omitting trace works by omitting all trace points.
> > For API and ABI compatibility, the trace feature itself remains.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  app/test/test_trace.c                      | 11 ++++++++++-
> >  config/rte_config.h                        |  1 +
> >  lib/eal/include/rte_trace_point.h          |  6 +++++-
> >  lib/eal/include/rte_trace_point_register.h |  8 ++++++++
> >  4 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> > index 00809f433b..7918cc865d 100644
> > --- a/app/test/test_trace.c
> > +++ b/app/test/test_trace.c
> > @@ -12,7 +12,16 @@
> >
> >  int app_dpdk_test_tp_count;
> >
> > -#ifdef RTE_EXEC_ENV_WINDOWS
> > +#if !defined(RTE_TRACE)
> > +
> > +static int
> > +test_trace(void)
> > +{
> > +       printf("trace omitted at build-time, skipping test\n");
> > +       return TEST_SKIPPED;
> > +}
> > +
> > +#elif defined(RTE_EXEC_ENV_WINDOWS)
> >
> >  static int
> >  test_trace(void)
> > diff --git a/config/rte_config.h b/config/rte_config.h
> > index dd7bb0d35b..fd6f8a2f1a 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -49,6 +49,7 @@
> >  #define RTE_MAX_TAILQ 32
> >  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
> >  #define RTE_MAX_VFIO_CONTAINERS 64
> > +#define RTE_TRACE 1
> >
> >  /* bsd module defines */
> >  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> > diff --git a/lib/eal/include/rte_trace_point.h
> b/lib/eal/include/rte_trace_point.h
> > index 41e2a7f99e..1b60bba043 100644
> > --- a/lib/eal/include/rte_trace_point.h
> > +++ b/lib/eal/include/rte_trace_point.h
> > @@ -212,6 +212,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
> >  __rte_experimental
> >  rte_trace_point_t *rte_trace_point_lookup(const char *name);
> >
> > +#ifdef RTE_TRACE
> >  /**
> >   * @internal
> >   *
> > @@ -230,6 +231,7 @@ __rte_trace_point_fp_is_enabled(void)
> >         return false;
> >  #endif
> >  }
> > +#endif /* RTE_TRACE */
> >
> >  /**
> >   * @internal
> > @@ -356,6 +358,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
> >         return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
> >  }
> >
> > +#ifdef RTE_TRACE
> 
> 
> Please change to 1.4.5 style _if possible_ in
> https://doc.dpdk.org/guides/contributing/coding_style.html.

The Coding Style's chapter 1.4.5 only applies to O/S selection.
Boolean configuration definitions either have the value 1 or are not defined.

> Assuming linker will remove the memory from the image if it is not
> using by stubbing out.

Assumption could be false if building with optimizations disabled. Don't know.

> 
> Untested.
> 
> #define __rte_trace_point_emit_header_generic(t) \
> void *mem; \
> do { \
>   +      if (RTE_TRACE_ENABLED == 0) \
>   +             return \
>         const uint64_t val = rte_atomic_load_explicit(t,
> rte_memory_order_acquire); \
>         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
>                 return; \
>         mem = __rte_trace_mem_get(val); \
>         if (unlikely(mem == NULL)) \
>                 return; \
>         mem = __rte_trace_point_emit_ev_header(mem, val); \
> } while (0)

I was initially down that path, inspired by how FP is enabled/disabled;
using an added __rte_trace_point_is_enabled(void) inline function, like the __rte_trace_point_fp_is_enabled(void).

But I kept getting lots of warnings, either about nonexisting references, or stuff not being used.

So I gave up and did it this way instead.
After having tried the other way, this way also looks cleaner to me.

> 
> 
> 
> > +
> >  #define __rte_trace_point_emit_header_generic(t) \
> >  void *mem; \
> >  do { \
> > @@ -411,7 +415,7 @@ do { \
> >         RTE_SET_USED(len); \
> >  } while (0)
> >
> > -
> > +#endif /* RTE_TRACE */
> >  #endif /* ALLOW_EXPERIMENTAL_API */
> >  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
> >
> > diff --git a/lib/eal/include/rte_trace_point_register.h
> b/lib/eal/include/rte_trace_point_register.h
> > index 41260e5964..78c0ede5f1 100644
> > --- a/lib/eal/include/rte_trace_point_register.h
> > +++ b/lib/eal/include/rte_trace_point_register.h
> > @@ -18,6 +18,8 @@ extern "C" {
> >
> >  RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
> >
> > +#ifdef RTE_TRACE
> > +
> >  #define RTE_TRACE_POINT_REGISTER(trace, name) \
> >  rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
> >  static const char __##trace##_name[] = RTE_STR(name); \
> > @@ -27,6 +29,12 @@ RTE_INIT(trace##_init) \
> >                 (void (*)(void)) trace); \
> >  }
> >
> > +#else
> > +
> > +#define RTE_TRACE_POINT_REGISTER(trace, name)
> > +
> > +#endif /* RTE_TRACE */
> > +
> >  #define __rte_trace_point_emit_header_generic(t) \
> >         RTE_PER_LCORE(trace_point_sz) = __RTE_TRACE_EVENT_HEADER_SZ
> >
> > --
> > 2.43.0
> >

[PATCH v2] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      | 11 ++++++++++-
 config/rte_config.h                        |  1 +
 lib/eal/include/rte_trace_point.h          | 22 +++++++++++++++++++++-
 lib/eal/include/rte_trace_point_register.h | 17 +++++++++++++++++
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..7918cc865d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -12,7 +12,16 @@
 
 int app_dpdk_test_tp_count;
 
-#ifdef RTE_EXEC_ENV_WINDOWS
+#if !defined(RTE_TRACE)
+
+static int
+test_trace(void)
+{
+	printf("trace omitted at build-time, skipping test\n");
+	return TEST_SKIPPED;
+}
+
+#elif defined(RTE_EXEC_ENV_WINDOWS)
 
 static int
 test_trace(void)
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..fd864125a7 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -42,6 +42,8 @@ typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
  */
 #define RTE_TRACE_POINT_ARGS
 
+#ifdef RTE_TRACE
+
 /** @internal Helper macro to support RTE_TRACE_POINT and RTE_TRACE_POINT_FP */
 #define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
 extern rte_trace_point_t __##_tp; \
@@ -100,6 +102,20 @@ _tp _args \
 #define RTE_TRACE_POINT_FP(tp, args, ...) \
 	__RTE_TRACE_POINT(fp, tp, args, __VA_ARGS__)
 
+#else
+
+/** @internal Helper macro to support RTE_TRACE_POINT and RTE_TRACE_POINT_FP */
+#define __RTE_TRACE_POINT_VOID(_tp, ...) \
+static __rte_always_inline void \
+_tp(...) \
+{ \
+}
+
+#define RTE_TRACE_POINT(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
+#define RTE_TRACE_POINT_FP(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
+
+#endif /* RTE_TRACE */
+
 #ifdef __DOXYGEN__
 
 /**
@@ -212,6 +228,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+#ifdef RTE_TRACE
 /**
  * @internal
  *
@@ -230,6 +247,7 @@ __rte_trace_point_fp_is_enabled(void)
 	return false;
 #endif
 }
+#endif /* RTE_TRACE */
 
 /**
  * @internal
@@ -356,6 +374,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 	return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
 }
 
+#ifdef RTE_TRACE
+
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
@@ -411,7 +431,7 @@ do { \
 	RTE_SET_USED(len); \
 } while (0)
 
-
+#endif /* RTE_TRACE */
 #endif /* ALLOW_EXPERIMENTAL_API */
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..567fce534e 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -18,6 +18,8 @@ extern "C" {
 
 RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
+#ifdef RTE_TRACE
+
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
@@ -56,6 +58,21 @@ do { \
 		RTE_STR(uint8_t)); \
 } while (0)
 
+#else
+
+#define RTE_TRACE_POINT_REGISTER(...)
+#define __rte_trace_point_emit_header_generic(t) RTE_SET_USED(t)
+#define __rte_trace_point_emit_header_fp(t) RTE_SET_USED(t)
+#define __rte_trace_point_emit(in, type) RTE_SET_USED(in)
+#define rte_trace_point_emit_string(in) RTE_SET_USED(in)
+#define rte_trace_point_emit_blob(in, len) \
+do { \
+	RTE_SET_USED(in); \
+	RTE_SET_USED(len); \
+} while (0)
+
+#endif /* RTE_TRACE */
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.43.0

Re: [PATCH v2] eal: add build-time option to omit trace

[Jerin Jacob]

On Thu, Sep 19, 2024 at 1:37 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v2:
> * Added/modified macros required for building applications with trace
>   omitted.
> ---
>  app/test/test_trace.c                      | 11 ++++++++++-
>  config/rte_config.h                        |  1 +
>  lib/eal/include/rte_trace_point.h          | 22 +++++++++++++++++++++-
>  lib/eal/include/rte_trace_point_register.h | 17 +++++++++++++++++
>  4 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> index 00809f433b..7918cc865d 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -12,7 +12,16 @@
>
>  int app_dpdk_test_tp_count;
>
> -#ifdef RTE_EXEC_ENV_WINDOWS
> +#if !defined(RTE_TRACE)
> +
> +static int
> +test_trace(void)
> +{
> +       printf("trace omitted at build-time, skipping test\n");
> +       return TEST_SKIPPED;
> +}
> +
> +#elif defined(RTE_EXEC_ENV_WINDOWS)
>
>  static int
>  test_trace(void)
> diff --git a/config/rte_config.h b/config/rte_config.h
> index dd7bb0d35b..fd6f8a2f1a 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -49,6 +49,7 @@
>  #define RTE_MAX_TAILQ 32
>  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
>  #define RTE_MAX_VFIO_CONTAINERS 64
> +#define RTE_TRACE 1


Too many ifdefs down using this, IMO, This approach will make
difficult to manage the code and make sure
 RTE_TRACE 0 works always.

I understand your concern about to reduce the expose attach surface.
BTW, it will be there with telemetry too.
Disabling full feature  for the code via config/rte_config.h is new to
DPDK. I think, we need wider opinion on this(Adding techboard).
Also, DPDK never concerned with binary size. Other than binary size,
following patch and exposed the config through meson
will fix the other problems. IMO, That's the correct tradeoff.


#define __rte_trace_point_emit_header_generic(t) \
void *mem; \
do { \
  +      if (RTE_TRACE == 0) \
  +             return \
        const uint64_t val = rte_atomic_load_explicit(t,
rte_memory_order_acquire); \
        if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
                return; \
        mem = __rte_trace_mem_get(val); \
        if (unlikely(mem == NULL)) \
                return; \
        mem = __rte_trace_point_emit_ev_header(mem, val); \
} while (0)

>
>  /* bsd module defines */
>  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
> index 41e2a7f99e..fd864125a7 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -42,6 +42,8 @@ typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
>   */
>  #define RTE_TRACE_POINT_ARGS
>
> +#ifdef RTE_TRACE
> +
>  /** @internal Helper macro to support RTE_TRACE_POINT and RTE_TRACE_POINT_FP */
>  #define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
>  extern rte_trace_point_t __##_tp; \
> @@ -100,6 +102,20 @@ _tp _args \
>  #define RTE_TRACE_POINT_FP(tp, args, ...) \
>         __RTE_TRACE_POINT(fp, tp, args, __VA_ARGS__)
>
> +#else
> +
> +/** @internal Helper macro to support RTE_TRACE_POINT and RTE_TRACE_POINT_FP */
> +#define __RTE_TRACE_POINT_VOID(_tp, ...) \
> +static __rte_always_inline void \
> +_tp(...) \
> +{ \
> +}
> +
> +#define RTE_TRACE_POINT(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
> +#define RTE_TRACE_POINT_FP(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
> +
> +#endif /* RTE_TRACE */
> +
>  #ifdef __DOXYGEN__
>
>  /**
> @@ -212,6 +228,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
>  __rte_experimental
>  rte_trace_point_t *rte_trace_point_lookup(const char *name);
>
> +#ifdef RTE_TRACE
>  /**
>   * @internal
>   *
> @@ -230,6 +247,7 @@ __rte_trace_point_fp_is_enabled(void)
>         return false;
>  #endif
>  }
> +#endif /* RTE_TRACE */
>
>  /**
>   * @internal
> @@ -356,6 +374,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
>         return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
>  }
>
> +#ifdef RTE_TRACE
> +
>  #define __rte_trace_point_emit_header_generic(t) \
>  void *mem; \
>  do { \
> @@ -411,7 +431,7 @@ do { \
>         RTE_SET_USED(len); \
>  } while (0)
>
> -
> +#endif /* RTE_TRACE */
>  #endif /* ALLOW_EXPERIMENTAL_API */
>  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>
> diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
> index 41260e5964..567fce534e 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -18,6 +18,8 @@ extern "C" {
>
>  RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
>
> +#ifdef RTE_TRACE
> +
>  #define RTE_TRACE_POINT_REGISTER(trace, name) \
>  rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
>  static const char __##trace##_name[] = RTE_STR(name); \
> @@ -56,6 +58,21 @@ do { \
>                 RTE_STR(uint8_t)); \
>  } while (0)
>
> +#else
> +
> +#define RTE_TRACE_POINT_REGISTER(...)
> +#define __rte_trace_point_emit_header_generic(t) RTE_SET_USED(t)
> +#define __rte_trace_point_emit_header_fp(t) RTE_SET_USED(t)
> +#define __rte_trace_point_emit(in, type) RTE_SET_USED(in)
> +#define rte_trace_point_emit_string(in) RTE_SET_USED(in)
> +#define rte_trace_point_emit_blob(in, len) \
> +do { \
> +       RTE_SET_USED(in); \
> +       RTE_SET_USED(len); \
> +} while (0)
> +
> +#endif /* RTE_TRACE */
> +
>  #ifdef __cplusplus
>  }
>  #endif
> --
> 2.43.0
>

RE: [PATCH v2] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Thursday, 19 September 2024 10.52
> 
> On Thu, Sep 19, 2024 at 1:37 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Some applications want to omit the trace feature.
> > Either to reduce the memory footprint, to reduce the exposed attack
> > surface, or for other reasons.
> >
> > This patch adds an option in rte_config.h to include or omit trace in the
> > build. Trace is included by default.
> >
> > Omitting trace works by omitting all trace points.
> > For API and ABI compatibility, the trace feature itself remains.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v2:
> > * Added/modified macros required for building applications with trace
> >   omitted.
> > ---
> >  app/test/test_trace.c                      | 11 ++++++++++-
> >  config/rte_config.h                        |  1 +
> >  lib/eal/include/rte_trace_point.h          | 22 +++++++++++++++++++++-
> >  lib/eal/include/rte_trace_point_register.h | 17 +++++++++++++++++
> >  4 files changed, 49 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> > index 00809f433b..7918cc865d 100644
> > --- a/app/test/test_trace.c
> > +++ b/app/test/test_trace.c
> > @@ -12,7 +12,16 @@
> >
> >  int app_dpdk_test_tp_count;
> >
> > -#ifdef RTE_EXEC_ENV_WINDOWS
> > +#if !defined(RTE_TRACE)
> > +
> > +static int
> > +test_trace(void)
> > +{
> > +       printf("trace omitted at build-time, skipping test\n");
> > +       return TEST_SKIPPED;
> > +}
> > +
> > +#elif defined(RTE_EXEC_ENV_WINDOWS)
> >
> >  static int
> >  test_trace(void)
> > diff --git a/config/rte_config.h b/config/rte_config.h
> > index dd7bb0d35b..fd6f8a2f1a 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -49,6 +49,7 @@
> >  #define RTE_MAX_TAILQ 32
> >  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
> >  #define RTE_MAX_VFIO_CONTAINERS 64
> > +#define RTE_TRACE 1
> 
> 
> Too many ifdefs down using this, IMO, This approach will make
> difficult to manage the code and make sure
>  RTE_TRACE 0 works always.

I agree it's not pretty, but it's the best solution I can come up with, to meet the requirements driving this patch: reducing attack surface and memory footprint.

> 
> I understand your concern about to reduce the expose attach surface.
> BTW, it will be there with telemetry too.

Yes, I have telemetry in mind too. :-)
Got to work on them one by one, though.

> Disabling full feature  for the code via config/rte_config.h is new to
> DPDK. I think, we need wider opinion on this(Adding techboard).

Yes, it’s a good topic for discussion in the techboard.

Currently, there are three levels of build-time configuration:
1. Meson, for prominent options.
2. config/rte_config.h for less prominent options, e.g. enabling RTE_ASSERT(), mempool statistics, etc.
3. Header files in various modules/drivers, for tweaking the individual module.

There didn't seem to be any interest in enabling/disabling trace via Meson, so I chose the next level of configuration.

> Also, DPDK never concerned with binary size.

That is a big mistake. DPDK is not only for cloud.
DPDK should also be for CPEs, SOHO firewalls, and similar network appliances. For some of these high-volume network appliances, memory footprint matters.

Also, when running many instances of a service in the cloud, power consumption matters. Bloat increases power consumption.

> Other than binary size,
> following patch and exposed the config through meson
> will fix the other problems. IMO, That's the correct tradeoff.

Yes; the optimal solution would be making EAL less bloated, i.e. purely an abstraction layer to the underlying hardware and O/S.
This would make trace an independent module, which could be enabled/disabled through Meson.

Bruce has contributed a lot in this area, and already made many modules optional.

> 
> 
> #define __rte_trace_point_emit_header_generic(t) \
> void *mem; \
> do { \
>   +      if (RTE_TRACE == 0) \
>   +             return \
>         const uint64_t val = rte_atomic_load_explicit(t,
> rte_memory_order_acquire); \
>         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
>                 return; \
>         mem = __rte_trace_mem_get(val); \
>         if (unlikely(mem == NULL)) \
>                 return; \
>         mem = __rte_trace_point_emit_ev_header(mem, val); \
> } while (0)
> 
> >
> >  /* bsd module defines */
> >  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> > diff --git a/lib/eal/include/rte_trace_point.h
> b/lib/eal/include/rte_trace_point.h
> > index 41e2a7f99e..fd864125a7 100644
> > --- a/lib/eal/include/rte_trace_point.h
> > +++ b/lib/eal/include/rte_trace_point.h
> > @@ -42,6 +42,8 @@ typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
> >   */
> >  #define RTE_TRACE_POINT_ARGS
> >
> > +#ifdef RTE_TRACE
> > +
> >  /** @internal Helper macro to support RTE_TRACE_POINT and
> RTE_TRACE_POINT_FP */
> >  #define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
> >  extern rte_trace_point_t __##_tp; \
> > @@ -100,6 +102,20 @@ _tp _args \
> >  #define RTE_TRACE_POINT_FP(tp, args, ...) \
> >         __RTE_TRACE_POINT(fp, tp, args, __VA_ARGS__)
> >
> > +#else
> > +
> > +/** @internal Helper macro to support RTE_TRACE_POINT and
> RTE_TRACE_POINT_FP */
> > +#define __RTE_TRACE_POINT_VOID(_tp, ...) \
> > +static __rte_always_inline void \
> > +_tp(...) \
> > +{ \
> > +}
> > +
> > +#define RTE_TRACE_POINT(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
> > +#define RTE_TRACE_POINT_FP(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
> > +
> > +#endif /* RTE_TRACE */
> > +
> >  #ifdef __DOXYGEN__
> >
> >  /**
> > @@ -212,6 +228,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
> >  __rte_experimental
> >  rte_trace_point_t *rte_trace_point_lookup(const char *name);
> >
> > +#ifdef RTE_TRACE
> >  /**
> >   * @internal
> >   *
> > @@ -230,6 +247,7 @@ __rte_trace_point_fp_is_enabled(void)
> >         return false;
> >  #endif
> >  }
> > +#endif /* RTE_TRACE */
> >
> >  /**
> >   * @internal
> > @@ -356,6 +374,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
> >         return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
> >  }
> >
> > +#ifdef RTE_TRACE
> > +
> >  #define __rte_trace_point_emit_header_generic(t) \
> >  void *mem; \
> >  do { \
> > @@ -411,7 +431,7 @@ do { \
> >         RTE_SET_USED(len); \
> >  } while (0)
> >
> > -
> > +#endif /* RTE_TRACE */
> >  #endif /* ALLOW_EXPERIMENTAL_API */
> >  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
> >
> > diff --git a/lib/eal/include/rte_trace_point_register.h
> b/lib/eal/include/rte_trace_point_register.h
> > index 41260e5964..567fce534e 100644
> > --- a/lib/eal/include/rte_trace_point_register.h
> > +++ b/lib/eal/include/rte_trace_point_register.h
> > @@ -18,6 +18,8 @@ extern "C" {
> >
> >  RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
> >
> > +#ifdef RTE_TRACE
> > +
> >  #define RTE_TRACE_POINT_REGISTER(trace, name) \
> >  rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
> >  static const char __##trace##_name[] = RTE_STR(name); \
> > @@ -56,6 +58,21 @@ do { \
> >                 RTE_STR(uint8_t)); \
> >  } while (0)
> >
> > +#else
> > +
> > +#define RTE_TRACE_POINT_REGISTER(...)
> > +#define __rte_trace_point_emit_header_generic(t) RTE_SET_USED(t)
> > +#define __rte_trace_point_emit_header_fp(t) RTE_SET_USED(t)
> > +#define __rte_trace_point_emit(in, type) RTE_SET_USED(in)
> > +#define rte_trace_point_emit_string(in) RTE_SET_USED(in)
> > +#define rte_trace_point_emit_blob(in, len) \
> > +do { \
> > +       RTE_SET_USED(in); \
> > +       RTE_SET_USED(len); \
> > +} while (0)
> > +
> > +#endif /* RTE_TRACE */
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > --
> > 2.43.0
> >

Re: [PATCH v2] eal: add build-time option to omit trace

[Jerin Jacob]

On Thu, Sep 19, 2024 at 6:45 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Thursday, 19 September 2024 10.52
> >
> > On Thu, Sep 19, 2024 at 1:37 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > Some applications want to omit the trace feature.
> > > Either to reduce the memory footprint, to reduce the exposed attack
> > > surface, or for other reasons.
> > >
> > > This patch adds an option in rte_config.h to include or omit trace in the
> > > build. Trace is included by default.
> > >
> > > Omitting trace works by omitting all trace points.
> > > For API and ABI compatibility, the trace feature itself remains.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > > v2:
> > > * Added/modified macros required for building applications with trace
> > >   omitted.
> > > ---
> > >  app/test/test_trace.c                      | 11 ++++++++++-
> > >  config/rte_config.h                        |  1 +
> > >  lib/eal/include/rte_trace_point.h          | 22 +++++++++++++++++++++-
> > >  lib/eal/include/rte_trace_point_register.h | 17 +++++++++++++++++
> > >  4 files changed, 49 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> > > index 00809f433b..7918cc865d 100644
> > > --- a/app/test/test_trace.c
> > > +++ b/app/test/test_trace.c
> > > @@ -12,7 +12,16 @@
> > >
> > >  int app_dpdk_test_tp_count;
> > >
> > > -#ifdef RTE_EXEC_ENV_WINDOWS
> > > +#if !defined(RTE_TRACE)
> > > +
> > > +static int
> > > +test_trace(void)
> > > +{
> > > +       printf("trace omitted at build-time, skipping test\n");
> > > +       return TEST_SKIPPED;
> > > +}
> > > +
> > > +#elif defined(RTE_EXEC_ENV_WINDOWS)
> > >
> > >  static int
> > >  test_trace(void)
> > > diff --git a/config/rte_config.h b/config/rte_config.h
> > > index dd7bb0d35b..fd6f8a2f1a 100644
> > > --- a/config/rte_config.h
> > > +++ b/config/rte_config.h
> > > @@ -49,6 +49,7 @@
> > >  #define RTE_MAX_TAILQ 32
> > >  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
> > >  #define RTE_MAX_VFIO_CONTAINERS 64
> > > +#define RTE_TRACE 1
> >
> >
> > Too many ifdefs down using this, IMO, This approach will make
> > difficult to manage the code and make sure
> >  RTE_TRACE 0 works always.
>
> I agree it's not pretty, but it's the best solution I can come up with, to meet the requirements driving this patch: reducing attack surface and memory footprint.
>
> >
> > I understand your concern about to reduce the expose attach surface.
> > BTW, it will be there with telemetry too.
>
> Yes, I have telemetry in mind too. :-)
> Got to work on them one by one, though.
>
> > Disabling full feature  for the code via config/rte_config.h is new to
> > DPDK. I think, we need wider opinion on this(Adding techboard).
>
> Yes, it’s a good topic for discussion in the techboard.
>
> Currently, there are three levels of build-time configuration:
> 1. Meson, for prominent options.
> 2. config/rte_config.h for less prominent options, e.g. enabling RTE_ASSERT(), mempool statistics, etc.
> 3. Header files in various modules/drivers, for tweaking the individual module.
>
> There didn't seem to be any interest in enabling/disabling trace via Meson, so I chose the next level of configuration.
>
> > Also, DPDK never concerned with binary size.
>
> That is a big mistake. DPDK is not only for cloud.
> DPDK should also be for CPEs, SOHO firewalls, and similar network appliances. For some of these high-volume network appliances, memory footprint matters.

Could you share data with a real-world application of the elf size?
1)Without any change
2)Only disabling via __rte_trace_point_emit_header_generic() .. aka below patch.
3)Full disable.

I think, size command will spit out with different section's size.
This data can be used to take decision and to know how much % it
adding up?


>
> Also, when running many instances of a service in the cloud, power consumption matters. Bloat increases power consumption.
>
> > Other than binary size,
> > following patch and exposed the config through meson
> > will fix the other problems. IMO, That's the correct tradeoff.
>
> Yes; the optimal solution would be making EAL less bloated, i.e. purely an abstraction layer to the underlying hardware and O/S.
> This would make trace an independent module, which could be enabled/disabled through Meson.
>
> Bruce has contributed a lot in this area, and already made many modules optional.
>
> >
> >
> > #define __rte_trace_point_emit_header_generic(t) \
> > void *mem; \
> > do { \
> >   +      if (RTE_TRACE == 0) \
> >   +             return \
> >         const uint64_t val = rte_atomic_load_explicit(t,
> > rte_memory_order_acquire); \
> >         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
> >                 return; \
> >         mem = __rte_trace_mem_get(val); \
> >         if (unlikely(mem == NULL)) \
> >                 return; \
> >         mem = __rte_trace_point_emit_ev_header(mem, val); \
> > } while (0)
> >
> > >
> > >  /* bsd module defines */
> > >  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> > > diff --git a/lib/eal/include/rte_trace_point.h
> > b/lib/eal/include/rte_trace_point.h
> > > index 41e2a7f99e..fd864125a7 100644
> > > --- a/lib/eal/include/rte_trace_point.h
> > > +++ b/lib/eal/include/rte_trace_point.h
> > > @@ -42,6 +42,8 @@ typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
> > >   */
> > >  #define RTE_TRACE_POINT_ARGS
> > >
> > > +#ifdef RTE_TRACE
> > > +
> > >  /** @internal Helper macro to support RTE_TRACE_POINT and
> > RTE_TRACE_POINT_FP */
> > >  #define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
> > >  extern rte_trace_point_t __##_tp; \
> > > @@ -100,6 +102,20 @@ _tp _args \
> > >  #define RTE_TRACE_POINT_FP(tp, args, ...) \
> > >         __RTE_TRACE_POINT(fp, tp, args, __VA_ARGS__)
> > >
> > > +#else
> > > +
> > > +/** @internal Helper macro to support RTE_TRACE_POINT and
> > RTE_TRACE_POINT_FP */
> > > +#define __RTE_TRACE_POINT_VOID(_tp, ...) \
> > > +static __rte_always_inline void \
> > > +_tp(...) \
> > > +{ \
> > > +}
> > > +
> > > +#define RTE_TRACE_POINT(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
> > > +#define RTE_TRACE_POINT_FP(tp, args, ...) __RTE_TRACE_POINT_VOID(tp, args)
> > > +
> > > +#endif /* RTE_TRACE */
> > > +
> > >  #ifdef __DOXYGEN__
> > >
> > >  /**
> > > @@ -212,6 +228,7 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
> > >  __rte_experimental
> > >  rte_trace_point_t *rte_trace_point_lookup(const char *name);
> > >
> > > +#ifdef RTE_TRACE
> > >  /**
> > >   * @internal
> > >   *
> > > @@ -230,6 +247,7 @@ __rte_trace_point_fp_is_enabled(void)
> > >         return false;
> > >  #endif
> > >  }
> > > +#endif /* RTE_TRACE */
> > >
> > >  /**
> > >   * @internal
> > > @@ -356,6 +374,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
> > >         return RTE_PTR_ADD(mem, __RTE_TRACE_EVENT_HEADER_SZ);
> > >  }
> > >
> > > +#ifdef RTE_TRACE
> > > +
> > >  #define __rte_trace_point_emit_header_generic(t) \
> > >  void *mem; \
> > >  do { \
> > > @@ -411,7 +431,7 @@ do { \
> > >         RTE_SET_USED(len); \
> > >  } while (0)
> > >
> > > -
> > > +#endif /* RTE_TRACE */
> > >  #endif /* ALLOW_EXPERIMENTAL_API */
> > >  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
> > >
> > > diff --git a/lib/eal/include/rte_trace_point_register.h
> > b/lib/eal/include/rte_trace_point_register.h
> > > index 41260e5964..567fce534e 100644
> > > --- a/lib/eal/include/rte_trace_point_register.h
> > > +++ b/lib/eal/include/rte_trace_point_register.h
> > > @@ -18,6 +18,8 @@ extern "C" {
> > >
> > >  RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
> > >
> > > +#ifdef RTE_TRACE
> > > +
> > >  #define RTE_TRACE_POINT_REGISTER(trace, name) \
> > >  rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
> > >  static const char __##trace##_name[] = RTE_STR(name); \
> > > @@ -56,6 +58,21 @@ do { \
> > >                 RTE_STR(uint8_t)); \
> > >  } while (0)
> > >
> > > +#else
> > > +
> > > +#define RTE_TRACE_POINT_REGISTER(...)
> > > +#define __rte_trace_point_emit_header_generic(t) RTE_SET_USED(t)
> > > +#define __rte_trace_point_emit_header_fp(t) RTE_SET_USED(t)
> > > +#define __rte_trace_point_emit(in, type) RTE_SET_USED(in)
> > > +#define rte_trace_point_emit_string(in) RTE_SET_USED(in)
> > > +#define rte_trace_point_emit_blob(in, len) \
> > > +do { \
> > > +       RTE_SET_USED(in); \
> > > +       RTE_SET_USED(len); \
> > > +} while (0)
> > > +
> > > +#endif /* RTE_TRACE */
> > > +
> > >  #ifdef __cplusplus
> > >  }
> > >  #endif
> > > --
> > > 2.43.0
> > >

RE: [PATCH v2] eal: add build-time option to omit trace

[Morten Brørup]

> Could you share data with a real-world application of the elf size?
> 1)Without any change

Size of the statically linked executable on the target file system:
3,800,528 byte

> 2)Only disabling via __rte_trace_point_emit_header_generic() .. aka below
> patch.

3,572,032 byte

> 3)Full disable.

3,572,032 byte

> 
> I think, size command will spit out with different section's size.
> This data can be used to take decision and to know how much % it
> adding up?

I.e. trace adds 228,496 byte = 6.4 % to the size of this specific executable.

~200 KB might not seem as much, but this is just omitting one module.
If other unused modules also add ~200 KB, it adds up.

And I haven't done any significant additional memory footprint tuning.


> > > #define __rte_trace_point_emit_header_generic(t) \
> > > void *mem; \
> > > do { \
> > >   +      if (RTE_TRACE == 0) \
> > >   +             return \

Tested without the RTE_TRACE==0 check, simply:
  void *mem; \
  do { \
+         return; \
          const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \

> > >         const uint64_t val = rte_atomic_load_explicit(t,
> > > rte_memory_order_acquire); \
> > >         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
> > >                 return; \
> > >         mem = __rte_trace_mem_get(val); \
> > >         if (unlikely(mem == NULL)) \
> > >                 return; \
> > >         mem = __rte_trace_point_emit_ev_header(mem, val); \
> > > } while (0)

I don't understand why your method uses as little memory as mine...
My method should also omit the trace points themselves, with their names and numbers, and their initialization.

I haven't looked deeper into it.

If your method of omitting trace is as efficient as all my ifdefs, I also prefer your method.
Simpler is better.

Re: [PATCH v2] eal: add build-time option to omit trace

[Jerin Jacob]

On Thu, Sep 19, 2024 at 9:05 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > Could you share data with a real-world application of the elf size?
> > 1)Without any change
>
> Size of the statically linked executable on the target file system:
> 3,800,528 byte
>
> > 2)Only disabling via __rte_trace_point_emit_header_generic() .. aka below
> > patch.
>
> 3,572,032 byte
>
> > 3)Full disable.
>
> 3,572,032 byte
>
> >
> > I think, size command will spit out with different section's size.
> > This data can be used to take decision and to know how much % it
> > adding up?
>
> I.e. trace adds 228,496 byte = 6.4 % to the size of this specific executable.
>
> ~200 KB might not seem as much, but this is just omitting one module.
> If other unused modules also add ~200 KB, it adds up.
>
> And I haven't done any significant additional memory footprint tuning.
>
>
> > > > #define __rte_trace_point_emit_header_generic(t) \
> > > > void *mem; \
> > > > do { \
> > > >   +      if (RTE_TRACE == 0) \
> > > >   +             return \
>
> Tested without the RTE_TRACE==0 check, simply:
>   void *mem; \
>   do { \
> +         return; \
>           const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
>
> > > >         const uint64_t val = rte_atomic_load_explicit(t,
> > > > rte_memory_order_acquire); \
> > > >         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
> > > >                 return; \
> > > >         mem = __rte_trace_mem_get(val); \
> > > >         if (unlikely(mem == NULL)) \
> > > >                 return; \
> > > >         mem = __rte_trace_point_emit_ev_header(mem, val); \
> > > > } while (0)
>
> I don't understand why your method uses as little memory as mine...

Compiler is start enough to understand those functions are NOP.


> My method should also omit the trace points themselves, with their names and numbers, and their initialization.
>
> I haven't looked deeper into it.
>
> If your method of omitting trace is as efficient as all my ifdefs, I also prefer your method.
> Simpler is better.
>

RE: [PATCH v2] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Thursday, 19 September 2024 17.49
> 
> On Thu, Sep 19, 2024 at 9:05 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > Could you share data with a real-world application of the elf size?
> > > 1)Without any change
> >
> > Size of the statically linked executable on the target file system:
> > 3,800,528 byte
> >
> > > 2)Only disabling via __rte_trace_point_emit_header_generic() .. aka below
> > > patch.
> >
> > 3,572,032 byte
> >
> > > 3)Full disable.
> >
> > 3,572,032 byte
> >
> > >
> > > I think, size command will spit out with different section's size.
> > > This data can be used to take decision and to know how much % it
> > > adding up?
> >
> > I.e. trace adds 228,496 byte = 6.4 % to the size of this specific
> executable.
> >
> > ~200 KB might not seem as much, but this is just omitting one module.
> > If other unused modules also add ~200 KB, it adds up.
> >
> > And I haven't done any significant additional memory footprint tuning.
> >
> >
> > > > > #define __rte_trace_point_emit_header_generic(t) \
> > > > > void *mem; \
> > > > > do { \
> > > > >   +      if (RTE_TRACE == 0) \
> > > > >   +             return \
> >
> > Tested without the RTE_TRACE==0 check, simply:
> >   void *mem; \
> >   do { \
> > +         return; \
> >           const uint64_t val = rte_atomic_load_explicit(t,
> rte_memory_order_acquire); \
> >
> > > > >         const uint64_t val = rte_atomic_load_explicit(t,
> > > > > rte_memory_order_acquire); \
> > > > >         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
> > > > >                 return; \
> > > > >         mem = __rte_trace_mem_get(val); \
> > > > >         if (unlikely(mem == NULL)) \
> > > > >                 return; \
> > > > >         mem = __rte_trace_point_emit_ev_header(mem, val); \
> > > > > } while (0)
> >
> > I don't understand why your method uses as little memory as mine...
> 
> Compiler is start enough to understand those functions are NOP.

Yes, I understand why the compiler understands that this code is NOP.

But I don't understand why the tracepoints' values and names are not registered by the unmodified RTE_TRACE_POINT_REGISTER().

> 
> 
> > My method should also omit the trace points themselves, with their names and
> numbers, and their initialization.
> >
> > I haven't looked deeper into it.
> >
> > If your method of omitting trace is as efficient as all my ifdefs, I also
> prefer your method.
> > Simpler is better.
> >

Re: [PATCH v2] eal: add build-time option to omit trace

[Jerin Jacob]

On Thu, Sep 19, 2024 at 9:38 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Thursday, 19 September 2024 17.49
> >
> > On Thu, Sep 19, 2024 at 9:05 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > Could you share data with a real-world application of the elf size?
> > > > 1)Without any change
> > >
> > > Size of the statically linked executable on the target file system:
> > > 3,800,528 byte
> > >
> > > > 2)Only disabling via __rte_trace_point_emit_header_generic() .. aka below
> > > > patch.
> > >
> > > 3,572,032 byte
> > >
> > > > 3)Full disable.
> > >
> > > 3,572,032 byte
> > >
> > > >
> > > > I think, size command will spit out with different section's size.
> > > > This data can be used to take decision and to know how much % it
> > > > adding up?
> > >
> > > I.e. trace adds 228,496 byte = 6.4 % to the size of this specific
> > executable.
> > >
> > > ~200 KB might not seem as much, but this is just omitting one module.
> > > If other unused modules also add ~200 KB, it adds up.
> > >
> > > And I haven't done any significant additional memory footprint tuning.
> > >
> > >
> > > > > > #define __rte_trace_point_emit_header_generic(t) \
> > > > > > void *mem; \
> > > > > > do { \
> > > > > >   +      if (RTE_TRACE == 0) \
> > > > > >   +             return \
> > >
> > > Tested without the RTE_TRACE==0 check, simply:
> > >   void *mem; \
> > >   do { \
> > > +         return; \
> > >           const uint64_t val = rte_atomic_load_explicit(t,
> > rte_memory_order_acquire); \
> > >
> > > > > >         const uint64_t val = rte_atomic_load_explicit(t,
> > > > > > rte_memory_order_acquire); \
> > > > > >         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
> > > > > >                 return; \
> > > > > >         mem = __rte_trace_mem_get(val); \
> > > > > >         if (unlikely(mem == NULL)) \
> > > > > >                 return; \
> > > > > >         mem = __rte_trace_point_emit_ev_header(mem, val); \
> > > > > > } while (0)
> > >
> > > I don't understand why your method uses as little memory as mine...
> >
> > Compiler is start enough to understand those functions are NOP.
>
> Yes, I understand why the compiler understands that this code is NOP.
>
> But I don't understand why the tracepoints' values and names are not registered by the unmodified RTE_TRACE_POINT_REGISTER().

My best _guess_ is __rte_trace_point_register()'s register_fn()
execution comes as NOP.




>
> >
> >
> > > My method should also omit the trace points themselves, with their names and
> > numbers, and their initialization.
> > >
> > > I haven't looked deeper into it.
> > >
> > > If your method of omitting trace is as efficient as all my ifdefs, I also
> > prefer your method.
> > > Simpler is better.
> > >

[PATCH v3] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c             | 11 ++++++++++-
 config/rte_config.h               |  1 +
 lib/eal/include/rte_trace_point.h | 21 +++++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..7918cc865d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -12,7 +12,16 @@
 
 int app_dpdk_test_tp_count;
 
-#ifdef RTE_EXEC_ENV_WINDOWS
+#if !defined(RTE_TRACE)
+
+static int
+test_trace(void)
+{
+	printf("trace omitted at build-time, skipping test\n");
+	return TEST_SKIPPED;
+}
+
+#elif defined(RTE_EXEC_ENV_WINDOWS)
 
 static int
 test_trace(void)
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..b80688ce89 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,25 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+/**
+ * @internal
+ *
+ * Test if the tracepoint compile-time option is enabled.
+ *
+ * @return
+ *   true if tracepoint enabled, false otherwise.
+ */
+__rte_experimental
+static __rte_always_inline bool
+__rte_trace_point_generic_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * @internal
  *
@@ -359,6 +378,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
-- 
2.43.0

Re: [PATCH v3] eal: add build-time option to omit trace

[Jerin Jacob]

On Fri, Sep 20, 2024 at 2:38 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v3:
> * Simpler version with much fewer ifdefs. (Jerin Jacob)
> v2:
> * Added/modified macros required for building applications with trace
>   omitted.
> ---
>  app/test/test_trace.c             | 11 ++++++++++-
>  config/rte_config.h               |  1 +
>  lib/eal/include/rte_trace_point.h | 21 +++++++++++++++++++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> index 00809f433b..7918cc865d 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -12,7 +12,16 @@
>
>  int app_dpdk_test_tp_count;
>
> -#ifdef RTE_EXEC_ENV_WINDOWS
> +#if !defined(RTE_TRACE)

Now that, we are is not disabling any symbols,
This conditional compilation can be removed. Use
__rte_trace_point_generic_is_enabled() in another leg.


> +
> +static int
> +test_trace(void)
> +{
> +       printf("trace omitted at build-time, skipping test\n");
> +       return TEST_SKIPPED;
> +}
> +
> +#elif defined(RTE_EXEC_ENV_WINDOWS)
>
>  static int
>  test_trace(void)
> diff --git a/config/rte_config.h b/config/rte_config.h
> index dd7bb0d35b..fd6f8a2f1a 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -49,6 +49,7 @@
>  #define RTE_MAX_TAILQ 32
>  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
>  #define RTE_MAX_VFIO_CONTAINERS 64
> +#define RTE_TRACE 1
>
>  /* bsd module defines */
>  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
> index 41e2a7f99e..b80688ce89 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -212,6 +212,25 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
>  __rte_experimental
>  rte_trace_point_t *rte_trace_point_lookup(const char *name);
>
> +/**
> + * @internal
> + *
> + * Test if the tracepoint compile-time option is enabled.
> + *
> + * @return
> + *   true if tracepoint enabled, false otherwise.
> + */
> +__rte_experimental
> +static __rte_always_inline bool
> +__rte_trace_point_generic_is_enabled(void)

Add to version.map file

> +{
> +#ifdef RTE_TRACE
> +       return true;
> +#else
> +       return false;
> +#endif
> +}
> +
>  /**
>   * @internal
>   *
> @@ -359,6 +378,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
>  #define __rte_trace_point_emit_header_generic(t) \
>  void *mem; \
>  do { \
> +       if (!__rte_trace_point_generic_is_enabled()) \
> +               return; \


To be super safe, Add similar check in
RTE_INIT(trace##_init)

>         const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
>         if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
>                 return; \
> --
> 2.43.0
>

RE: [PATCH v3] eal: add build-time option to omit trace

[Morten Brørup]

> > +++ b/app/test/test_trace.c
> > @@ -12,7 +12,16 @@
> >
> >  int app_dpdk_test_tp_count;
> >
> > -#ifdef RTE_EXEC_ENV_WINDOWS
> > +#if !defined(RTE_TRACE)
> 
> Now that, we are is not disabling any symbols,
> This conditional compilation can be removed. Use
> __rte_trace_point_generic_is_enabled() in another leg.

Done in v4.
Also added to perf test.


> > +/**
> > + * @internal
> > + *
> > + * Test if the tracepoint compile-time option is enabled.
> > + *
> > + * @return
> > + *   true if tracepoint enabled, false otherwise.
> > + */
> > +__rte_experimental
> > +static __rte_always_inline bool
> > +__rte_trace_point_generic_is_enabled(void)
> 
> Add to version.map file

It's static inline, so not an exposed symbol.
__rte_trace_point_fp_is_enabled is also not in the version.map file.


> >  #define __rte_trace_point_emit_header_generic(t) \
> >  void *mem; \
> >  do { \
> > +       if (!__rte_trace_point_generic_is_enabled()) \
> > +               return; \
> 
> 
> To be super safe, Add similar check in
> RTE_INIT(trace##_init)

Done in v4.

[PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v4:
* Added check for generic trace enabled when registering trace points, in
  RTE_INIT. (Jerin Jacob)
* Test application uses function instead of macro to check if generic
  trace is enabled. (Jerin Jacob)
* Performance test application uses function to check if generic trace is
  enabled.
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      |  4 ++++
 app/test/test_trace_perf.c                 |  5 +++++
 config/rte_config.h                        |  1 +
 lib/eal/include/rte_trace_point.h          | 21 +++++++++++++++++++++
 lib/eal/include/rte_trace_point_register.h |  2 ++
 5 files changed, 33 insertions(+)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..26656fe024 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
 static int
 test_trace(void)
 {
+	if (!__rte_trace_point_generic_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
 	return unit_test_suite_runner(&trace_tests);
 }
 
diff --git a/app/test/test_trace_perf.c b/app/test/test_trace_perf.c
index 8257cc02be..504574ef8a 100644
--- a/app/test/test_trace_perf.c
+++ b/app/test/test_trace_perf.c
@@ -150,6 +150,11 @@ test_trace_perf(void)
 	struct test_data *data;
 	size_t sz;
 
+	if (!__rte_trace_point_generic_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
+
 	nb_cores = rte_lcore_count();
 	nb_workers = nb_cores - 1;
 	if (nb_cores < 2) {
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..b80688ce89 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,25 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+/**
+ * @internal
+ *
+ * Test if the tracepoint compile-time option is enabled.
+ *
+ * @return
+ *   true if tracepoint enabled, false otherwise.
+ */
+__rte_experimental
+static __rte_always_inline bool
+__rte_trace_point_generic_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * @internal
  *
@@ -359,6 +378,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..429b993fc2 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -23,6 +23,8 @@ rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
-- 
2.43.0

Re: [PATCH v4] eal: add build-time option to omit trace

[Stephen Hemminger]

On Tue, 24 Sep 2024 13:39:57 +0000
Morten Brørup <mb@smartsharesystems.com> wrote:

> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
> 
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
> 
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

This is good compact solution. In future, it would be nice if DPDK could
use static keys to isolate rarely used features. But static keys require
runtime modification of instructions.

https://docs.kernel.org/6.7/staging/static-keys.html

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH v4] eal: add build-time option to omit trace

[Jerin Jacob]

On Tue, Sep 24, 2024 at 7:10 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v4:
> * Added check for generic trace enabled when registering trace points, in
>   RTE_INIT. (Jerin Jacob)
> * Test application uses function instead of macro to check if generic
>   trace is enabled. (Jerin Jacob)
> * Performance test application uses function to check if generic trace is
>   enabled.
> v3:
> * Simpler version with much fewer ifdefs. (Jerin Jacob)
> v2:
> * Added/modified macros required for building applications with trace
>   omitted.

>
> +/**
> + * @internal

Since it is used in app/test. Can we remove it as internal and make
symbol as rte_trace_point_is_enabled

> + *
> + * Test if the tracepoint compile-time option is enabled.
> + *
> + * @return
> + *   true if tracepoint enabled, false otherwise.
> + */
> +__rte_experimental
> +static __rte_always_inline bool
> +__rte_trace_point_generic_is_enabled(void)

Do we need to keep _generic_

Rest looks good to me.

RE: [PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 24 September 2024 11.42
> 
> On Tue, Sep 24, 2024 at 7:10 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Some applications want to omit the trace feature.
> > Either to reduce the memory footprint, to reduce the exposed attack
> > surface, or for other reasons.
> >
> > This patch adds an option in rte_config.h to include or omit trace in
> the
> > build. Trace is included by default.
> >
> > Omitting trace works by omitting all trace points.
> > For API and ABI compatibility, the trace feature itself remains.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v4:
> > * Added check for generic trace enabled when registering trace points,
> in
> >   RTE_INIT. (Jerin Jacob)
> > * Test application uses function instead of macro to check if generic
> >   trace is enabled. (Jerin Jacob)
> > * Performance test application uses function to check if generic trace
> is
> >   enabled.
> > v3:
> > * Simpler version with much fewer ifdefs. (Jerin Jacob)
> > v2:
> > * Added/modified macros required for building applications with trace
> >   omitted.
> 
> >
> > +/**
> > + * @internal
> 
> Since it is used in app/test. Can we remove it as internal and make
> symbol as rte_trace_point_is_enabled

I don't think we should make functions public if only used for test purposes.

Do you think it is useful for normal usage too? Does rte_trace_is_enabled() not suffice?

> 
> > + *
> > + * Test if the tracepoint compile-time option is enabled.
> > + *
> > + * @return
> > + *   true if tracepoint enabled, false otherwise.
> > + */
> > +__rte_experimental
> > +static __rte_always_inline bool
> > +__rte_trace_point_generic_is_enabled(void)
> 
> Do we need to keep _generic_

Other internal functions have _generic_ too, so I added it.
If we decide to make it public, I agree _generic_ should be removed.

> 
> Rest looks good to me.

Re: [PATCH v4] eal: add build-time option to omit trace

[Jerin Jacob]

On Tue, Sep 24, 2024 at 9:23 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 24 September 2024 11.42
> >
> > On Tue, Sep 24, 2024 at 7:10 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > Some applications want to omit the trace feature.
> > > Either to reduce the memory footprint, to reduce the exposed attack
> > > surface, or for other reasons.
> > >
> > > This patch adds an option in rte_config.h to include or omit trace in
> > the
> > > build. Trace is included by default.
> > >
> > > Omitting trace works by omitting all trace points.
> > > For API and ABI compatibility, the trace feature itself remains.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > > v4:
> > > * Added check for generic trace enabled when registering trace points,
> > in
> > >   RTE_INIT. (Jerin Jacob)
> > > * Test application uses function instead of macro to check if generic
> > >   trace is enabled. (Jerin Jacob)
> > > * Performance test application uses function to check if generic trace
> > is
> > >   enabled.
> > > v3:
> > > * Simpler version with much fewer ifdefs. (Jerin Jacob)
> > > v2:
> > > * Added/modified macros required for building applications with trace
> > >   omitted.
> >
> > >
> > > +/**
> > > + * @internal
> >
> > Since it is used in app/test. Can we remove it as internal and make
> > symbol as rte_trace_point_is_enabled
>
> I don't think we should make functions public if only used for test purposes.
>
> Do you think it is useful for normal usage too? Does rte_trace_is_enabled() not suffice?

It will be good for an app to know, Is trace feature disabled if the
application cares about.
Yes. rte_trace_is_enabled() suffice.


>
> >
> > > + *
> > > + * Test if the tracepoint compile-time option is enabled.
> > > + *
> > > + * @return
> > > + *   true if tracepoint enabled, false otherwise.
> > > + */
> > > +__rte_experimental
> > > +static __rte_always_inline bool
> > > +__rte_trace_point_generic_is_enabled(void)
> >
> > Do we need to keep _generic_
>
> Other internal functions have _generic_ too, so I added it.
> If we decide to make it public, I agree _generic_ should be removed.
>
> >
> > Rest looks good to me.

RE: [PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

Jerin,

If you have no further comments, please add review/ack tag, to help Thomas see that the patch has been accepted by the maintainer, and can be merged.

Re: [PATCH v4] eal: add build-time option to omit trace

[Jerin Jacob]

On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Jerin,
>
> If you have no further comments, please add review/ack tag, to help Thomas see that the patch has been accepted by the maintainer, and can be merged.

There was a comment to make the function as rte_trace_is_enabled() and
remove internal. The rest looks good to me. I will Ack in the next
version.


>

RE: [PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 1 October 2024 16.05
> 
> On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Jerin,
> >
> > If you have no further comments, please add review/ack tag, to help
> Thomas see that the patch has been accepted by the maintainer, and can
> be merged.
> 
> There was a comment to make the function as rte_trace_is_enabled() and
> remove internal. The rest looks good to me. I will Ack in the next
> version.

Perhaps my reply to that comment was unclear... such a public function already exists in the previous API:
https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace.h#L36

That function tells if trace enabled at both build time and runtime, and returns false if not.

A separate public function to tell if trace is enabled at build time seems like overkill to me. Is that what you are asking for?

Re: [PATCH v4] eal: add build-time option to omit trace

[Jerin Jacob]

On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 1 October 2024 16.05
> >
> > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > Jerin,
> > >
> > > If you have no further comments, please add review/ack tag, to help
> > Thomas see that the patch has been accepted by the maintainer, and can
> > be merged.
> >
> > There was a comment to make the function as rte_trace_is_enabled() and
> > remove internal. The rest looks good to me. I will Ack in the next
> > version.
>
> Perhaps my reply to that comment was unclear... such a public function already exists in the previous API:

I see. It was not clear.

> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace.h#L36
>
> That function tells if trace enabled at both build time and runtime, and returns false if not.
>
> A separate public function to tell if trace is enabled at build time seems like overkill to me. Is that what you are asking for?

No. Just use rte_trace_is_enabled() in app/test instead of
__rte_trace_point_generic_is_enabled() as it is internal.

RE: [PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 1 October 2024 17.02
> 
> On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Tuesday, 1 October 2024 16.05
> > >
> > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > Jerin,
> > > >
> > > > If you have no further comments, please add review/ack tag, to
> help
> > > Thomas see that the patch has been accepted by the maintainer, and
> can
> > > be merged.
> > >
> > > There was a comment to make the function as rte_trace_is_enabled()
> and
> > > remove internal. The rest looks good to me. I will Ack in the next
> > > version.
> >
> > Perhaps my reply to that comment was unclear... such a public
> function already exists in the previous API:
> 
> I see. It was not clear.
> 
> >
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> .h#L36
> >
> > That function tells if trace enabled at both build time and runtime,
> and returns false if not.
> >
> > A separate public function to tell if trace is enabled at build time
> seems like overkill to me. Is that what you are asking for?
> 
> No. Just use rte_trace_is_enabled() in app/test instead of
> __rte_trace_point_generic_is_enabled() as it is internal.

Just tested it, and it didn't have the wanted effect.
I think rte_trace_is_enabled() returns false until at least one tracepoint has been enabled, which seems like a good optimization.
But it also means that we cannot use it to replace __rte_trace_point_generic_is_enabled() in test/app, because no tracepoints have been enabled at this point of execution, so it returns false here.

I looked around in the code, and cannot find a method without looking at internals, or duplicating a test case.

I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns non-NULL, but that would duplicate the same test in test_trace_points_lookup().

What do you think...
Keep using internal function __rte_trace_point_generic_is_enabled(),
test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
or any other idea?

Re: [PATCH v4] eal: add build-time option to omit trace

[Jerin Jacob]

On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 1 October 2024 17.02
> >
> > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > Sent: Tuesday, 1 October 2024 16.05
> > > >
> > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > <mb@smartsharesystems.com>
> > > > wrote:
> > > > >
> > > > > Jerin,
> > > > >
> > > > > If you have no further comments, please add review/ack tag, to
> > help
> > > > Thomas see that the patch has been accepted by the maintainer, and
> > can
> > > > be merged.
> > > >
> > > > There was a comment to make the function as rte_trace_is_enabled()
> > and
> > > > remove internal. The rest looks good to me. I will Ack in the next
> > > > version.
> > >
> > > Perhaps my reply to that comment was unclear... such a public
> > function already exists in the previous API:
> >
> > I see. It was not clear.
> >
> > >
> > https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > .h#L36
> > >
> > > That function tells if trace enabled at both build time and runtime,
> > and returns false if not.
> > >
> > > A separate public function to tell if trace is enabled at build time
> > seems like overkill to me. Is that what you are asking for?
> >
> > No. Just use rte_trace_is_enabled() in app/test instead of
> > __rte_trace_point_generic_is_enabled() as it is internal.
>
> Just tested it, and it didn't have the wanted effect.
> I think rte_trace_is_enabled() returns false until at least one tracepoint has been enabled, which seems like a good optimization.
> But it also means that we cannot use it to replace __rte_trace_point_generic_is_enabled() in test/app, because no tracepoints have been enabled at this point of execution, so it returns false here.
>
> I looked around in the code, and cannot find a method without looking at internals, or duplicating a test case.
>
> I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns non-NULL, but that would duplicate the same test in test_trace_points_lookup().
>
> What do you think...
> Keep using internal function __rte_trace_point_generic_is_enabled(),
> test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> or any other idea?

How about the following, it is anyway the correct thing to do

bool
rte_trace_is_enabled(void)
{
 +       if (__rte_trace_point_generic_is_enabled() == false)
  +              return false;
        return rte_atomic_load_explicit(&trace.status,
rte_memory_order_acquire) != 0;
}


>

RE: [PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Tuesday, 1 October 2024 18.06
> 
> On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Tuesday, 1 October 2024 17.02
> > >
> > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > Sent: Tuesday, 1 October 2024 16.05
> > > > >
> > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > > <mb@smartsharesystems.com>
> > > > > wrote:
> > > > > >
> > > > > > Jerin,
> > > > > >
> > > > > > If you have no further comments, please add review/ack tag,
> to
> > > help
> > > > > Thomas see that the patch has been accepted by the maintainer,
> and
> > > can
> > > > > be merged.
> > > > >
> > > > > There was a comment to make the function as
> rte_trace_is_enabled()
> > > and
> > > > > remove internal. The rest looks good to me. I will Ack in the
> next
> > > > > version.
> > > >
> > > > Perhaps my reply to that comment was unclear... such a public
> > > function already exists in the previous API:
> > >
> > > I see. It was not clear.
> > >
> > > >
> > >
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > > .h#L36
> > > >
> > > > That function tells if trace enabled at both build time and
> runtime,
> > > and returns false if not.
> > > >
> > > > A separate public function to tell if trace is enabled at build
> time
> > > seems like overkill to me. Is that what you are asking for?
> > >
> > > No. Just use rte_trace_is_enabled() in app/test instead of
> > > __rte_trace_point_generic_is_enabled() as it is internal.
> >
> > Just tested it, and it didn't have the wanted effect.
> > I think rte_trace_is_enabled() returns false until at least one
> tracepoint has been enabled, which seems like a good optimization.
> > But it also means that we cannot use it to replace
> __rte_trace_point_generic_is_enabled() in test/app, because no
> tracepoints have been enabled at this point of execution, so it returns
> false here.
> >
> > I looked around in the code, and cannot find a method without looking
> at internals, or duplicating a test case.
> >
> > I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns
> non-NULL, but that would duplicate the same test in
> test_trace_points_lookup().
> >
> > What do you think...
> > Keep using internal function __rte_trace_point_generic_is_enabled(),
> > test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> > or any other idea?
> 
> How about the following, it is anyway the correct thing to do
> 
> bool
> rte_trace_is_enabled(void)
> {
>  +       if (__rte_trace_point_generic_is_enabled() == false)
>   +              return false;
>         return rte_atomic_load_explicit(&trace.status,
> rte_memory_order_acquire) != 0;
> }
> 

It's the opposite that's causing problems:
Even when built with trace, rte_trace_is_enabled() returns false, because no trace points have been enabled when the test application checks if it should run test cases or not. At this point, trace.status is zero, so it skips testing.

We don't need to add the test for rte_trace_point_generic_is_enabled()==false in rte_trace_is_enabled(), because nothing can increase trace.status if no tracepoints exist. (As far as I understand.)

Re: [PATCH v4] eal: add build-time option to omit trace

[Jerin Jacob]

On Tue, Oct 1, 2024 at 9:45 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Tuesday, 1 October 2024 18.06
> >
> > On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > Sent: Tuesday, 1 October 2024 17.02
> > > >
> > > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup
> > <mb@smartsharesystems.com>
> > > > wrote:
> > > > >
> > > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > > Sent: Tuesday, 1 October 2024 16.05
> > > > > >
> > > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > > > <mb@smartsharesystems.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > Jerin,
> > > > > > >
> > > > > > > If you have no further comments, please add review/ack tag,
> > to
> > > > help
> > > > > > Thomas see that the patch has been accepted by the maintainer,
> > and
> > > > can
> > > > > > be merged.
> > > > > >
> > > > > > There was a comment to make the function as
> > rte_trace_is_enabled()
> > > > and
> > > > > > remove internal. The rest looks good to me. I will Ack in the
> > next
> > > > > > version.
> > > > >
> > > > > Perhaps my reply to that comment was unclear... such a public
> > > > function already exists in the previous API:
> > > >
> > > > I see. It was not clear.
> > > >
> > > > >
> > > >
> > https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > > > .h#L36
> > > > >
> > > > > That function tells if trace enabled at both build time and
> > runtime,
> > > > and returns false if not.
> > > > >
> > > > > A separate public function to tell if trace is enabled at build
> > time
> > > > seems like overkill to me. Is that what you are asking for?
> > > >
> > > > No. Just use rte_trace_is_enabled() in app/test instead of
> > > > __rte_trace_point_generic_is_enabled() as it is internal.
> > >
> > > Just tested it, and it didn't have the wanted effect.
> > > I think rte_trace_is_enabled() returns false until at least one
> > tracepoint has been enabled, which seems like a good optimization.
> > > But it also means that we cannot use it to replace
> > __rte_trace_point_generic_is_enabled() in test/app, because no
> > tracepoints have been enabled at this point of execution, so it returns
> > false here.
> > >
> > > I looked around in the code, and cannot find a method without looking
> > at internals, or duplicating a test case.
> > >
> > > I could test if rte_trace_point_lookup("app.dpdk.test.tp") returns
> > non-NULL, but that would duplicate the same test in
> > test_trace_points_lookup().
> > >
> > > What do you think...
> > > Keep using internal function __rte_trace_point_generic_is_enabled(),
> > > test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> > > or any other idea?
> >
> > How about the following, it is anyway the correct thing to do
> >
> > bool
> > rte_trace_is_enabled(void)
> > {
> >  +       if (__rte_trace_point_generic_is_enabled() == false)
> >   +              return false;
> >         return rte_atomic_load_explicit(&trace.status,
> > rte_memory_order_acquire) != 0;
> > }
> >
>
> It's the opposite that's causing problems:
> Even when built with trace, rte_trace_is_enabled() returns false, because no trace points have been enabled when the test application checks if it should run test cases or not. At this point, trace.status is zero, so it skips testing.
>
> We don't need to add the test for rte_trace_point_generic_is_enabled()==false in rte_trace_is_enabled(), because nothing can increase trace.status if no tracepoints exist. (As far as I understand.)

OK. I see. IMO, It is not good to expose internal symbol to app/test.
How about,
Changing __rte_trace_point_generic_is_enabled() as
rte_trace_feature_is_enabled() or similar. This variant is more like
feature is disabled at compiled time or not? And make it as public and
use in app/test.


>

RE: [PATCH v4] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Wednesday, 2 October 2024 18.18
> 
> On Tue, Oct 1, 2024 at 9:45 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > Sent: Tuesday, 1 October 2024 18.06
> > >
> > > On Tue, Oct 1, 2024 at 9:32 PM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > Sent: Tuesday, 1 October 2024 17.02
> > > > >
> > > > > On Tue, Oct 1, 2024 at 7:44 PM Morten Brørup
> > > <mb@smartsharesystems.com>
> > > > > wrote:
> > > > > >
> > > > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > > > > > > Sent: Tuesday, 1 October 2024 16.05
> > > > > > >
> > > > > > > On Tue, Oct 1, 2024 at 7:19 PM Morten Brørup
> > > > > <mb@smartsharesystems.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Jerin,
> > > > > > > >
> > > > > > > > If you have no further comments, please add review/ack
> tag,
> > > to
> > > > > help
> > > > > > > Thomas see that the patch has been accepted by the
> maintainer,
> > > and
> > > > > can
> > > > > > > be merged.
> > > > > > >
> > > > > > > There was a comment to make the function as
> > > rte_trace_is_enabled()
> > > > > and
> > > > > > > remove internal. The rest looks good to me. I will Ack in
> the
> > > next
> > > > > > > version.
> > > > > >
> > > > > > Perhaps my reply to that comment was unclear... such a public
> > > > > function already exists in the previous API:
> > > > >
> > > > > I see. It was not clear.
> > > > >
> > > > > >
> > > > >
> > >
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_trace
> > > > > .h#L36
> > > > > >
> > > > > > That function tells if trace enabled at both build time and
> > > runtime,
> > > > > and returns false if not.
> > > > > >
> > > > > > A separate public function to tell if trace is enabled at
> build
> > > time
> > > > > seems like overkill to me. Is that what you are asking for?
> > > > >
> > > > > No. Just use rte_trace_is_enabled() in app/test instead of
> > > > > __rte_trace_point_generic_is_enabled() as it is internal.
> > > >
> > > > Just tested it, and it didn't have the wanted effect.
> > > > I think rte_trace_is_enabled() returns false until at least one
> > > tracepoint has been enabled, which seems like a good optimization.
> > > > But it also means that we cannot use it to replace
> > > __rte_trace_point_generic_is_enabled() in test/app, because no
> > > tracepoints have been enabled at this point of execution, so it
> returns
> > > false here.
> > > >
> > > > I looked around in the code, and cannot find a method without
> looking
> > > at internals, or duplicating a test case.
> > > >
> > > > I could test if rte_trace_point_lookup("app.dpdk.test.tp")
> returns
> > > non-NULL, but that would duplicate the same test in
> > > test_trace_points_lookup().
> > > >
> > > > What do you think...
> > > > Keep using internal function
> __rte_trace_point_generic_is_enabled(),
> > > > test rte_trace_point_lookup("app.dpdk.test.tp") != NULL,
> > > > or any other idea?
> > >
> > > How about the following, it is anyway the correct thing to do
> > >
> > > bool
> > > rte_trace_is_enabled(void)
> > > {
> > >  +       if (__rte_trace_point_generic_is_enabled() == false)
> > >   +              return false;
> > >         return rte_atomic_load_explicit(&trace.status,
> > > rte_memory_order_acquire) != 0;
> > > }
> > >
> >
> > It's the opposite that's causing problems:
> > Even when built with trace, rte_trace_is_enabled() returns false,
> because no trace points have been enabled when the test application
> checks if it should run test cases or not. At this point, trace.status
> is zero, so it skips testing.
> >
> > We don't need to add the test for
> rte_trace_point_generic_is_enabled()==false in rte_trace_is_enabled(),
> because nothing can increase trace.status if no tracepoints exist. (As
> far as I understand.)
> 
> OK. I see. IMO, It is not good to expose internal symbol to app/test.

Agree. Fixed in v5.

> How about,
> Changing __rte_trace_point_generic_is_enabled() as
> rte_trace_feature_is_enabled() or similar. This variant is more like
> feature is disabled at compiled time or not? And make it as public and
> use in app/test.

In v5, I have added the public function rte_trace_feature_is_enabled(), which returns true iff the application is built with trace enabled (this is tested using inline), and the DPDK - in case it is built separately - is built with trace enabled (this is compiled with the trace lib's C file).

App/test now uses this function instead of the internal one.

__rte_trace_point_generic_is_enabled() needs to remain static inline, due to the way it is used for omitting trace at build time.

PS: Both trace_autotest and trace_perf_autotest have been tested with and without RTE_TRACE, and they behave as expected.

[PATCH v5] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v5:
  Added public function rte_trace_feature_is_enabled(), to test if trace
  is build time enabled in both the DPDK and the application. Use in test
  application instead of private function. (Jerin Jacob)
v4:
* Added check for generic trace enabled when registering trace points, in
  RTE_INIT. (Jerin Jacob)
* Test application uses function instead of macro to check if generic
  trace is enabled. (Jerin Jacob)
* Performance test application uses function to check if generic trace is
  enabled.
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      |  4 +++
 app/test/test_trace_perf.c                 |  6 ++++
 config/rte_config.h                        |  1 +
 lib/eal/common/eal_common_trace.c          | 10 +++++++
 lib/eal/include/rte_trace.h                | 33 ++++++++++++++++++++++
 lib/eal/include/rte_trace_point.h          | 21 ++++++++++++++
 lib/eal/include/rte_trace_point_register.h |  2 ++
 lib/eal/version.map                        |  3 ++
 8 files changed, 80 insertions(+)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..8ea1443044 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
 static int
 test_trace(void)
 {
+	if (!rte_trace_feature_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
 	return unit_test_suite_runner(&trace_tests);
 }
 
diff --git a/app/test/test_trace_perf.c b/app/test/test_trace_perf.c
index 8257cc02be..a805fa171b 100644
--- a/app/test/test_trace_perf.c
+++ b/app/test/test_trace_perf.c
@@ -8,6 +8,7 @@
 #include <rte_eal_trace.h>
 #include <rte_malloc.h>
 #include <rte_lcore.h>
+#include <rte_trace.h>
 
 #include "test.h"
 #include "test_trace.h"
@@ -150,6 +151,11 @@ test_trace_perf(void)
 	struct test_data *data;
 	size_t sz;
 
+	if (!rte_trace_feature_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
+
 	nb_cores = rte_lcore_count();
 	nb_workers = nb_cores - 1;
 	if (nb_cores < 2) {
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 918f49bf4f..06130c756d 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -100,6 +100,16 @@ rte_trace_is_enabled(void)
 	return rte_atomic_load_explicit(&trace.status, rte_memory_order_acquire) != 0;
 }
 
+bool
+__rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 static void
 trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
diff --git a/lib/eal/include/rte_trace.h b/lib/eal/include/rte_trace.h
index a6e991fad3..340df4f8a0 100644
--- a/lib/eal/include/rte_trace.h
+++ b/lib/eal/include/rte_trace.h
@@ -35,6 +35,39 @@ extern "C" {
 __rte_experimental
 bool rte_trace_is_enabled(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * @internal
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+__rte_experimental
+bool __rte_trace_feature_is_enabled(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+static __rte_always_inline
+bool rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return __rte_trace_feature_is_enabled();
+#else
+	return false;
+#endif
+}
+
 /**
  * Enumerate trace mode operation.
  */
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..b80688ce89 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,25 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+/**
+ * @internal
+ *
+ * Test if the tracepoint compile-time option is enabled.
+ *
+ * @return
+ *   true if tracepoint enabled, false otherwise.
+ */
+__rte_experimental
+static __rte_always_inline bool
+__rte_trace_point_generic_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * @internal
  *
@@ -359,6 +378,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..429b993fc2 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -23,6 +23,8 @@ rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
diff --git a/lib/eal/version.map b/lib/eal/version.map
index e3ff412683..15c694c2da 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -396,6 +396,9 @@ EXPERIMENTAL {
 
 	# added in 24.03
 	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
+
+	# added in 24.11
+	__rte_trace_feature_is_enabled;
 };
 
 INTERNAL {
-- 
2.43.0

[PATCH v6] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v6:
  Removed test_trace_perf.c changes; they don't compile for Windows
  target, and are superfluous.
v5:
  Added public function rte_trace_feature_is_enabled(), to test if trace
  is build time enabled in both the DPDK and the application. Use in test
  application instead of private function. (Jerin Jacob)
v4:
* Added check for generic trace enabled when registering trace points, in
  RTE_INIT. (Jerin Jacob)
* Test application uses function instead of macro to check if generic
  trace is enabled. (Jerin Jacob)
* Performance test application uses function to check if generic trace is
  enabled.
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      |  4 +++
 config/rte_config.h                        |  1 +
 lib/eal/common/eal_common_trace.c          | 10 +++++++
 lib/eal/include/rte_trace.h                | 33 ++++++++++++++++++++++
 lib/eal/include/rte_trace_point.h          | 21 ++++++++++++++
 lib/eal/include/rte_trace_point_register.h |  2 ++
 lib/eal/version.map                        |  3 ++
 7 files changed, 74 insertions(+)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..8ea1443044 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
 static int
 test_trace(void)
 {
+	if (!rte_trace_feature_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
 	return unit_test_suite_runner(&trace_tests);
 }
 
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 918f49bf4f..06130c756d 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -100,6 +100,16 @@ rte_trace_is_enabled(void)
 	return rte_atomic_load_explicit(&trace.status, rte_memory_order_acquire) != 0;
 }
 
+bool
+__rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 static void
 trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
diff --git a/lib/eal/include/rte_trace.h b/lib/eal/include/rte_trace.h
index a6e991fad3..340df4f8a0 100644
--- a/lib/eal/include/rte_trace.h
+++ b/lib/eal/include/rte_trace.h
@@ -35,6 +35,39 @@ extern "C" {
 __rte_experimental
 bool rte_trace_is_enabled(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * @internal
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+__rte_experimental
+bool __rte_trace_feature_is_enabled(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+static __rte_always_inline
+bool rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return __rte_trace_feature_is_enabled();
+#else
+	return false;
+#endif
+}
+
 /**
  * Enumerate trace mode operation.
  */
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..b80688ce89 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,25 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+/**
+ * @internal
+ *
+ * Test if the tracepoint compile-time option is enabled.
+ *
+ * @return
+ *   true if tracepoint enabled, false otherwise.
+ */
+__rte_experimental
+static __rte_always_inline bool
+__rte_trace_point_generic_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * @internal
  *
@@ -359,6 +378,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..429b993fc2 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -23,6 +23,8 @@ rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
diff --git a/lib/eal/version.map b/lib/eal/version.map
index e3ff412683..15c694c2da 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -396,6 +396,9 @@ EXPERIMENTAL {
 
 	# added in 24.03
 	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
+
+	# added in 24.11
+	__rte_trace_feature_is_enabled;
 };
 
 INTERNAL {
-- 
2.43.0

Re: [PATCH v6] eal: add build-time option to omit trace

[Jerin Jacob]

On Sun, Oct 6, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v6:
>   Removed test_trace_perf.c changes; they don't compile for Windows
>   target, and are superfluous.
> v5:
>   Added public function rte_trace_feature_is_enabled(), to test if trace
>   is build time enabled in both the DPDK and the application. Use in test
>   application instead of private function. (Jerin Jacob)
> v4:
> * Added check for generic trace enabled when registering trace points, in
>   RTE_INIT. (Jerin Jacob)
> * Test application uses function instead of macro to check if generic
>   trace is enabled. (Jerin Jacob)
> * Performance test application uses function to check if generic trace is
>   enabled.
> v3:
> * Simpler version with much fewer ifdefs. (Jerin Jacob)
> v2:
> * Added/modified macros required for building applications with trace
>   omitted.
> ---
>  app/test/test_trace.c                      |  4 +++
>  config/rte_config.h                        |  1 +
>  lib/eal/common/eal_common_trace.c          | 10 +++++++
>  lib/eal/include/rte_trace.h                | 33 ++++++++++++++++++++++
>  lib/eal/include/rte_trace_point.h          | 21 ++++++++++++++
>  lib/eal/include/rte_trace_point_register.h |  2 ++
>  lib/eal/version.map                        |  3 ++
>  7 files changed, 74 insertions(+)
>
> diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> index 00809f433b..8ea1443044 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
>  static int
>  test_trace(void)
>  {
> +       if (!rte_trace_feature_is_enabled()) {
> +               printf("Trace omitted at build-time, skipping test\n");
> +               return TEST_SKIPPED;
> +       }
>         return unit_test_suite_runner(&trace_tests);
>  }
>
> diff --git a/config/rte_config.h b/config/rte_config.h
> index dd7bb0d35b..fd6f8a2f1a 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -49,6 +49,7 @@
>  #define RTE_MAX_TAILQ 32
>  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
>  #define RTE_MAX_VFIO_CONTAINERS 64
> +#define RTE_TRACE 1
>
>  /* bsd module defines */
>  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index 918f49bf4f..06130c756d 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -100,6 +100,16 @@ rte_trace_is_enabled(void)
>         return rte_atomic_load_explicit(&trace.status, rte_memory_order_acquire) != 0;
>  }
>
> +bool
> +__rte_trace_feature_is_enabled(void)
> +{
> +#ifdef RTE_TRACE
> +       return true;
> +#else
> +       return false;
> +#endif
> +}
> +static __rte_always_inline
> +bool rte_trace_feature_is_enabled(void)
> +{
> +#ifdef RTE_TRACE
> +       return __rte_trace_feature_is_enabled();
> +#else
> +       return false;
> +#endif
> +}
> +__rte_experimental
> +static __rte_always_inline bool
> +__rte_trace_point_generic_is_enabled(void)
> +{
> +#ifdef RTE_TRACE
> +       return true;
> +#else
> +       return false;
> +#endif

__rte_trace_feature_is_enabled(), rte_trace_feature_is_enabled(),
__rte_trace_point_generic_is_enabled() are duplicates.
There is no harm in using a public API inside the implementation.
Please keep only rte_trace_feature_is_enabled()
and use it across implementation and app/test.

> +}
> +
>  /**
>   * @internal
>   *

>         rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
> +
> +       # added in 24.11
> +       __rte_trace_feature_is_enabled;

rte_trace_feature_is_enabled;

With the above changes,

Acked-by: Jerin Jacob <jerinj@marvell.com>



>  };
>
>  INTERNAL {
> --
> 2.43.0
>

RE: [PATCH v6] eal: add build-time option to omit trace

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Monday, 7 October 2024 07.45
> 
> On Sun, Oct 6, 2024 at 7:44 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > Some applications want to omit the trace feature.
> > Either to reduce the memory footprint, to reduce the exposed attack
> > surface, or for other reasons.
> >
> > This patch adds an option in rte_config.h to include or omit trace in
> the
> > build. Trace is included by default.
> >
> > Omitting trace works by omitting all trace points.
> > For API and ABI compatibility, the trace feature itself remains.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> > v6:
> >   Removed test_trace_perf.c changes; they don't compile for Windows
> >   target, and are superfluous.
> > v5:
> >   Added public function rte_trace_feature_is_enabled(), to test if
> trace
> >   is build time enabled in both the DPDK and the application. Use in
> test
> >   application instead of private function. (Jerin Jacob)
> > v4:
> > * Added check for generic trace enabled when registering trace
> points, in
> >   RTE_INIT. (Jerin Jacob)
> > * Test application uses function instead of macro to check if generic
> >   trace is enabled. (Jerin Jacob)
> > * Performance test application uses function to check if generic
> trace is
> >   enabled.
> > v3:
> > * Simpler version with much fewer ifdefs. (Jerin Jacob)
> > v2:
> > * Added/modified macros required for building applications with trace
> >   omitted.
> > ---
> >  app/test/test_trace.c                      |  4 +++
> >  config/rte_config.h                        |  1 +
> >  lib/eal/common/eal_common_trace.c          | 10 +++++++
> >  lib/eal/include/rte_trace.h                | 33
> ++++++++++++++++++++++
> >  lib/eal/include/rte_trace_point.h          | 21 ++++++++++++++
> >  lib/eal/include/rte_trace_point_register.h |  2 ++
> >  lib/eal/version.map                        |  3 ++
> >  7 files changed, 74 insertions(+)
> >
> > diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> > index 00809f433b..8ea1443044 100644
> > --- a/app/test/test_trace.c
> > +++ b/app/test/test_trace.c
> > @@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
> >  static int
> >  test_trace(void)
> >  {
> > +       if (!rte_trace_feature_is_enabled()) {
> > +               printf("Trace omitted at build-time, skipping
> test\n");
> > +               return TEST_SKIPPED;
> > +       }
> >         return unit_test_suite_runner(&trace_tests);
> >  }
> >
> > diff --git a/config/rte_config.h b/config/rte_config.h
> > index dd7bb0d35b..fd6f8a2f1a 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -49,6 +49,7 @@
> >  #define RTE_MAX_TAILQ 32
> >  #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
> >  #define RTE_MAX_VFIO_CONTAINERS 64
> > +#define RTE_TRACE 1
> >
> >  /* bsd module defines */
> >  #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
> > diff --git a/lib/eal/common/eal_common_trace.c
> b/lib/eal/common/eal_common_trace.c
> > index 918f49bf4f..06130c756d 100644
> > --- a/lib/eal/common/eal_common_trace.c
> > +++ b/lib/eal/common/eal_common_trace.c
> > @@ -100,6 +100,16 @@ rte_trace_is_enabled(void)
> >         return rte_atomic_load_explicit(&trace.status,
> rte_memory_order_acquire) != 0;
> >  }
> >
> > +bool
> > +__rte_trace_feature_is_enabled(void)
> > +{
> > +#ifdef RTE_TRACE
> > +       return true;
> > +#else
> > +       return false;
> > +#endif
> > +}
> > +static __rte_always_inline
> > +bool rte_trace_feature_is_enabled(void)
> > +{
> > +#ifdef RTE_TRACE
> > +       return __rte_trace_feature_is_enabled();
> > +#else
> > +       return false;
> > +#endif
> > +}
> > +__rte_experimental
> > +static __rte_always_inline bool
> > +__rte_trace_point_generic_is_enabled(void)
> > +{
> > +#ifdef RTE_TRACE
> > +       return true;
> > +#else
> > +       return false;
> > +#endif
> 
> __rte_trace_feature_is_enabled(), rte_trace_feature_is_enabled(),
> __rte_trace_point_generic_is_enabled() are duplicates.
> There is no harm in using a public API inside the implementation.
> Please keep only rte_trace_feature_is_enabled()
> and use it across implementation and app/test.

They are there to support DPDK being built as a library, and the application being built separately with a different rte_config.h.
But I suppose we generally don't support that. I will spin another version, which assumes that both DPDK (library) and application are built with the same rte_config.h.

I will rename __rte_trace_point_generic_is_enabled() to rte_trace_feature_is_enabled(), make it non-internal, and use that everywhere. It still needs to be inline, so the compiler can recognize it and omit the implementation when building with RTE_TRACE disabled.

> 
> > +}
> > +
> >  /**
> >   * @internal
> >   *
> 
> >         rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
> > +
> > +       # added in 24.11
> > +       __rte_trace_feature_is_enabled;
> 
> rte_trace_feature_is_enabled;

It will be inline, and thus removed from here.

> 
> With the above changes,
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>

Thank you for the quick response, Jerin.

[PATCH v7] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v7:
  Updated version.map to not export __rte_trace_feature_is_enabled for
  Windows target.
v6:
  Removed test_trace_perf.c changes; they don't compile for Windows
  target, and are superfluous.
v5:
  Added public function rte_trace_feature_is_enabled(), to test if trace
  is build time enabled in both the DPDK and the application. Use in test
  application instead of private function. (Jerin Jacob)
v4:
* Added check for generic trace enabled when registering trace points, in
  RTE_INIT. (Jerin Jacob)
* Test application uses function instead of macro to check if generic
  trace is enabled. (Jerin Jacob)
* Performance test application uses function to check if generic trace is
  enabled.
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      |  4 +++
 config/rte_config.h                        |  1 +
 lib/eal/common/eal_common_trace.c          | 10 +++++++
 lib/eal/include/rte_trace.h                | 33 ++++++++++++++++++++++
 lib/eal/include/rte_trace_point.h          | 21 ++++++++++++++
 lib/eal/include/rte_trace_point_register.h |  2 ++
 lib/eal/version.map                        |  3 ++
 7 files changed, 74 insertions(+)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..8ea1443044 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
 static int
 test_trace(void)
 {
+	if (!rte_trace_feature_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
 	return unit_test_suite_runner(&trace_tests);
 }
 
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 918f49bf4f..06130c756d 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -100,6 +100,16 @@ rte_trace_is_enabled(void)
 	return rte_atomic_load_explicit(&trace.status, rte_memory_order_acquire) != 0;
 }
 
+bool
+__rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 static void
 trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
diff --git a/lib/eal/include/rte_trace.h b/lib/eal/include/rte_trace.h
index a6e991fad3..340df4f8a0 100644
--- a/lib/eal/include/rte_trace.h
+++ b/lib/eal/include/rte_trace.h
@@ -35,6 +35,39 @@ extern "C" {
 __rte_experimental
 bool rte_trace_is_enabled(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * @internal
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+__rte_experimental
+bool __rte_trace_feature_is_enabled(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+static __rte_always_inline
+bool rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return __rte_trace_feature_is_enabled();
+#else
+	return false;
+#endif
+}
+
 /**
  * Enumerate trace mode operation.
  */
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..b80688ce89 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -212,6 +212,25 @@ bool rte_trace_point_is_enabled(rte_trace_point_t *tp);
 __rte_experimental
 rte_trace_point_t *rte_trace_point_lookup(const char *name);
 
+/**
+ * @internal
+ *
+ * Test if the tracepoint compile-time option is enabled.
+ *
+ * @return
+ *   true if tracepoint enabled, false otherwise.
+ */
+__rte_experimental
+static __rte_always_inline bool
+__rte_trace_point_generic_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * @internal
  *
@@ -359,6 +378,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..429b993fc2 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -23,6 +23,8 @@ rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
+	if (!__rte_trace_point_generic_is_enabled()) \
+		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
diff --git a/lib/eal/version.map b/lib/eal/version.map
index e3ff412683..6fd3dba85e 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -396,6 +396,9 @@ EXPERIMENTAL {
 
 	# added in 24.03
 	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
+
+	# added in 24.11
+	__rte_trace_feature_is_enabled; # WINDOWS_NO_EXPORT
 };
 
 INTERNAL {
-- 
2.43.0

RE: [PATCH v7] eal: add build-time option to omit trace

[Morten Brørup]

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Sunday, 6 October 2024 16.04
> 
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
> 
> This patch adds an option in rte_config.h to include or omit trace in
> the
> build. Trace is included by default.
> 
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

Forgot to carry over Stephen's Ack to v4, so here it comes for the benefit of Patchwork...

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH v9] eal: add build-time option to omit trace

[Morten Brørup]

Some applications want to omit the trace feature.
Either to reduce the memory footprint, to reduce the exposed attack
surface, or for other reasons.

This patch adds an option in rte_config.h to include or omit trace in the
build. Trace is included by default.

Omitting trace works by omitting all trace points.
For API and ABI compatibility, the trace feature itself remains.

Furthermore, a public function to determine if trace is build time enabled
is added; mainly for the benefit of the dpdk-test application.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Jerin Jacob <jerinj@marvell.com>
---
v9:
* Assume library and application are built with same rte_config.h.
* Renamed internal function __rte_trace_point_generic_is_enabled() to
  rte_trace_feature_is_enabled(), which is public. (Jerin Jacob)
* Removed changes that became superfluous with the above change.
v8:
* Added Stephen's Ack to v4, forgot to carry over.
v7:
* Updated version.map to not export __rte_trace_feature_is_enabled for
  Windows target.
v6:
* Removed test_trace_perf.c changes; they don't compile for Windows
  target, and are superfluous.
v5:
* Added public function rte_trace_feature_is_enabled(), to test if trace
  is build time enabled in both the DPDK and the application. Use in test
  application instead of private function. (Jerin Jacob)
v4:
* Added check for generic trace enabled when registering trace points, in
  RTE_INIT. (Jerin Jacob)
* Test application uses function instead of macro to check if generic
  trace is enabled. (Jerin Jacob)
* Performance test application uses function to check if generic trace is
  enabled.
v3:
* Simpler version with much fewer ifdefs. (Jerin Jacob)
v2:
* Added/modified macros required for building applications with trace
  omitted.
---
 app/test/test_trace.c                      |  4 ++++
 config/rte_config.h                        |  1 +
 lib/eal/include/rte_trace.h                | 19 +++++++++++++++++++
 lib/eal/include/rte_trace_point.h          |  3 +++
 lib/eal/include/rte_trace_point_register.h |  2 ++
 5 files changed, 29 insertions(+)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 00809f433b..8ea1443044 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -245,6 +245,10 @@ static struct unit_test_suite trace_tests = {
 static int
 test_trace(void)
 {
+	if (!rte_trace_feature_is_enabled()) {
+		printf("Trace omitted at build-time, skipping test\n");
+		return TEST_SKIPPED;
+	}
 	return unit_test_suite_runner(&trace_tests);
 }
 
diff --git a/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..fd6f8a2f1a 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -49,6 +49,7 @@
 #define RTE_MAX_TAILQ 32
 #define RTE_LOG_DP_LEVEL RTE_LOG_INFO
 #define RTE_MAX_VFIO_CONTAINERS 64
+#define RTE_TRACE 1
 
 /* bsd module defines */
 #define RTE_CONTIGMEM_MAX_NUM_BUFS 64
diff --git a/lib/eal/include/rte_trace.h b/lib/eal/include/rte_trace.h
index a6e991fad3..6eac95188b 100644
--- a/lib/eal/include/rte_trace.h
+++ b/lib/eal/include/rte_trace.h
@@ -35,6 +35,25 @@ extern "C" {
 __rte_experimental
 bool rte_trace_is_enabled(void);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Test if trace feature is enabled at compile time.
+ *
+ * @return
+ *   true if trace feature is enabled, false otherwise.
+ */
+static __rte_always_inline
+bool rte_trace_feature_is_enabled(void)
+{
+#ifdef RTE_TRACE
+	return true;
+#else
+	return false;
+#endif
+}
+
 /**
  * Enumerate trace mode operation.
  */
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 41e2a7f99e..dfe89302ed 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -30,6 +30,7 @@ extern "C" {
 #include <rte_per_lcore.h>
 #include <rte_stdatomic.h>
 #include <rte_string_fns.h>
+#include <rte_trace.h>
 #include <rte_uuid.h>
 
 /** The tracepoint object. */
@@ -359,6 +360,8 @@ __rte_trace_point_emit_ev_header(void *mem, uint64_t in)
 #define __rte_trace_point_emit_header_generic(t) \
 void *mem; \
 do { \
+	if (!rte_trace_feature_is_enabled()) \
+		return; \
 	const uint64_t val = rte_atomic_load_explicit(t, rte_memory_order_acquire); \
 	if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \
 		return; \
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 41260e5964..283dcef75d 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -23,6 +23,8 @@ rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
+	if (!rte_trace_feature_is_enabled()) \
+		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
-- 
2.43.0

RE: [PATCH v9] eal: add build-time option to omit trace

[Morten Brørup]

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 7 October 2024 13.46
> 
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
> 
> This patch adds an option in rte_config.h to include or omit trace in
> the
> build. Trace is included by default.
> 
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
> 
> Furthermore, a public function to determine if trace is build time
> enabled
> is added; mainly for the benefit of the dpdk-test application.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> ---

Both trace_autotest and trace_perf_autotest have been tested (in a Linux environment) to behave as expected, both with and without RTE_TRACE defined.

Tested-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v9] eal: add build-time option to omit trace

[David Marchand]

On Mon, Oct 7, 2024 at 1:46 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> Some applications want to omit the trace feature.
> Either to reduce the memory footprint, to reduce the exposed attack
> surface, or for other reasons.
>
> This patch adds an option in rte_config.h to include or omit trace in the
> build. Trace is included by default.
>
> Omitting trace works by omitting all trace points.
> For API and ABI compatibility, the trace feature itself remains.
>
> Furthermore, a public function to determine if trace is build time enabled
> is added; mainly for the benefit of the dpdk-test application.
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Jerin Jacob <jerinj@marvell.com>

Series applied.
Thanks Morten.


-- 
David Marchand