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

[Thomas Monjalon]

Let's decide in a techboard meeting whether traces are mandatory or not.


01/09/2023 04:32, Jerin Jacob:
> On Thu, Aug 31, 2023 at 12:08 AM Morten Brørup <mb at smartsharesystems.com> wrote:
> >
> > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > Sent: Wednesday, 30 August 2023 18.24
> > >
> > > 21/08/2023 16:46, Morten Brørup:
> > > > > From: Ankur Dwivedi [mailto:adwivedi at marvell.com]
> > > > > Sent: Monday, 21 August 2023 15.54
> 
> > >
> > > 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.
> 
> IMO, There is a lot of effort to put trace points to existing
> libraries, without these check, soon the disparity shows up when
> someone adds new APIs to library and forget
> to add trace points.
> 
> It is pretty easy to add trace point and there are a lot of
> references, so I don't think there is burden  for a developer
> comparing to the effort of adding a new API.
> 
> Most of the time, contributors forgets to add trace point because
> there is no automatic way to find it.
> Also, additional git commits are needs if some decide to add it later.
> 
> No strong opinion, If not find not useful, we could mark this patch is
> not appliable. Keeping the patch is limbo state is the issue. It is
> already reached to v5.
> So please decide one way or another.
> 
> 
> 
> 
> > </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.