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

[Ankur Dwivedi <adwivedi@marvell.com>]

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Friday, September 1, 2023 12:59 PM
>To: Morten Brørup <mb@smartsharesystems.com>; Jerin Jacob
><jerinjacobk@gmail.com>
>Cc: Ankur Dwivedi <adwivedi@marvell.com>; Stephen Hemminger
><stephen@networkplumber.org>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; dev@dpdk.org; techboard@dpdk.org
>Subject: Re: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in
>checkpatch
>
>Let's decide in a techboard meeting whether traces are mandatory or not.

It was agreed in techboard meeting in September, that tracepoints needs to be present for new public API functions in cryptodev, ethdev, eventdev, mempool. Can this patch which automates the check for presence of tracepoint be reviewed and merged if its fine?
>
>
>01/09/2023 04:32, Jerin Jacob:
>> On Thu, Aug 31, 2023 at 12:08 AM Morten Brørup
><mb@smartsharesystems.com> wrote:
>> >
>> > > 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
>>
>> > >
>> > > 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.
>
>
>