[2.6 patch] make sched_feat_{names,open} static

[2.6 patch] make sched_feat_{names,open} static

[Adrian Bunk]

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;

[rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

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

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 them via Sparse, or via some other special 
scripting? Could you perhaps describe how you generate them? Do you 
create the patches manually perhaps?

We could extend "make namespacecheck" to emit a patch that just marks 
all symbols static that are needlessly global.

Firstly, the practical problem: today "make namespacecheck" emits way 
too many false positives even on an allyesconfig build - this is due to 
some symbols only being utilized on certain config variations and on 
certain architectures. A healthy set of false positives has accumulated 
during the past few years.

Nowhere do we truly document the symbols that are _purposefully_ global 
but "make namespacecheck" complains about them. If we documented them we 
could improve the documentation of the kernel.

The (rather trivial) annotation would look like this:

   __kernel_api int arch_reinit_sched_domains(void)

this marks the arch_reinit_sched_domains symbol in kernel/sched.c as 
"intentionally global" - and it would cause "make namespacecheck" to not 
emit warnings about that symbol by default. These annotations are for 
global symbols that are not always utilized in an allyesconfig build, 
but which we still want to keep global nevertheless.

( Note that because we'd only annotate the sites that trigger "make
  namespacecheck" on an allyesconfig build the overwhelming majority of 
  kernel-internal APIs would stay untouched. )

My estimation is that only about 1 out of 10 new symbols that "make 
namespacecheck" complains about in a new kernel release needs to be 
annotated this way. I.e. we could turn 10 trivial "global => static" 
patches into 1 trivial annotation patch => a 10x reduction in the rate 
of patches!

Secondly, we could introduce a new "make namespacecheck_fix" check 
method. Maintainers could (optionally) use it the following way:

    make namespacecheck_fix arch/x86/
    make namespacecheck_fix kernel/sched.c

this would take the namespacecheck output and would apply it to the 
source - basically emitting a patch.

Subsystem maintainers do not _have to_ run this, it's not in any way 
compulsory (they could just opt to wait for your patches), it's just an 
optional tool like the many other tools we have in the kernel today that 
make life easier for maintainers.

I can see five immediate benefits from all this:

- this way we can push back these trivial patches to the subsystem 
  maintainers who introduce them, and it would not pollute our global 
  workflow at all.

- due to the annotation multiple such trivial changes could be collapsed 
  into a single patch per subsystem - reducing the per patch maintenance 
  overhead even more. The maintainer only needs to check whether it 
  contains some new, intentionally-global symbol. (this is relatively 
  rare - the majority of new global symbols is accidental)

- we'd also document the kernel better, via those "purposefully global" 
  __kernel_api annotations.

- we could concentrate these changes to a single point in time during 
  the kernel cycle - just like we do documentation and typo patches in a 
  single point too. We'd do fewer patches and we'd do them less 
  frequently. That too reduces disruption and increases the net positive 
  effects of these patches.

- another benefit would be for newbies: they could run this tool and 
  could generate patches - saving you and kernel maintainers from having 
  to spend quality time on such trivial issues, and giving kernel 
  newbies an easy first introduction to Linux kernel modifications.

What do you think?

	Ingo

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Randy.Dunlap]

On Mon, 5 May 2008, Ingo Molnar wrote:

> 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).

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
20 - 50, but we both find too many problems (IMHO).

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

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Andrew Morton]

On Mon, 5 May 2008 22:19:06 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> Firstly, the practical problem: today "make namespacecheck" emits way 
> too many false positives even on an allyesconfig build

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.

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Adrian Bunk]

On Mon, May 05, 2008 at 10:19:06PM +0200, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > This patch makes the following needlessly global code in kernel/sched.c  
> > static:
> > - sched_feat_names[]
> > - sched_feat_open()
> 
> 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.


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 
warnings.


> And unlike other kernel developers, my opinion is not that we should 
> eliminate this disruption by not doing these trivial patches _at all_. 


Who are these "other kernel developers"?


> 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 

- "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).

And it might even be the automation you are looking for.

> allyesconfig kernel to find these 'needlessly global' sites, correct?

Not exactly allyesconfig since my compile tests predate the 
introduction of the *config targets in 2.5, but my standard
.config's are roughly identical to allmodconfig and allyesconfig.

