Message-ID: <ZTcXtklgqYXfoSce@rli9-mobl>
Date: Tue, 24 Oct 2023 09:02:46 +0800
From: Philip Li <philip.li@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "Nambiar, Amritha" <amritha.nambiar@...el.com>,
	<oe-kbuild-all@...ts.linux.dev>, kernel test robot <lkp@...el.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [net-next PATCH v5 01/10] netdev-genl: spec: Extend netdev
 netlink spec in YAML for queue

On Mon, Oct 23, 2023 at 07:52:21AM -0700, Jakub Kicinski wrote:
> On Sat, 21 Oct 2023 09:53:03 +0800 Philip Li wrote:
> > > Some of them are bogus. TBH I'm not sure how much value running
> > > checkpatch in the bot adds. It's really trivial to run for the  
> > 
> > It is found there're quite some checkpatch related fix commits on
> > mainline.
> 
> Those changes are mostly for old code, aren't they?

Got it, we don't have the statistics data for this yet. I think this
is helpful for us to understand the status in depth. We will think of
this to gather some data.

> It'd be useful to do some analysis of how long ago the mis-formatted
> code has been introduced. Because if new code doesn't get fixes
> there's no point testing new patches..
> 
> > Thus the bot wants to extend the coverage and do shift
> > left testing on developer repos and mailing list patches.
> 
> 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?

> It'd be quite bad if we have to double check the reports.

Got it, We are aware of keeping the reports in high quality to make
it fully useful. Usually for static analysis tool like sparse/smatch/cocci,
we maintain both high confidence pattern and low confidence one, which is
continously improved per developer feedback and our own analysis. For low
confidence one, it need manual check to be sent out.

Since we fully enable checkpatch not long ago, it causes various negative
reports. Sorry about this.

> 
> Speaking of false positive rate - we disabled some checks in our own
> use of checkpatch:
> https://github.com/kuba-moo/nipa/blob/master/tests/patch/checkpatch/checkpatch.sh#L6-L12
> and we still get about 26% false positive rate! (Count by looking at
> checks that failed and were ignored, because patch was merged anyway).
> A lot of those may be line length related (we still prefer 80 char
> limit) but even without that - checkpatch false positives a lot.

Thanks for the great info, this is very helpful to us to learn from
this. We will adopt some practices here and continue improving the
reports.

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

__func__ should be used instead of gcc specific __FUNCTION__
line over 80 characters
LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
Missing commit description - Add an appropriate one
please write a help paragraph that fully describes the config symbol
Possible repeated word: 'Google'
Possible unwrapped commit description \(prefer a maximum 75 chars per line\)

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

> 

Powered by blists - more mailing lists