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 3C20845A9E; Wed, 2 Oct 2024 18:18:27 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A94240268; Wed, 2 Oct 2024 18:18:27 +0200 (CEST) Received: from mail-oo1-f52.google.com (mail-oo1-f52.google.com [209.85.161.52]) by mails.dpdk.org (Postfix) with ESMTP id 1CBB84025C for ; Wed, 2 Oct 2024 18:18:25 +0200 (CEST) Received: by mail-oo1-f52.google.com with SMTP id 006d021491bc7-5e7aec9e15fso848369eaf.3 for ; Wed, 02 Oct 2024 09:18:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1727885904; x=1728490704; 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=5u7ZplnpuBUgXyZzw/r6Hqnr9aYYC2V2iUqDL3Ub8X8=; b=iCNlZza3QywbtyOaQl+KnerHlB/Ag/R5cccvPe9mkskDevQv9BOgh1MiW14r9VBAiq /4PhvUdFnMpUSRei1r3Sf9zU0syWS4cnvVT/nPWWuiXZYMKFeBuBGFkDjXEqWXizSykj jH7wQe7xVrnuw02FKb8ZE04Kl6bC4dUwovujRWLunk4ir4sToMjC8OYz1HhDPKmFXfqS ZI6tJxG7xGiPmQRpWxlXdUYmBoKQKhMSXMvXt3ufM+XYCtLmKB3X5KwV2b1wKJ4DvRjG qpyj2hRjaKYqIR2GbGnHPa+feHKuYyBvFF1BKABargW0525af1Eh3JI4sWugYs8X8TS6 Ch/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727885904; x=1728490704; 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=5u7ZplnpuBUgXyZzw/r6Hqnr9aYYC2V2iUqDL3Ub8X8=; b=JDUPMBwhKXT4/WzUAb48z8NbSNMgBIGv7u+yZdmQ0L7tW+fSYXqLekCgniHOMm0B1z 6Qg98/UrVz/I7dCgDg75eWdcVclS2EFE5lYbU+jaTq4eDUknCOwqB7Ag9VeYfPzyFzVv 5kIT0cKdE1v5WJEJuvjo+estgPOEoRCux84XlYP3lVOToC2LouNg/D6/+2+Y7ExoMfOD YPa8HZxz4xwV/0qm14h+vRPOqAEeNKwNu1nolEm/Z9DWz84Jdg5kynLKUh2qOjSemxBB 9xrEDpumVgLq2mhgRbAxEIK+z+7yV725e1Z8sBO2u2Wb91rKqZxS2V38uAyBvySbuRKv dh9Q== X-Forwarded-Encrypted: i=1; AJvYcCVSSRDeGHdL6/ff4oMYjqUSDSnkieWnpGKbK6NQB3Vrt1kfyZ0n20PkX32A1P8Ex3ievkg=@dpdk.org X-Gm-Message-State: AOJu0YyfUS1saw7qYirls6dfQiG0+7aH5YdaO5A3KDEEO3xKPHhTXfZf 1iFTVX/HfDY+pIF7XiNuhEsfFY4yWLxhEK4WpHmFpBHPP3fF4PH5I4MLPkD74qJjZA7FWim2053 tVdtYehtTw3I+nEQeos7o6I2tiYc= X-Google-Smtp-Source: AGHT+IEHiVJg4cjMEKEF6/0AvrNZFoCUkXM6cflUe+pYxZghsd1RG33TkBCWKCwYERk1syynpHw1kWv+0k9N3o5Euqk= X-Received: by 2002:a05:6358:d596:b0:1b8:341d:36a1 with SMTP id e5c5f4694b2df-1c0cecbe6bbmr267804155d.2.1727885903948; Wed, 02 Oct 2024 09:18:23 -0700 (PDT) MIME-Version: 1.0 References: <20240918085551.231015-1-mb@smartsharesystems.com> <20240924133957.1505113-1-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35E9F72A@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35E9F74B@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35E9F74C@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35E9F74D@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35E9F74E@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F74E@smartserver.smartshare.dk> From: Jerin Jacob Date: Wed, 2 Oct 2024 21:47:57 +0530 Message-ID: Subject: Re: [PATCH v4] 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 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 Tue, Oct 1, 2024 at 9:45=E2=80=AFPM Morten Br=C3=B8rup wrote: > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > > Sent: Tuesday, 1 October 2024 18.06 > > > > On Tue, Oct 1, 2024 at 9:32=E2=80=AFPM Morten Br=C3=B8rup > > wrote: > > > > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > > > > Sent: Tuesday, 1 October 2024 17.02 > > > > > > > > On Tue, Oct 1, 2024 at 7:44=E2=80=AFPM Morten Br=C3=B8rup > > > > > > wrote: > > > > > > > > > > > From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > > > > > > Sent: Tuesday, 1 October 2024 16.05 > > > > > > > > > > > > On Tue, Oct 1, 2024 at 7:19=E2=80=AFPM Morten Br=C3=B8rup > > > > > > > > > > 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") !=3D 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() =3D=3D false) > > + return false; > > return rte_atomic_load_explicit(&trace.status, > > rte_memory_order_acquire) !=3D 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 s= hould run test cases or not. At this point, trace.status is zero, so it ski= ps testing. > > We don't need to add the test for rte_trace_point_generic_is_enabled()=3D= =3Dfalse in rte_trace_is_enabled(), because nothing can increase trace.stat= us 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. >