> Do 
> you perhaps also generate them via Sparse, or via some other special 
> scripting? Could you perhaps describe how you generate them? Do you 
> create the patches manually perhaps?

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 
static code).

>...
> What do you think?

You can implement whatever you want to implement.

If that results in me sending less patches that's not a problem for me.

> 	Ingo

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Adrian Bunk]

On Mon, May 05, 2008 at 01:42:52PM -0700, Andrew Morton wrote:
> On Mon, 5 May 2008 22:19:06 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > Firstly, the practical problem: today "make namespacecheck" emits way 
> > too many false positives even on an allyesconfig build
> 
> 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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Andrew Morton]

On Tue, 6 May 2008 00:07:12 +0300
Adrian Bunk <bunk@kernel.org> wrote:

> On Mon, May 05, 2008 at 01:42:52PM -0700, Andrew Morton wrote:
> > On Mon, 5 May 2008 22:19:06 +0200
> > Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > Firstly, the practical problem: today "make namespacecheck" emits way 
> > > too many false positives even on an allyesconfig build
> > 
> > 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...
> 

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.

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Adrian Bunk]

On Mon, May 05, 2008 at 02:26:04PM -0700, Andrew Morton wrote:
> On Tue, 6 May 2008 00:07:12 +0300
> Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Mon, May 05, 2008 at 01:42:52PM -0700, Andrew Morton wrote:
> > > On Mon, 5 May 2008 22:19:06 +0200
> > > Ingo Molnar <mingo@elte.hu> wrote:
> > > 
> > > > Firstly, the practical problem: today "make namespacecheck" emits way 
> > > > too many false positives even on an allyesconfig build
> > > 
> > > 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...
> > 
> 
> 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.

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Arjan van de Ven]

On Mon, 5 May 2008 14:26:04 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

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

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)

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Arjan van de Ven]

On Mon, 5 May 2008 13:40:53 -0700 (PDT)
"Randy.Dunlap" <rdunlap@xenotime.net> wrote:

> On Mon, 5 May 2008, Ingo Molnar wrote:
> 
> > 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).
> 
> 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
> 20 - 50, but we both find too many problems (IMHO).
> 
> > Let me raise some thoughts about what we could do to achieve these 
> > goals.
> 
> Good discussion material.
> 


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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[david]

On Mon, 5 May 2008, Arjan van de Ven wrote:

> On Mon, 5 May 2008 13:40:53 -0700 (PDT)
> "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
>
>> On Mon, 5 May 2008, Ingo Molnar wrote:
>>
>>> 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).
>>
>> 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
>> 20 - 50, but we both find too many problems (IMHO).
>>
>>> Let me raise some thoughts about what we could do to achieve these
>>> goals.
>>
>> Good discussion material.
>>
>
>
> 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...
>

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Arjan van de Ven]

On Mon, 5 May 2008 16:19:13 -0700 (PDT)
david@lang.hm wrote:

> On Mon, 5 May 2008, Arjan van de Ven wrote:
> 
> > On Mon, 5 May 2008 13:40:53 -0700 (PDT)
> > "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> >
> >> On Mon, 5 May 2008, Ingo Molnar wrote:
> >>
> >>> 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).
> >>
> >> 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
> >> 20 - 50, but we both find too many problems (IMHO).
> >>
> >>> Let me raise some thoughts about what we could do to achieve these
> >>> goals.
> >>
> >> Good discussion material.
> >>
> >
> >
> > 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...
> >
> 
> 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? 

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.

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Theodore Tso]

On Mon, May 05, 2008 at 02:51:32PM -0700, Arjan van de Ven wrote:
> 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)

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> writes:

> But the per patch benefit is arguably extremely low: for example this 
> particular patch saves 0.00016% from my particular vmlinux's size.

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Arjan van de Ven]

On Mon, 5 May 2008 19:45:15 -0400
Theodore Tso <tytso@mit.edu> wrote:

> On Mon, May 05, 2008 at 02:51:32PM -0700, Arjan van de Ven wrote:
> > 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)
> 
> I'm not sure we could do it for every single patch (because of the
> time it would take), 

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Arjan van de Ven]

On Mon, 5 May 2008 14:51:32 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

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

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_report.log after/$1/export_report.log | grep -q "^>" && verbose=1
+	if [ $verbose -eq 1 ]; then
+		echo "New unused exports:"
+		diff before/$1/export_report.log after/$1/export_report.log | grep "^>" | cut -c2-
+	fi
+}
+
+
+echo "Before series"
+#build before allnoconfig
+build before allyesconfig
+build before allmodconfig
+
+echo "Applying patch"
+cat $1 | patch -p1
+
+echo "After series"
+
+#build after allnoconfig
+build after allyesconfig
+build after allmodconfig
+
+echo "Reverting patch"
+cat $1 | patch -p1  -R
+
+echo 
+echo "Running checks"
+check_static allyesconfig
+check_exports allmodconfig
+
diff --git a/scripts/export_report.pl b/scripts/export_report.pl
index 705b5ba..a2f8b8d 100644
--- a/scripts/export_report.pl
+++ b/scripts/export_report.pl
@@ -44,7 +44,8 @@ sub usage {
 	      "\t-h: print detailed help\n",
 	      "\t-k: the path to Module.symvers file. By default uses ",
 	      "the file from the current directory\n",
-	      "\t-o outputfile: output the report to outputfile\n";
+	      "\t-o outputfile: output the report to outputfile\n",
+	      "\t-u: display only unused symbols\n";
 	exit 0;
 }
 
@@ -57,7 +58,7 @@ sub collectcfiles {
 
 my (%SYMBOL, %MODULE, %opt, @allcfiles);
 
-if (not getopts('hk:o:f',\%opt) or defined $opt{'h'}) {
+if (not getopts('huk:o:f',\%opt) or defined $opt{'h'}) {
         usage($0);
 }
 
@@ -81,6 +82,11 @@ if (defined $opt{'o'}) {
 	}
 	select OUTPUT_HANDLE;
 }
+
+my $unused_only = 0;
+if (defined $opt{'u'}) {
+	$unused_only = 1;
+}
 #
 # collect all the symbols and their attributes from the
 # Module.symvers file
@@ -127,6 +133,7 @@ foreach my $thismod (@allcfiles) {
 	close(MODULE_MODULE);
 }
 
+if ($unused_only <= 0) {
 print "\tThis file reports the exported symbols usage patterns by in-tree\n",
 	"\t\t\t\tmodules\n";
 printf("%s\n\n\n","x"x80);
@@ -137,12 +144,16 @@ printf("%s\n\n\n","x"x80);
 printf("SECTION 1:\tThe exported symbols and their usage count\n\n");
 printf("%-25s\t%-25s\t%-5s\t%-25s\n", "Symbol", "Module", "Usage count",
 	"export type");
-
+}
 #
 # print the list of unused exported symbols
 #
 foreach my $list (sort alphabetically values(%SYMBOL)) {
 	my ($module, $value, $symbol, $gpl) = @{$list};
+	
+	if ($value > 0 && $unused_only > 0) {
+		exit;
+	}
 	printf("%-25s\t%-25s\t%-10s\t", $symbol, $module, $value);
 	if (defined $gpl) {
 		printf("%-25s\n",$gpl);
@@ -152,6 +163,9 @@ foreach my $list (sort alphabetically values(%SYMBOL)) {
 }
 printf("%s\n\n\n","x"x80);
 
+if ($unused_only > 0) {
+ exit;
+}
 printf("SECTION 2:\n\tThis section reports export-symbol-usage of in-kernel
 modules. Each module lists the modules, and the symbols from that module that
 it uses.  Each listed symbol reports the number of modules using it\n");
diff --git a/scripts/namespace.pl b/scripts/namespace.pl
index c6e88c6..f512d8a 100755
--- a/scripts/namespace.pl
+++ b/scripts/namespace.pl
@@ -70,12 +70,15 @@ my $nm = ($ENV{'NM'} || "nm") . " -p";
 my $objdump = ($ENV{'OBJDUMP'} || "objdump") . " -s -j .comment";
 my $srctree = "";
 my $objtree = "";
+
+my $compact = 0;
 $srctree = "$ENV{'srctree'}/" if (exists($ENV{'srctree'}));
 $objtree = "$ENV{'objtree'}/" if (exists($ENV{'objtree'}));
 
-if ($#ARGV != -1) {
-	print STDERR "usage: $0 takes no parameters\n";
-	die("giving up\n");
+if ($ARGV[0]) {
+	if ($ARGV[0] eq "--compact") {
+		$compact = 1;
+	}
 }
 
 my %nmdata = ();	# nm data for each object
@@ -192,7 +195,7 @@ sub do_nm
 		}
 		close(OBJDUMPDATA);
 		if (!defined($comment) || $comment !~ /GCC\:.*GCC\:/m) {
-			printf STDERR "No source file found for $fullname\n";
+			printf STDERR "No source file found for $fullname\n" if $compact == 0;
 		}
 		return;
 	}
@@ -288,7 +291,7 @@ sub do_nm
 			&& $fullname ne "fs/ntfs/sysctl.o"
 			&& $fullname ne "fs/jfs/jfs_debug.o"
 		) {
-			printf "No nm data for $fullname\n";
+			printf "No nm data for $fullname\n" if $compact == 0;
 		}
 		return;
 	}
@@ -317,6 +320,9 @@ sub drop_def
 sub list_multiply_defined
 {
 	my ($name, $module);
+	if ($compact == 1) {
+		return;
+	}
 	foreach $name (keys(%def)) {
 		if ($#{$def{$name}} > 0) {
 			# Special case for cond_syscall
@@ -411,6 +417,7 @@ sub resolve_external_references
 					&& $name !~ /^__mod_page_state/
 					&& $name !~ /^init_module/
 					&& $name !~ /^cleanup_module/
+					&& $compact == 0
 				) {
 					printf "Cannot resolve ";
 					printf "weak " if ($type eq "w");
@@ -437,9 +444,8 @@ sub list_extra_externals
 		}
 	}
 	if (%noref) {
-		printf "\nExternally defined symbols with no external references\n";
+		printf "\nExternally defined symbols with no external references\n" if $compact == 0;
 		foreach $module (sort(keys(%noref))) {
-			printf "  $module\n";
 			foreach (sort(@{$noref{$module}})) {
 				if (exists($export{$_})) {
 					$export = " (export only)";
@@ -447,7 +453,11 @@ sub list_extra_externals
 				else {
 					$export = "";
 				}
-				printf "    $_$export\n";
+				if ($compact>0) {
+					printf "$module	$_$export\n";
+				} else {
+					printf "	$_$export\n";
+				}
 			}
 		}
 	}
-- 
1.5.4.5

Re: [rfc] the kernel workflow & trivial "global -> static" patches

[Adrian Bunk]

On Tue, May 06, 2008 at 02:21:31AM +0200, Andi Kleen wrote:
> 
> 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)

It's a common case that a function has only one caller. It should always 
be an (at least tiny) space win to get them inlined.

> 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)
>...

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 
class of bugs and gcc can find such problems at compile time.

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

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
of code no longer emitted.

> -Andi

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

Re: [rfc] the kernel workflow & trivial "global -> static" patches (was: Re: [2.6 patch] make sched_feat_{names,open} static)

[Andy Whitcroft]

On Mon, May 05, 2008 at 02:46:25PM -0700, Arjan van de Ven wrote:
> On Mon, 5 May 2008 14:26:04 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > 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.
> 
> 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

Re: [rfc] the kernel workflow & trivial "global -> static" patches

[Andi Kleen]

Adrian Bunk wrote:
> On Tue, May 06, 2008 at 02:21:31AM +0200, Andi Kleen wrote:
>> 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)
> 
> It's a common case that a function has only one caller. It should always 
> be an (at least tiny) space win to get them inlined.

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
proc. Both cannot be inlined.

> 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 
> class of bugs and gcc can find such problems at compile time.

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
like the one in NetBSD or splint.	

>> I could see some advantage from static in future compiler versions 
>> though from better optimization, but it's quite remote.
>> ...
> 
> 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
> of code no longer emitted.

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

-Andi

Re: [rfc] the kernel workflow & trivial "global -> static" patches

[Adrian Bunk]

On Tue, May 06, 2008 at 01:13:01PM +0200, Andi Kleen wrote:
> Adrian Bunk wrote:
> > On Tue, May 06, 2008 at 02:21:31AM +0200, Andi Kleen wrote:
>...
> >> I could see some advantage from static in future compiler versions 
> >> though from better optimization, but it's quite remote.
> >> ...
> > 
> > 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
> > of code no longer emitted.
> 
> Sounds like the variable should just have been removed then in the source?

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

(I'm not claiming this was a common case.)

> -Andi

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