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 663A0459D3; Thu, 19 Sep 2024 10:52:01 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 37193402D4; Thu, 19 Sep 2024 10:52:01 +0200 (CEST) Received: from mail-qt1-f169.google.com (mail-qt1-f169.google.com [209.85.160.169]) by mails.dpdk.org (Postfix) with ESMTP id 1B2D8400D5; Thu, 19 Sep 2024 10:52:00 +0200 (CEST) Received: by mail-qt1-f169.google.com with SMTP id d75a77b69052e-457cfc2106aso5463651cf.2; Thu, 19 Sep 2024 01:52:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1726735919; x=1727340719; 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=uyYVR0xpv1AYHC95HEBNlDWV997OgoKBZbk2CdkENXY=; b=lDIdiNr407MEq9YXhjAYflet57SoP6guETBZAJ+vuj3IelAdWC/QCGdSt+DLrRSvnP FH0+xS2AGoXyDJwyveshuJkoa6ftXeUEFnRyCgpFlW1OvlXJCVDqZubmK3bnrACmkqDO JfIPkI797qml33ev5bfCjMzq3wFwF+yxwqg3MI087Q1z4PF/8jghgdkoPnKsKEOvj/hj IwjWSvnz2BeEk6TUEA8jOTcPIMk+5zZ755O1MNCpFGcHXcwM2xODCRFkeR/20TKnq0S6 t2ABAaInRgcTaXRJVzkYix9dqAeGzaGa6WxN6KFJukO8+e7wOa7J7woivl7u6m3keoPe T9bA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726735919; x=1727340719; 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=uyYVR0xpv1AYHC95HEBNlDWV997OgoKBZbk2CdkENXY=; b=CNFpxtxKRYlN8sxOgLzBjSMGHjgFzgucTG633afrZ9D/fT+DbYzznht3NVSFtuKrrX /uTWvhrTwgKv+t2w5gdL6urrbeQ5kNMGpbqxcDdRoX3Y+GB8kDkTT+gch2BgAlhwBfmU iXZdx3nrwq/U8WLUvuFBCnDdQ2sEypxGCYIa+ei94iiLuZon6vLwXGE6X4maAyNCcUDO kS+iVYK87mPKvxXrtx4YLDxUiAXCZX8wAxb3SjWI4pzm4zgnWtYQjT2FrBFy81illX+t K04r6oOwS08jQ6elyVa9N4ONKHD+gJnqIPrXqGhDICciKkLpsqCLMp5OWVI7AaA91PPP 7AlQ== X-Forwarded-Encrypted: i=1; AJvYcCVMEI2jzADsyXywDEZjRw6s73rKnzUJzYK/WmObTptI9Djmvw2ayR0MsIAA5aIxVPBLV4w=@dpdk.org, AJvYcCVk1vWSnm7tSOG3iGucNs1XMfFVGSiw3OUqrBIDSolrwaKR8D7PTNAvbW25nhtuxXOnl4Jb6jhH7Da9@dpdk.org X-Gm-Message-State: AOJu0YwuQNfhodrUeX5kyJZUiCmPR0BimfbqQ8mO6//vsDHy3Ljthaae T/euISCfiIqfkbEmII+Guigi87dvIU+zs1ZPKaar7MaLLsKZTA07tKdUN3pbqZdBN9sbpEsncB+ gvda5W6pX36Ma3HVUJSxPOdBRtCY= X-Google-Smtp-Source: AGHT+IESvrdLr3MFynp4dhqMYmK5mDhCvFK6xDn7q80dBV43+T5ubNWFvpV9DU0ENkldNDWUqG9QGUcznHvI9ntKpKY= X-Received: by 2002:a05:622a:1897:b0:458:26ef:6c with SMTP id d75a77b69052e-4599d2359c0mr407553261cf.13.1726735919227; Thu, 19 Sep 2024 01:51:59 -0700 (PDT) MIME-Version: 1.0 References: <20240918085551.231015-1-mb@smartsharesystems.com> <20240919080658.252532-1-mb@smartsharesystems.com> In-Reply-To: <20240919080658.252532-1-mb@smartsharesystems.com> From: Jerin Jacob Date: Thu, 19 Sep 2024 14:21:33 +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 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 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 =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_trac= e_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_POIN= T_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_POIN= T_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, arg= s) > + > +#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[] =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 >