From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 04888459D6; Thu, 19 Sep 2024 15:54:44 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9F4E24336A; Thu, 19 Sep 2024 15:54:44 +0200 (CEST) Received: from mail-qt1-f170.google.com (mail-qt1-f170.google.com [209.85.160.170]) by mails.dpdk.org (Postfix) with ESMTP id 17F6343366; Thu, 19 Sep 2024 15:54:43 +0200 (CEST) Received: by mail-qt1-f170.google.com with SMTP id d75a77b69052e-4585721f6edso7335381cf.2; Thu, 19 Sep 2024 06:54:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1726754082; x=1727358882; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=6YbUCvvX/UTDaGrbk/MvAKSV0uJDdoo+0eM+uYmzT2U=; b=UYYR9Pw3GiR77UUGzlHCzA9/zPGG4kOhhe/rG1KSOCH3oLATzZPkRDQDQbueUo0eC7 dJZk1feTUAyzSx/qCic0+xkRkHnLp9C/Ly8kUHAuXpqHiji2Joj75XbSwO5bSfVx2+wv 4/9M347oGf/p6G0kuQxLKM7fe9zPlrurDAbdLpgOer1nUZuSatLCXCMdyemlGy1KHfku JkgR6xK9z8y7hcCWwyTpGKF72b0PDR2l6+U+u2vGV8AOa6c4CK6IS4vTxnBncuwbaVxh NJ2tWlYFzj0XluQWPvKI2BBwzLTpv2gOrTEPu1erwAL5CbPvFeJlC2Lgz1D6gdpWIPqv 171A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726754082; x=1727358882; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=6YbUCvvX/UTDaGrbk/MvAKSV0uJDdoo+0eM+uYmzT2U=; b=q4AeM7Gn78kdNIhl3HXR1wWcE/ATtO0uiKaNQawVBA6fSeVrZYvAdXvO3AyUaze8Ae bPJtp3WGAuxL9gCZDoa5fcwoc5PQ1SCjsaVQnfvUkD5J4/ZbIczi0TqSWlRb5YnveYfV NiUKlQM4VKo/KQehet4Rf0hIW7X7FMbReFGuMygi2KtpFyR7iEmEzzynyayovcGccXjI 7zATmG+0cRt1bOZliwYTbvm5UH0tijs4hNkVeqMzD1zPNSJ3WKat4io6f1zVkF2QGFQT Q3mwyUnUElRDCh5q7HEUoTMhyhSoFqjB2kqxmubEULGCWUG/aYT89LGDEx53+DRvQozN YHRQ== X-Forwarded-Encrypted: i=1; AJvYcCVuKtfVPU5+u7er4m9TOUenWrS0yQXmUTaBljxE9q+sMr2ZN7WNBC9WUO5lgLiDE4t8NWi5xFHTK3yp@dpdk.org, AJvYcCWXgdm8QjHvQ+48GTBDTInK7qC21rRxyPtuYyz6ngVi21clCezJiOh5LsyBLcdWwQijzv8=@dpdk.org X-Gm-Message-State: AOJu0YwqhLPcoDBNz5haLQCawTUznMvQSM1N9+g1wQeHVvNhF5P4hVv4 kjzXHI3pG76xZDOCjWFb8xCd+3tiQ4mgQemFHZzV/YS++I4vvNwPhwL5LdKF3mLLLg//1EfcZTB vPCcp7RedqMsxBfCdTUh4EYaOmFk= X-Google-Smtp-Source: AGHT+IEKvDl4VdPPgYAZYj6WqXCujoSFkWO90+JUQOeDBU/yunMFnnF6/TA2WP97t75VwtqCxwyI6Zsj19S0IMbqSeA= X-Received: by 2002:ac8:7f0f:0:b0:451:d859:202d with SMTP id d75a77b69052e-4599d2ba36cmr340962811cf.54.1726754082077; Thu, 19 Sep 2024 06:54:42 -0700 (PDT) MIME-Version: 1.0 References: <20240918085551.231015-1-mb@smartsharesystems.com> <20240919080658.252532-1-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35E9F702@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F702@smartserver.smartshare.dk> From: Jerin Jacob Date: Thu, 19 Sep 2024 19:24:15 +0530 Message-ID: Subject: Re: [PATCH v2] eal: add build-time option to omit trace To: =?UTF-8?Q?Morten_Br=C3=B8rup?= Cc: Jerin Jacob , Sunil Kumar Kori , dev@dpdk.org, techboard@dpdk.org, Ferruh Yigit Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Sep 19, 2024 at 6:45=E2=80=AFPM Morten Br=C3=B8rup wrote: > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > > Sent: Thursday, 19 September 2024 10.52 > > > > On Thu, Sep 19, 2024 at 1:37=E2=80=AFPM Morten Br=C3=B8rup > > 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=C3=B8rup > > > --- > > > 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, t= o meet the requirements driving this patch: reducing attack surface and mem= ory 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=E2=80=99s 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_ASSE= RT(), mempool statistics, etc. > 3. Header files in various modules/drivers, for tweaking the individual m= odule. > > There didn't seem to be any interest in enabling/disabling trace via Meso= n, 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 applian= ces. For some of these high-volume network appliances, memory footprint mat= ters. 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 p= atch. 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 consum= ption 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 a= n abstraction layer to the underlying hardware and O/S. > This would make trace an independent module, which could be enabled/disab= led through Meson. > > Bruce has contributed a lot in this area, and already made many modules o= ptional. > > > > > > > #define __rte_trace_point_emit_header_generic(t) \ > > void *mem; \ > > do { \ > > + if (RTE_TRACE =3D=3D 0) \ > > + return \ > > const uint64_t val =3D rte_atomic_load_explicit(t, > > rte_memory_order_acquire); \ > > if (likely(!(val & __RTE_TRACE_FIELD_ENABLE_MASK))) \ > > return; \ > > mem =3D __rte_trace_mem_get(val); \ > > if (unlikely(mem =3D=3D NULL)) \ > > return; \ > > mem =3D __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, ar= gs) > > > +#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, uint6= 4_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[] =3D 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 > > >