login
Search
Hello, Lee Schermerhorn was trying to use percpu from slab.h and ran into a dependency loop. percpu.h was using slab.h for UP inline implementation which isn't a big deal in itself but it turns out that percpu.h ends up being included everywhere via module.h and sched.h. So, removing that implicit inclusion breaks a lot of files. The following git tree contains trial conversion on x86_64. allmodconfig builds fine on it but a lot of other archs are likely to break although fixing them up shouldn't be too hard. git://git.kernel.org/pub/scm/linux/kernel/git/tj/percpu.git review-remove-implied-slab-inclusion This can be solved much easier by moving some of the stuff that's necessary for slab.h from percpu.h into percpu-defs.h which originally got separated so that it can be used by asm/percpu.h but it's hackish and for longer term, it would be better to have slab.h explicitly included where necessary. So, what do you guys think? Probably-the-right-thing-to-do >544 file patch or somewhat-ugly-but-let's-worry-about-it-tommorrow two file patch? I omitted the patch itself as it's mind numbingly boring and long. Thanks. Patch description and diffstat follows. Patch content omitted. =============================================================== From b0bcd3a0e6a49077a2e7b073831fbae9e8f87b4b Mon Sep 17 00:00:00 2001 From: Tejun Heo <tj@kernel.org> Date: Thu, 11 Mar 2010 23:27:38 +0900 Subject: [PATCH] percpu: break implied slab.h inclusing via percpu.h percpu.h has always been including slab.h to get k[mz]alloc/free() for UP inline implementation. percpu.h being used by very low level headers including module.h and sched.h, this meant that a lot files unintentionally got slab.h inclusion. Lee Schermerhorn was trying to make slab.h use percpu.h and got bitten by this implicit inclusion. Apparently, the right thing to do is breaking this ultimately unnecessary implicit inclusion; however, because it has been there for so long, removing the implicit ...
You can include slab.h only for UP case. Since everyone tests on allmodconfig which has SMP=y, configuration will be more strict wrt headers, and compile breakages amount negligible. --
Hello, But that wouldn't change anything about having to do an oneshot huge change, right? And, if we're gonna do that anyway, I think it would be better to remove the implicit dependency for UP case too so that for example slab.h in this case doesn't have to do ifdef on CONFIG_SMP before using percpu accessors. Thanks. -- tejun --
( /me mumbles something about not having a patch in the email to review and pulling the tree. 200k patch is just fine for lkml - i've attached it below for easier review. percpu.h and percpu.c has the meat of the changes. ) i like the dependency reduction. Noticed one small detail: this new 2000-lines #ifdef block percpu.c: +#ifdef CONFIG_SMP +#else /* CONFIG_SMP */ +#endif /* CONFIG_SMP */ feels a bit lame. A separate percpu_up.c file would be nicer i suppose? Also, why should we make this opt-in and expose a wide range of configs to build breakages? 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. You can map the pattern experimentally: the insertion pattern could be built from the x86 allmodconfig build you did [i.e. extend the pattern until you make it build on allmodconfig] - that would cover most cases in practice (not just allmodconfig) - and would cover most architectures as well. Ingo diff --git a/arch/x86/ia32/sys_ia32.c b/arch/x86/ia32/sys_ia32.c index 74c3543..626be15 100644 --- a/arch/x86/ia32/sys_ia32.c +++ b/arch/x86/ia32/sys_ia32.c @@ -40,6 +40,7 @@ #include <linux/ptrace.h> #include <linux/highuid.h> #include <linux/sysctl.h> +#include <linux/slab.h> #include <asm/mman.h> #include <asm/types.h> #include <asm/uaccess.h> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 3a4bf35..1a160d5 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -8,6 +8,7 @@ #include <linux/vmalloc.h> #include <linux/memory.h> #include <linux/stop_machine.h> +#include <linux/slab.h> #include <asm/alternative.h> #include <asm/sections.h> #include <asm/pgtable.h> diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c index 8aa65ad..5191860 100644 --- a/arch/x86/kernel/apic/nmi.c +++ b/arch/x86/kernel/apic/nmi.c @@ -26,6 +26,7 @@ #include <linux/kernel_stat.h> ...
Hello, Ingo. I wanted to keep the discussion high level while giving a general idea about the extent of necessary changes. I'll include the patch from I don't really get the 'experimental' part but if I count all the files which ends up including percpu.h directly or indirectly on allmodconfig it ends up including much more .c files than necessasry - 11203 to be exact, ~20 times more than necessary. Inclusions from .c files definitely are much less troublesome so the situation would be better than now but we'll still end up with a LOT of bogus inclusions without any good way to eventually remove them. 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. Thanks. -- tejun --
Hello, 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 --
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
--
It's question only for you. There are observations, like sched.h includes some VM routines which better live in mm.h and some some cred/UID routines which better live in cred.h and sched.h unnecessarily includes capability.h which wasn't The problem is that slab.h is included unnecessarily in some other headers conditionally and unconditionally. It's amazing to see how much sidework people are willing to create to _not_ do simple and obvious thing -- just remove slab.h. --
I'm not following your idea here. Where do you propose moving kmalloc() and friends to? kernel.h? --
That raises another problem we have: based on the sanitization of #include
lines in a couple of files in the past, about 70-80% [+-10%] of all include
lines are superfluous and duplicative.
So besides include file dependency incest, we have a random #include mess at
the top of virtually every .c file in the kernel that has been around for more
than a couple of years.
Yeah, actual API usages would be quite good as an insertion pattern. I've done
a good deal of such large-scale conversions in the past, and what worked (for
me) best was along the lines of:
- step 1: shoot for an all-tree scripted conversion (which tries to overshoot
the target, not under-shoot it)
- step 2: some good build testing as there's always a few exceptions not
worth scripting
The solution you went for is good for an initial prototype, but i'd expect it
to cause quite some build breakage that will be a shock to the system.
The shock can be avoided i think, with some more work (on your side :-/ ).
Thanks,
Ingo
--
I am all for untangling the #include mess in slab.h and friends and I
think Ingo's suggestion is probably the best way forward. We should
avoid creating tree-wide breakage for this kind of cleanups.
Pekka
--
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 --
Sigh. If you want to skip compile testing, just say so in changelog. --
Hey, "sigh" right back at you, Alexey!
I am not saying I want to skip "compile testing", I'm just stating the
obvious fact: for a header such as slab.h that's implicitly needed in
pretty much everywhere, it's very difficult to find all the relevant
broken configurations. I don't see the benefit of breaking the world
for this cleanup as Ingo's suggestion will eventually get us to the
exact same situation _without_ causing tree-wide breakage.
Pekka
--
Yes. In large-scale conversions i typically used (rather extensive) build-testing as a tool to check a script's correctness - distinctly _not_ to create the actual patch itself. I.e. it's an adaptive feedback loop in essence: the script gets perfected by repeated build tests, and the end result is that we have a scripted conversion that covers more code than build testing is able to reach (it covers not just x86, covers rare config combos, etc.), _plus_ we also have the final proof of the pudding via the actual build tests. In the end it all converges nicely and the build breakage exported is minimal. That is the mechanism i suggested. Alexey calls it 'smartass', i call it a defensive approach to large-scale changes, that we should practice more, not less. [ Or, sometimes, for visibly trivial matters i just take the gamble, go into cowboy mode and say 'to the heck, let others find those bugs' and do a change based on a sed -i oneliner and build a few configs, fix up the breakages it finds and hope for the best. I dont complain when i get slapped for that though ;-) ] Ingo --
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@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@elte.hu>
[ Combined several one-liners from Ingo into one single patch - Linus ]
Signed-off-by: Linus Torvalds <torvalds@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 ...The problem is that percpu.h includes slab.h for the UP case. slab.h does not use percpu.h. Lee was trying to have topology.h use percpu.h which fails because percpu.h uses gfp.h which in turn uses topology.h again. Did something change there or does the description need an update? --
Hello, Yes, that's the reason why slab.h is included in percpu.h But apparently a lot of files depend on getting slab.h through percpu.h and removing slab.h causes a lot of build breakages UP or I don't find anything too wrong about the description? Thanks. -- tejun --
"Lee Schermerhorn nwas trying to use percpu from slab.h and ..." --
Well, indirectly, I was including percpu.h in slab.h by way of gpf.h/topology.h. Not actually what I was *trying* to do. s/from slab.h/from topology.h/ is the update Christoph is indicating. --
OIC, I'll update accordingly. Thanks. -- tejun --
