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

Message ID 20241006135858.2768371-1-mb@smartsharesystems.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v6] eal: add build-time option to omit trace |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS

Commit Message

Morten Brørup Oct. 6, 2024, 1:58 p.m. UTC
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(+)
  

Comments

Jerin Jacob Oct. 7, 2024, 5:45 a.m. UTC | #1
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
>
  
Morten Brørup Oct. 7, 2024, 6:07 a.m. UTC | #2
> 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

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 {