From: Philip Li <philip.li@intel•com>
To: Jakub Kicinski <kuba@kernel•org>
Cc: "Nambiar, Amritha" <amritha.nambiar@intel•com>,
<oe-kbuild-all@lists•linux.dev>,
kernel test robot <lkp@intel•com>, <netdev@vger•kernel.org>,
<pabeni@redhat•com>
Subject: Re: [net-next PATCH v5 01/10] netdev-genl: spec: Extend netdev netlink spec in YAML for queue
Date: Tue, 24 Oct 2023 10:00:53 +0800 [thread overview]
Message-ID: <ZTclVfX5mtWXziSW@rli9-mobl> (raw)
In-Reply-To: <20231023184411.73919423@kernel.org>
On Mon, Oct 23, 2023 at 06:44:11PM -0700, Jakub Kicinski wrote:
> On Tue, 24 Oct 2023 09:02:46 +0800 Philip Li wrote:
> > > I understand and appreciate the effort.
> > >
> > > I think that false positive has about a 100x the negative effect of a
> > > true positive. If more than 1% of checkpatch warnings are ignored, we
> > > should *not* report them to the list. Currently in networking we fully
> > > trust the build bot and as soon as a patch set gets a reply from you it
> > > gets auto-dropped from our review queue.
> >
> > Thanks for the trust. Sorry I didn't notice the false checkpatch report leads
> > to trouble. From below info, may i understand networking already runs own
> > checkpatch? Also consider the checkpatch reports from bot still contains quite
> > some false ones, probably we can pause the checkpatch reporting for network
> > side if it doesn't add much value and causes trouble?
>
> Yes, correct, we already run checkpatch --strict on all patches.
>
> If you have the ability to selectively disable checkpatch for net/ and
> drivers/net, and/or patches which CC netdev@vger, that'd be great!
Got it, thanks for the detail info, we will pause the reports for these
places. Before that, i will also pause the overall checkpatch check until
we have resolved this for networking side.
>
>
> FWIW we have a simple dashboard reporting which checks in our own
> local build fail the most: https://netdev.bots.linux.dev/checks.html
> Not sure if it's of any interest to you, but that's where I got the
> false positive rate I mentioned previously.
This is very advanced and clear! Thanks for sharing, let me dig into
it to learn from the dashboard.
>
> > > And the maintainer is not very receptive to improvements for false
> > > positives:
> > > https://lore.kernel.org/all/20231013172739.1113964-1-kuba@kernel.org/
> >
> > I see. We got this pattern as well, what we do now is to maintain the pattern
> > internally to avoid unnecessary reports (some are extracted below). I'm looking
> > for publishing these patterns later, which may get more inputs to filter out
> > unnecessary reports.
> >
> > == part of low confidence patterns of checkpatch in bot ==
>
> Interesting!
>
> > __func__ should be used instead of gcc specific __FUNCTION__
>
> This one I don't see failing often.
>
> > line over 80 characters
>
> This one happens a lot, yes.
>
> > LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
>
> This is very rare upstream.
>
> > Missing commit description - Add an appropriate one
>
> Should be rare upstream..
>
> > please write a help paragraph that fully describes the config symbol
>
> This check I think is semi-broken in checkpatch.
> Sometimes it just doesn't recognize the help even if symbol has it.
> So yes, we see if false-positive as well.
>
> > Possible repeated word: 'Google'
>
> Yes! :)
>
> > Possible unwrapped commit description \(prefer a maximum 75 chars per line\)
>
> This one indeed has a lot of false positives. It should check if
> *majority* of the commit message lines (excluding tags) are too long,
> not any single line. Because one line can be a crash dump or a commit
> reference, and be longer for legit reasons..
Thanks for all above comments/analysis/experience, which brings a lot insights.
>
> Every now and then I feel like we should fork checkpatch or start a new
> tool which would report only high-confidence problems.
:)
>
> > > > But as you mentioned above, we will take furture care to the output
> > > > of checkpatch to be conservative for the reporting.
> > >
> > > FWIW the most issues that "get through" in networking are issues
> > > in documentation (warnings for make htmldocs) :(
> >
> > Do you suggest that warnings for make htmldocs or kernel-doc warning when building
> > with W=1 can be ignored and no need to send them to networking side?
>
> No, no, the opposite! Documentation is one part we currently don't test,
> even tho we should.
>
> Do you run make htmldocs as part of kernel build bot? As you allude to -
yes, the bot runs make htmldocs check as part of various checks such as
includecheck, dtcheck, etc.
We will continue doing this for networking side.
> W=1 checks kdoc already, and scripts/kernel-doc can be used to validate
> headers even more easily. But to validate the ReST files under
> Documentation/ one has to actually run make htmldocs (or perhaps some
> other docs target), not just a normal build.
>
next prev parent reply other threads:[~2023-10-24 2:01 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-19 0:06 [net-next PATCH v5 00/10] Introduce queue and NAPI support in netdev-genl (Was: Introduce NAPI queues support) Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 01/10] netdev-genl: spec: Extend netdev netlink spec in YAML for queue Amritha Nambiar
2023-10-19 2:12 ` kernel test robot
2023-10-20 20:26 ` Nambiar, Amritha
2023-10-20 22:05 ` Jakub Kicinski
2023-10-21 0:37 ` Nambiar, Amritha
2023-10-21 1:53 ` Philip Li
2023-10-23 14:52 ` Jakub Kicinski
2023-10-23 19:09 ` Nambiar, Amritha
2023-10-23 22:39 ` Jakub Kicinski
2023-10-24 1:02 ` Philip Li
2023-10-24 1:44 ` Jakub Kicinski
2023-10-24 2:00 ` Philip Li [this message]
2023-10-19 0:06 ` [net-next PATCH v5 02/10] net: Add queue and napi association Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 03/10] ice: Add support in the driver for associating queue with napi Amritha Nambiar
2023-10-19 5:18 ` kernel test robot
2023-10-19 0:06 ` [net-next PATCH v5 04/10] netdev-genl: Add netlink framework functions for queue Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 05/10] netdev-genl: spec: Extend netdev netlink spec in YAML for NAPI Amritha Nambiar
2023-10-19 2:12 ` kernel test robot
2023-10-19 0:06 ` [net-next PATCH v5 06/10] netdev-genl: Add netlink framework functions for napi Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 07/10] netdev-genl: spec: Add irq in netdev netlink YAML spec Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 08/10] net: Add NAPI IRQ support Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 09/10] netdev-genl: spec: Add PID in netdev netlink YAML spec Amritha Nambiar
2023-10-19 0:06 ` [net-next PATCH v5 10/10] netdev-genl: Add PID for the NAPI thread Amritha Nambiar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZTclVfX5mtWXziSW@rli9-mobl \
--to=philip.li@intel$(echo .)com \
--cc=amritha.nambiar@intel$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=lkp@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=oe-kbuild-all@lists$(echo .)linux.dev \
--cc=pabeni@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox