RE: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 30 August 2023 18.24
> 
> 21/08/2023 16:46, Morten Brørup:
> > > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> > > Sent: Monday, 21 August 2023 15.54
> > >
> > > >From: Stephen Hemminger <stephen@networkplumber.org>
> > > >Sent: Thursday, May 18, 2023 9:04 PM
> > > >
> > > >-------------------------------------------------------------------
> ---
> > > >On Thu, 18 May 2023 13:45:29 +0000
> > > >Ankur Dwivedi <adwivedi@marvell.com> wrote:
> > > >
> > > >> >From: Ankur Dwivedi <adwivedi@marvell.com>
> > > >> >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 <adwivedi@marvell.com>
> > > >
> > > >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.
> 
> In general, I wonder how much the check is useful compared to the
> complexity.
> 
> 
> > The bigger question is: Do we really want to change tracepoints in
> functions from opt-in to opt-out?
> 
> Yes that's the question: should traces be mandatory in some libs?
> 
> > In my opinion, opt-in for trace is more appropriate.
> 
> 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.

<irony>
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.
</irony>

> 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.

> 
> 
> > 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.
> 
> 
>