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 1CC9641FD2; Wed, 30 Aug 2023 20:38:25 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EC6734027D; Wed, 30 Aug 2023 20:38:24 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 4E4C24027C for ; Wed, 30 Aug 2023 20:38:23 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 164A5204BA; Wed, 30 Aug 2023 20:38:22 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch Date: Wed, 30 Aug 2023 20:38:20 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87B5D@smartserver.smartshare.dk> In-Reply-To: <3574329.R56niFO833@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch Thread-Index: AdnbXmIYg6jWdQxaQvqYMDlvUI7KJQAEPTSA References: <20230303155811.2751210-1-adwivedi@marvell.com> <98CBD80474FA8B44BF855DF32C47DC35D87B25@smartserver.smartshare.dk> <3574329.R56niFO833@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , "Ankur Dwivedi" Cc: "Stephen Hemminger" , "Jerin Jacob Kollanukkaran" , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Wednesday, 30 August 2023 18.24 >=20 > 21/08/2023 16:46, Morten Br=F8rup: > > > From: Ankur Dwivedi [mailto:adwivedi@marvell.com] > > > Sent: Monday, 21 August 2023 15.54 > > > > > > >From: Stephen Hemminger > > > >Sent: Thursday, May 18, 2023 9:04 PM > > > > > > > = >------------------------------------------------------------------- > --- > > > >On Thu, 18 May 2023 13:45:29 +0000 > > > >Ankur Dwivedi wrote: > > > > > > > >> >From: Ankur Dwivedi > > > >> >Sent: Tuesday, March 7, 2023 5:35 PM > > > >> > > > > >> >This patch adds a validation in checkpatch tool, to check if a > > > >> >tracepoint is present in any new function added in cryptodev, > > > ethdev, > > > >> >eventdev and mempool library. > > > >> > > > > >> >In this patch, the build_map_changes function is copied from > > > >> >check-symbol- change.sh to check-tracepoint.sh. The > > > >> >check-tracepoint.sh script uses build_map_changes function to > create > > > a > > > >map of functions. > > > >> >In the map, the newly added functions, added in the = experimental > > > >> >section are identified and their definition are checked for = the > > > >> >presence of tracepoint. The checkpatch return error if the > > > tracepoint is not > > > >present. > > > >> > > > > >> >For functions for which trace is not needed, they can be added > to > > > >> >devtools/trace-skiplist.txt file. The above tracepoint check > will be > > > >> >skipped for them. > > > >> > > > > >> >Signed-off-by: Ankur Dwivedi > > > > > > > >Given the amount of string processing, it would be more readable = in > > > python. > > > >That is not a show stopper, just a suggestion. > > > > > > Hi Thomas, > > > > > > Please let me know if the shell script in this patch is fine or > would a > > > python implementation would be more preferable. >=20 > In general, I wonder how much the check is useful compared to the > complexity. >=20 >=20 > > The bigger question is: Do we really want to change tracepoints in > functions from opt-in to opt-out? >=20 > Yes that's the question: should traces be mandatory in some libs? >=20 > > In my opinion, opt-in for trace is more appropriate. >=20 > There was some work to add traces everywhere in few libs, so why not > maintaining this state? I still think it's a silly and burdening requirement, but I'm not = against it for the libs where it is already a de facto standard. I can imagine similar requirements regarding logging, telemetry and dump = functions being imposed on select libs. Perhaps we should also require that new libs implement all four types of = instrumentation, to ensure that only high quality (i.e. fully = instrumented) libs are accepted into DPDK. > I don't really like adding a skip list as one more burden for future > authors. If the warning omitted from checkpatches refers to the skip list and its = location, it is relatively easy for developers to manage. And reviewers will notice if new functions have been added to the skip = list, indicating that trace has been omitted. So there are also = advantages to the skip list. >=20 >=20 > > Nonetheless, having a tool to check for tracepoint presence might > still be useful for library reviewers and maintainers. And such a tool > might be useful for any library, not just the few libraries suggested = by > this patch. >=20 >=20 >=20