This patch makes the following needlessly global code in kernel/sched.c  
static:
- sched_feat_names[]
- sched_feat_open()

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

This patch has been sent on:
- 23 Apr 2008

 kernel/sched.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

2a1e54c660bb9c574133bda95468b1ea28239e95 diff --git a/kernel/sched.c b/kernel/sched.c
index 57ba7ea..19df986 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -757,14 +757,14 @@ const_debug unsigned int sysctl_sched_features =
 #define SCHED_FEAT(name, enabled)	\
 	#name ,
 
-__read_mostly char *sched_feat_names[] = {
+static __read_mostly char *sched_feat_names[] = {
 #include "sched_features.h"
 	NULL
 };
 
 #undef SCHED_FEAT
 
-int sched_feat_open(struct inode *inode, struct file *filp)
+static int sched_feat_open(struct inode *inode, struct file *filp)
 {
 	filp->private_data = inode->i_private;
 	return 0;

--

Adrian, you've been doing these "make needlessly global code static" 
patches for years meanwhile. I'm asking whether you have made any 
thoughts about going to the next level and automating (or at least 
half-automating) the generation of these trivial patches, so that they 
become less disruptive to our patch flow?

My opinion is that the technical benefit of these patches is positive to 
Linux: they make the kernel image a tiny bit smaller - for example this 
sched.c patch of yours i'm replying to makes the 64-bit kernel 8 bytes 
smaller. So i always applied your patches, and will do so in the future 
too.

But the per patch benefit is arguably extremely low: for example this 
particular patch saves 0.00016% from my particular vmlinux's size. Even 
on a Linux-Tiny kernel config, it's just 0.0008% of the vmlinux's size. 
We'd need more than 600 such patches (!) to make just 0.1% of a code 
size difference to my vmlinux, and even 0.1% is still very, very small. 
[ and many of these patches do not even trigger any savings on tiny 
  configs. ]

The point that many kernel developers make is that if we compare this 
small benefit to the disruption to the workflow this many small patches 
can cause, they could in fact become a net negative contribution (!). So 
it is extremely important to reduce the disruption, as much as we can.

And unlike other kernel developers, my opinion is not that we should 
eliminate this disruption by not doing these trivial patches _at all_. 
My opinion is that we should make it easier for maintainers to _avoid 
introducing_ these problems.

I.e. we need to fight the root of this problem (the steady introduction 
of needlessly global symbols), not its symptoms (the needlessly global 
symbols themselves).

Let me raise some thoughts about what we could do to achieve these 
goals.

Firstly, the methodology: you are using "make namespacecheck" on an 
allyesconfig kernel to find these 'needlessly global' sites, correct? Do 
you perhaps also generate...
[ message continues ]

I don't think the code changes actually with current gcc for integer
code if you change something from global to static (unless it causes
gcc to inline the function, but then it might be well larger if you're
unlucky)

The only file size change you'll see will be from a smaller symbol
table in the vmlinux ELF file, but that is not even loaded at run time
or included into the bzImage (and the kallsyms table has statics too)

So if we follow that "smaller symbol table" is better model
should we make a rule that all globals be 8 characters only? Or perhaps 6?  @)
I'm sure that could be easily enforced by some tool... 

I could see some advantage from static in future compiler versions 
though from better optimization, but it's quite remote.

I agree with your point that it's not effective to spend human time
to generate such patches.

-Andi
--

It's a common case that a function has only one caller. It should always 

I'm not attaching size change information to these patches since 
whatever change one sees anyway also depends on other factors like
the exact kernel configuration, so it's non-trivial to get numbers
that could be taken seriously.

There are many small aspects, e.g. both gcc with -Wmissing-prototypes 
and sparse give warnings, and the problem might either be needlessly 
global code or the fact that a function prototype is either not in a 
header or the header not #include'd by the file. Although I've only
2 or 3 times catched such bugs in the kernel that is a nasty to debug 

The best case I've actually seen in practice was a variable I made 
static, and with CONFIG_DEBUG_FOOBAR=n gcc was now able to prove that 
the value never changed resulting in the variable plus quite a chunk

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

--

At least for the scheduler patch that started this thread this was not
the case. One was a global variable and the other was a callback from

Yes it's good to catch those. However I suspect there are better tools
for that that do it less work intensive. Traditionally in the Unix
world "lint" has been used to track this kind of bugs (dating back from
before prototypes were added to C). Now running any lint over the kernel
source would result in a incredibly number of warnings I'm sure, but
perhaps one can be configured to only output warnings related to
inconsistent prototypes over files. There are a couple of free lints

Sounds like the variable should just have been removed then in the source?

-Andi
--

No, an #ifdef CONFIG_DEBUG_FOOBAR guarded some sysctl that allows users 
to change the value.


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

--

I could have announced this patch as "fixes two sparse warnings"
(which is true and seems to even make Linus bypass maintainers for 
applying patches for similar patches I've seen from other people).

Fact is that I only very rarely use sparse, do the things from different 
perspectives, and only the result often happens to overlap with what 
sparse does.

If the labeling is what makes you say "benefit is arguably extremely 
low" I can put more emphasize on which of my patches also fix sparse 



- "make namespacecheck"
- "make export_report"
- the -Wmissing-prototypes gcc flag

The latter is what I actually started with many years ago (that might 
even predate sparse), and we are slowly getting nearer to being able to 
enable it in the kernel (also thanks to sparse now emitting warnings 
for the same things).


Not exactly allyesconfig since my compile tests predate the 
introduction of the *config targets in 2.5, but my standard

I grep the sources and check in git what happened and what to do
(this sometimes finds some real bugs).

I edit the file(s).

Then I do:
  cg-commit < /dev/null; git-show --pretty=oneline > /tmp/patch-static-sched

Write the email with mutt+nano.

Store the email in my postponed-msgs folder.

Give a batch of patches some compile testing and check the output 
(e.g. removing some code might make gcc complain about now unused 

You can implement whatever you want to implement.


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

--

On Mon, 5 May 2008 22:19:06 +0200

We don't actually care about what comes out of `make namespacecheck'.  We
care about the _difference_ in its output when a patch is applied.

So a script which reports on what changes a particular patch has upon
namespacecheck output might be the way to go.  If it is fast enough then it
can be run on a per-patch basis alongside checkpatch.

--

"make namespacecheck" works on the binary objects.

- touching header files can result in a complete rebuild
- if a patch alters which objects get built you should start with
  a clean object dir

The question is therefore basically whether a complete rebuild of an
all*config kernel is fast enough for you...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

--

On Tue, 6 May 2008 00:07:12 +0300

That would be quite a bother.

I do think that we should aim to get these things fixed _before_ the
offending patches get into mainline.  It's dopey to append a sprinkle of
fixups against any particular patch after it has hit mainline when we have
the tools to fix those things up beforehand.

And it'd help to educate submitters to check their own stuff.  So when
these post-facto fixups are prepared then it is good to rub people's
noses^W^W^Wgently remind submitters about the problems in their work.
Probably you are already doing this.



Actually, we could perhaps do a lot of this at the checkpatch level?  If
checkpatch sees a global symbol being added and the same patch does not add
references to that symbol from a different file then whine.  Obviously this
will generate false positives but that's OK.
--

On Mon, 5 May 2008 14:26:04 -0700

or.. doesn't add it to a header file. That might be even more generic;
(and enforces a "all global functions need a prototype in a header
somewhere)
--

That does sound possible.  I am sure it will false positive quite a lot,
but its probabally worth a stab.

-apw
--

Not sure what is possible at the checkpatch level.

Adding -Wmissing-prototypes to the CFLAGS (which was my original 
motivation for doing these patches) would help much, but it's still a 
long road until I can propose it without being lynched for the warnings 
it still generates...

Or teach people to use sparse?

After all, this thread erupted on a patch against kernel/sched.c
that fixes something sparse also warns about.

I just tried running sparse against this file - looks scary...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

--

in some automated ways...

We have Documentation/SubmitChecklist that we hope that patch
submitters will use, but we have little evidence that they (we) do
use it, and we have some evidence that they (we) do NOT use it.
(but this isn't automated)

I see being related to DaveM's "Slow down, please" thread.
We have developers hurriedly writing new code but not paying enough
attention to code that they have already written.
(not directed at anyone in particular)

E.g., you do maybe 200 randconfig builds per day.  I only do

Good discussion material.


I have a similar problem with trying to keep kernel documentation
(kernel-doc & docbook) in sync with source code changes.
I try to review all patches that contain Documentation/ changes
(OK, I'm a few emails behind on that).  Anyway, it seems to be
a never-ending battle.

-- 
~Randy
--

On Mon, 5 May 2008 13:40:53 -0700 (PDT)


can we do a "make patchcheck" kernel build target that would
* run checkpatch on teh patch
* build the kernel without the patch (in various .configs, probably
allyesconfig / allmodconfig is enough, but we can figure this out
later)
* apply the patch
* build the kernel in the same configs
* build a kernel for install that has the 'standard debug options' on
  (lockdep, slabpoison etc)
then we can
* compare if new gcc warnings got introduced
* compare if major stack usage got introduced
* compare if namespace_check and some of the others introduce new issues
* compare if new sparse warnings got introduced
and maybe even run a bloat-o-meter to show code growth/shrinkage
[insert other useful checks here]

if all of that is just one command away, I bet quite a few people would
use it
(and the more useful it gets the more people will use it)

yes you can do the same thing by hand.
but yes it's many steps that are cumbersome if not automated... so few
people will do it.

If it's all in one step... 


--

On Mon, 5 May 2008 14:51:32 -0700

since people seem to be at least somewhat interested; I've started a
shell script that will do (for now, part of) the above.

It's very very premature, for one it is missing any and all error
checking, but it's a start. It's getting late here.. .maybe someone
wants to improve it before I wake up ? ;)

From: Arjan van de Ven <arjan@linux.intel.com>
Date: Mon, 5 May 2008 19:14:40 -0700
Subject: [PATCH] first stab at a "build with/without patch and report new non-statics and new unused exports" script

---
 dopatchtest.sh           |   63 ++++++++++++++++++++++++++++++++++++++++++++++
 scripts/export_report.pl |   20 ++++++++++++--
 scripts/namespace.pl     |   26 +++++++++++++------
 3 files changed, 98 insertions(+), 11 deletions(-)
 create mode 100644 dopatchtest.sh

diff --git a/dopatchtest.sh b/dopatchtest.sh
new file mode 100644
index 0000000..9362493
--- /dev/null
+++ b/dopatchtest.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+
+echo "cleaning trees"
+make mrproper
+
+rm -rf before after
+mkdir before before/allyesconfig  before/allmodconfig before/allnoconfig &> /dev/null
+mkdir after after/allyesconfig after/allmodconfig  after/allnoconfig &> /dev/null
+
+function build {
+	echo "   building $2"
+	make O=$1/$2 $2 &> $1/$2/config.log
+	make O=$1/$2 -j16 -s &> $1/$2/build.log
+	export srctree=$PWD
+	pushd $1/$2 > /dev/null
+	perl ../../scripts/namespace.pl --compact &> namespace.log
+	perl ../../scripts/export_report.pl -u &> export_report.log
+	popd > /dev/null
+	make O=$1/$2 checkstack &> $1/$2/checkstack.log
+}
+
+function check_static {
+	verbose=0
+	diff before/$1/namespace.log after/$1/namespace.log | grep -q "^>" && verbose=1
+	if [ $verbose -eq 1 ]; then
+		echo "New global symbols without users:"
+		diff before/$1/namespace.log after/$1/namespace.log | grep "^>" | cut -c2-
+	fi
+}
+
+function check_exports {
+	verbose=0
+	diff before/$1/export_re...
[ message continues ]

I'm not sure we could do it for every single patch (because of the
time it would take), but how about automating it so that for every
single tree which is getting pushed to linux-next, we have a build
tree which automatically builds "origin..next" (i.e., the set of
commits that are being proposed for pushing into mainline), comparing
whether the current set of changes being proposed for pushing to
mainline, for each tree.   

Ideally the maintainer would do this himself before nominating the set
of patches to linux-next, but if not, if someone were interested in
doing this work automatically, and then sending the results to the
developer and cc'ed to LKML as a service, it would probably serve a
useful "gentle nudge" towards doing the right thing.  :-)

       	       	      	      	    	      - Ted
--

On Mon, 5 May 2008 19:45:15 -0400

I don't think build power is an actual problem for things like this,
since it tends to distribute really well.
--

keep listing the checks you want to have done. I'll bet that if you can 
automate it enough you will have people setting up farms to do the 
compiles (or, ideally, package it with a seti-at-home type of thing and 
many people will donate spare cycles to grinding away at it)

how much value is lost if this is run on larger chunks? during the merge 
window the number of patches will exceed anyone's capability to check each 
one individually (not to mention it being a little late), but I think 
there would still be value in checking larger groups of patches.

David Lang
--

On Mon, 5 May 2008 16:19:13 -0700 (PDT)

only some; if the output is a filename/linenumber (and some message) we
can always do "git blame" on when it changed last.
Heck we could then automate to run at that point again to see if that
patch really caused it (similar to bisect but a lot more targeted).

This of course breaks if you do it once a year... but if you do it
"small enough" it shouldn't be that bad... even during a merge window
it'll be fine.
--