Prev: GFS2: Pull request (fixes)
Next: [PATCH] Drivers: pci: code style cleanup
From: Alexey Dobriyan on 16 Mar 2010 03:20
On Tue, Mar 16, 2010 at 8:58 AM, Pekka Enberg <penberg(a)cs.helsinki.fi> wrote:
> We should avoid creating tree-wide breakage for this kind of cleanups.

This is done by compile testing, not by being smartass.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 16 Mar 2010 03:50
Hello,

On 03/16/2010 03:54 PM, Tejun Heo wrote:
> Maybe a better way is to grab for slab API usages in .c files which
> don't have slab.h inclusion. If breaking the dependency is the way to
> go, I can definitely write up some scripts and do test builds on some
> archs. There sure will be some fallouts but I think it won't be too
> bad.

Hmmm... here are some interesting numbers. Not completely exact but
should give the general ballpark idea.

all .c files : 13999
..c files which use any of slab interface : 5603
..c files which include slab.h : 2519
..c files which include slab.h but don't use it : 577
..c files which use slab but don't include it : 3661

..c files with k[mzc]alloc/k[z]free usage : 5291
..c files with other slab interface usage : 356

C files which use k[mzc]alloc/k[z]free covers ~38% of all c files.
One possibility is to separate out those into kmalloc.h and make it
available universally via kernel.h.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Pekka Enberg on 16 Mar 2010 04:00
Alexey Dobriyan kirjoitti:
> On Tue, Mar 16, 2010 at 8:58 AM, Pekka Enberg <penberg(a)cs.helsinki.fi> wrote:
>> We should avoid creating tree-wide breakage for this kind of cleanups.
>
> This is done by compile testing, not by being smartass.

I don't think compile testing is going to scale here because slab is
used is so many places of the kernel.

Pekka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 16 Mar 2010 04:00

* Tejun Heo <tj(a)kernel.org> wrote:

> Hello,
>
> On 03/16/2010 03:54 PM, Tejun Heo wrote:
>
> > Maybe a better way is to grab for slab API usages in .c files which don't
> > have slab.h inclusion. If breaking the dependency is the way to go, I can
> > definitely write up some scripts and do test builds on some archs. There
> > sure will be some fallouts but I think it won't be too bad.
>
> Hmmm... here are some interesting numbers. Not completely exact but should
> give the general ballpark idea.
>
> all .c files : 13999
> .c files which use any of slab interface : 5603
> .c files which include slab.h : 2519
> .c files which include slab.h but don't use it : 577
> .c files which use slab but don't include it : 3661
>
> .c files with k[mzc]alloc/k[z]free usage : 5291
> .c files with other slab interface usage : 356
>
> C files which use k[mzc]alloc/k[z]free covers ~38% of all c files. One
> possibility is to separate out those into kmalloc.h and make it available
> universally via kernel.h.

Well, that's the main question: do we want all-in-one big headers like
kernel.h (and sched.h / mm.h) or not?

If we want to avoid combo .h files then we inevitably want to go for
finegrained, per subsystem data type and API definitions - i.e. explicit
slab.h inclusion in the .c file.

I'm leaning towards that solution of avoiding combo .h files - even if in the
slab.h case the concept causes us to touch quite a few .c files.

As long as it's expected to cause no shock we can do it in one big atomic
risk-free commit: which just adds the #include slab.h's to the .c files,
nothing else.

Look at the advantages:

- (slightly) sped up kbuild

- it would be immediately obvious in the future which .c file makes use of
SLAB facilities. We wouldnt have to guess or go to object file analysis to
figure out any 'true' usage.

- [ it would be trivial in the future to actually _remove_ stale #include
files. When there's no indirect inclusion of facilities then the lack of
usage can be decided based on API usage patterns in the .c files. ]

It's a much cleaner end result IMHO (which also happens to be faster to build)
- and slab.h is definitely the worst-case, so we should not wimp out just due
to that difficulty.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 16 Mar 2010 04:20

* Alexey Dobriyan <adobriyan(a)gmail.com> wrote:

> On Tue, Mar 16, 2010 at 8:17 AM, Ingo Molnar <mingo(a)elte.hu> wrote:
> > Also, why should we make this opt-in and expose a wide range of configs to
> > build breakages?
>
> Because we want to #include less, not the same amount.
>
> > A more gradual approach would be to write a simple script
> > that adds a slab.h include to all .c's that include percpu.h, directly or
> > indirectly.
>
> As a defensive measure, one can explicitly add slab.h to every file which
> uses kmalloc/kfree.
>
> But, who cares, since one still has to compile test regardless.

Firstly, generating #include slab.h lines in .c files based on actual API
usage is _good_, pretty much unconditionally so. See my previous mail for the
list of advantages. Even if it increases the number of #include lines
temporarily.

Secondly, the point in scripting is to intentionally over-shoot the target, as
compile testing (especially if it's only modconfig), will only cover so much
and _if we can_ we should avoid breakage.

That way the over-shooting will cover cases you are not able (or did not
happen to) build-test. It's much better to have temporarily more include lines
than have fewer and break the build. We'll need a #include line reduction
mechanism in any case as APIs frequently get unused and #include lines get
amassed in .c files.

Thirdly, as for build testing only, see for example a sched.h include file
cleanup _you_ did in the past, which, despite your build testing, broke quite
some code:

commit 86ae13b006e48959981248493efd3ff4b2828b3d
Author: Ingo Molnar <mingo(a)elte.hu>
Date: Mon Oct 12 16:22:46 2009 +0200

headers: Fix build after <linux/sched.h> removal

Commit d43c36dc6b357fa1806800f18aa30123c747a6d1 ("headers: remove
sched.h from interrupt.h") left some build errors in some configurations
due to drivers having depended on getting header files "accidentally".


Signed-off-by: Ingo Molnar <mingo(a)elte.hu>
[ Combined several one-liners from Ingo into one single patch - Linus ]
Signed-off-by: Linus Torvalds <torvalds(a)linux-foundation.org>

drivers/char/genrtc.c | 1 +
drivers/char/rtc.c | 1 +
drivers/char/sonypi.c | 1 +
drivers/net/wan/c101.c | 1 +
drivers/net/wan/n2.c | 1 +
drivers/net/wan/pci200syn.c | 1 +
drivers/pci/hotplug/cpqphp.h | 1 +
7 files changed, 7 insertions(+), 0 deletions(-)

I dont mind the occasional breakage, but avoidable bugs spread out in the tree
can be a real PITA to testers.

I remember it, that imperfect conversion kept me busy chasing trivial build
bugs for most of the day, because they would pop up with different config
dependencies so they had to be addressed separately. (so i created 7 separate
fixes as the day progressed - and that in a busy period of the cycle so i
certainly didnt have a day to waste voluntarily)

Note that that d43c36dc patch of yours wasnt in linux-next IIRC, so you didnt
even get basic build coverage. I have no problem with fixing more difficult
bugs or with fixing the occasional silly oversight (that's the _point_ of
developing the kernel after all), but i have a problem with reasonably
avoidable bugs.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: GFS2: Pull request (fixes)
Next: [PATCH] Drivers: pci: code style cleanup