All against recent git-x86 --
Otherwise
WARNING: vmlinux.o(.text+0x64a9): Section mismatch: reference to .init.text:machine_specific_memory_setup (between 'memory_setup' and 'show_cpuinfo')
Signed-off-by: Andi Kleen <ak@suse.de>
---
arch/x86/kernel/setup_64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/arch/x86/kernel/setup_64.c
===================================================================
--- linux.orig/arch/x86/kernel/setup_64.c
+++ linux/arch/x86/kernel/setup_64.c
@@ -244,7 +244,7 @@ static inline void __init reserve_crashk
#endif
/* Overridden in paravirt.c if CONFIG_PARAVIRT */
-void __attribute__((weak)) memory_setup(void)
+void __attribute__((weak)) __init memory_setup(void)
{
machine_specific_memory_setup();
}
--thanks, applied. Ingo --
btw., as a sidenote, all of your recent patches to lkml had one or two missing Cc:s. for lockdep patches, please Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Ingo Molnar <mingo@elte.hu> for genirq patches, please Cc: Thomas Gleixner <tglx@linutronix.de> Ingo Molnar <mingo@elte.hu> for timer/clocksource/clockevents patches please Cc: Thomas Gleixner <tglx@linutronix.de> John Stultz <johnstul@us.ibm.com> Ingo Molnar <mingo@elte.hu> for x86 patches, please Cc: Thomas Gleixner <tglx@linutronix.de> "H. Peter Anvin" <hpa@zytor.com> Ingo Molnar <mingo@elte.hu> having a complete Cc: line makes it easier to coordinate patches - especially for larger patchbombs. Thanks, Ingo --
This avoids the requirement to mark a lot of initialization functions not __cpuinit just for resume from RAM. More functions could be converted now, didn't do all. Cc: rjw@sisk.pl Cc: pavel@suse.cz Signed-off-by: Andi Kleen <ak@suse.de> --- arch/x86/Kconfig | 5 +++++ arch/x86/kernel/cpu/mtrr/main.c | 2 +- arch/x86/kernel/trampoline_64.S | 2 +- include/linux/init.h | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) Index: linux/arch/x86/kernel/trampoline_64.S =================================================================== --- linux.orig/arch/x86/kernel/trampoline_64.S +++ linux/arch/x86/kernel/trampoline_64.S @@ -34,7 +34,7 @@ #include <asm/segment.h> /* We can free up trampoline after bootup if cpu hotplug is not supported. */ -#ifndef CONFIG_HOTPLUG_CPU +#if !defined(CONFIG_HOTPLUG_CPU) && !defined(CONFIG_PM_CPUINIT) .section .init.data, "aw", @progbits #else .section .rodata, "a", @progbits Index: linux/include/linux/init.h =================================================================== --- linux.orig/include/linux/init.h +++ linux/include/linux/init.h @@ -266,7 +266,7 @@ void __init parse_early_param(void); #define __devexitdata __exitdata #endif -#ifdef CONFIG_HOTPLUG_CPU +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_PM_CPUINIT) #define __cpuinit #define __cpuinitdata #define __cpuexit Index: linux/arch/x86/Kconfig =================================================================== --- linux.orig/arch/x86/Kconfig +++ linux/arch/x86/Kconfig @@ -130,6 +130,11 @@ config GENERIC_PENDING_IRQ depends on GENERIC_HARDIRQS && SMP default y +config PM_CPUINIT + bool + depends on PM + default y + config X86_SMP bool depends on SMP && ((X86_32 && !X86_VOYAGER) || X86_64) Index: linux/arch/x86/kernel/cpu/mtrr/main.c =================================================================== --- linux.orig/arch/x86/kernel/cpu/mtrr/main...
--
That was something that irritated me too while writing the patch, but the functions I am interested in with this are referenced from arch/x86/power/cpu.c and that is obj-$(CONFIG_PM) += cpu.o So you would need to fix that first. Would be fine for me, but is out of scope for my patch. -Andi --
OK, I'll fix that up later. Thanks, Rafael --
i'll apply Andi's patch to x86.git - or would like to maintain that patch? Ingo --
[Sorry for not replying earlier, I missed your message.] x86.git would be fine. Thanks, Rafael --
Shouldn't this aready be handled by the following?
config PM_SLEEP_SMP
bool
depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
depends on PM_SLEEP
select HOTPLUG_CPU
default y
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
--Won't help for UP at least. -Andi --
I know that it's not popular to care about the kernel size, but your
patch will cost precious memory in the common case of UP embedded
systems with CONFIG_PM=y.
It seems the correct solution would be not to hijack __cpuinit
cu
Adrian
BTW: Is there any good reason why your patch is x86 only?
No matter how this gets handled, it should be an architecture
independent issue.
--
"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
--The rationale is that after suspend the CPU has to be reinitialized. That is because it is essentially like a reboot. All the previous CPU state is gone. It doesn't need to touch state in memory only -- if you want you could create a new annotation for functions that only touch memory; but I'm not sure it would buy all that much. -Andi --
But your patch does:
+config PM_CPUINIT
+ bool
+ depends on PM
+ default y
As an example, even plain ACPI support without any suspend support in
the kernel at all requires CONFIG_PM and therefore forces all __cpuinit
code to be non-__init after your patch.
And if the dependency was corrected to PM_SLEEP it will still make
the UP kernel use more memory since we currently have __cpuinit code
that gets discarded after boot but suspend/resume is apparently working.
Plus my other point that it seems to be wrong to do whatever change only
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
--That is because arch/x86/power/cpu.c where this happens is currently obj-$(CONFIG_PM) += cpu.o If it was changed to CONFIG_something else then yes that dependency should be changed too. -Andi --
Then fix this first.
And the following other points you didn't bother to reply to also still
stand even after this fix:
- already __cpuinit code will waste memory with CONFIG_PM_SLEEP=y
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
--Rafael indicated he would do that, but it is really outside the scope Don't know what your point is. Anyways if you think there is a problem CPU initialization is deeply architecture specific. I don't see much use in generalizing that. -Andi --
Yes, and I'm still going to do that. :-) Thanks, Rafael --
[Ingo, this patch applies on top of the mm branch, please add.] --- From: Rafael J. Wysocki <rjw@sisk.pl> CONFIG_PM_CPUINIT should depend on CONFIG_PM_SLEEP rather than on CONFIG_PM, because it only is needed for suspend and hibernation. Also, it's not necessary to compile arch/x86/power/cpu.c if CONFIG_PM_SLEEP is not set, for the same reason. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- arch/x86/Kconfig | 2 +- arch/x86/power/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) Index: linux-2.6/arch/x86/Kconfig =================================================================== --- linux-2.6.orig/arch/x86/Kconfig +++ linux-2.6/arch/x86/Kconfig @@ -150,7 +150,7 @@ config GENERIC_PENDING_IRQ config PM_CPUINIT bool - depends on PM + depends on PM_SLEEP default y config X86_SMP Index: linux-2.6/arch/x86/power/Makefile =================================================================== --- linux-2.6.orig/arch/x86/power/Makefile +++ linux-2.6/arch/x86/power/Makefile @@ -1,2 +1,2 @@ -obj-$(CONFIG_PM) += cpu.o +obj-$(CONFIG_PM_SLEEP) += cpu.o obj-$(CONFIG_HIBERNATION) += swsusp.o suspend.o --
Your patch description doesn't mention any linker warning.
Can you send the linker warning so that we can see the problem and not
Technically you are the one who has to deal with problems in your
That the code is architecture specific is clear.
But how to best annotate suspend and CPU hotplug code is a problem that
is shared between many architectures and whose solution should not be
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
--If you believe that my patch adds a new problem then please describe it clearly so that I can understand it. -Andi --
Description:
- there are already __cpuinit* annotations in the kernel
- on UP kernels supporting suspend/resume, such annotated code
currently gets freed after booting (and this works)
- with your patch applied, this code no longer gets freed
Whether or not this is a problem depends on whether you care about the
memory used by the kernel or not...
My other issues with your patch were:
- whatever warning this patch was supposed to fixed was not shown
(the patch description didn't mention any warning at all) - and
understanding a fix gets easier when you know the problem it's
supposed to fix
- this patch shouldn't be x86 specific since the issue how to
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
--ok, i've dropped this patch from x86.git for now: Subject: x86: force __cpuinit on for CONFIG_PM without HOTPLUG_CPU From: Andi Kleen <ak@suse.de> but longer-term, shouldnt these annotations be automated? We'll see a constant stream of them, all around the clock as people regularly get it wrong (because it's not intuitive). Ingo --
Definitely. Even more when you looking at what Maciej Rozycki suggested regarding discardable strings. [1] cu Adrian [1] http://lkml.org/lkml/2007/10/12/297 -- "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 --
Would be great to have them automated - just dunno how to do it. Do you see a feasible way to do it? Short term modpost etc. will be enhanced to be less dependent on the actual configuration when performing the checks. It should not matter if CONFIG_HOTPLUG_CPU is enabled or not when we check the __cpuint annotations but this is how we see it today so far too many faults slip through. Sam --
a good starting point would be to make the warnings a lot more self-explanatory. Right now it's often non-obvious trying to figure out how the dependencies are structured and which one should be changed to a good way i see is to: - improve the debug output so that it's obvious at a glance what the problem is. - quickly reach a close-to-100%-perfect stage, brute-force. Drop __init* annotations en masse if they are not perfectly layered. Whoever reintroduces them will then have to do it perfectly. - then turn these into hard failures (which they _are_ in a significant percentage of the situations). once it's a hard build failure, people will fix them quickly and automated tests will pick them up as well. Ingo --
First of all, this would really hurt space limited systems.
And "whoever reintroduces them" will most likely mean in practice that
some janitors will go through the code and you as a maintainer will
anyway be busy with getting them in shape.
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
--also, in theory we've got a pretty reliable set of the following information: function X references symbol Y and we know what type of sections they are in, right? could -ffunction-sections be used to delay the categorization of functions to the link stage, and in essence remove the need to mark functions via any of the __init markers? Ingo --
find below the current set of warnings on -git. There are 62.
for example, instead of the rather cryptic:
WARNING: vmlinux.o(.text+0x1815e): Section mismatch: reference to
.init.text:calibrate_delay (between 'smp_callin' and '__cpu_up')
the following output would be more informative:
--------------------------------->
| function:
|
| ./init/calibrate.c:void __devinit calibrate_delay(void)
|
| calls:
|
| ./arch/x86/kernel/smpboot_32.c:static void __cpuinit smp_callin(void)
|
| but calibrate_delay() is __devinit while smp_callin() is __cpuinit.
| Change smp_callin() to __devinit to resolve this warning.
|-------------------------------->
would result in a lot faster cycles of fixing this.
do we have all the info to print this?
Ingo
.init.text: calibrate_delay ('smp_callin' <-> '__cpu_up')
.init.text: register_cpu ('arch_register_cpu' <-> 'in_gate_area_no_task')
.init.text: idle_regs ('fork_idle' <-> 'get_task_mm')
.init.text: ('process_zones' <-> 'pageset_cpuup_callback')
.init.text: pcibios_fixup_bus ('pci_scan_child_bus' <-> 'pci_scan_bus_parented')
.init.data: mtrr ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.data: mtrr ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.data: pmi_setpal ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.data: pmi_setpal ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.text: pci_acpi_scan_root ('acpi_pci_root_add' <-> 'acpi_pci_unregister_driver')
.init.text: ('olympic_open' <-> 'olympic_interrupt')
.init.text: setup_teles3 ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_s0box ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_telespci ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_avm_pcipnp ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_elsa ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_diva ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_sedl...The correct figure is 112. You need to do a: make KCFLAGS=-fno-unit-at-a-time Not yet. The patch I posted on lkml bring us one step further. At modpost time today we only see .init.text and .text but with the suggested patch we will be able to see what section a function belong to and we are several steps closer to better error messages. So let me get that into shape and I will revisit the messages. In this case I would not know if calibrate_delay() should be __cpuinit or smp_callin() should be __devinit looking at the information modpost has available without additional digging. But a quick grep tells me that the right fix is to annotate calibrate_delay() with __cpuinit because is is used from either __cpuinit annotated or __init annotated functions. Sam --
btw., please add a .config option to trigger the -fno-unit-at-a-time
flags. Something like CONFIG_SECTION_ERRORS=y - plus perhaps combine it
with the patch below that turns such section bugs into detectable build
errors. A distro does not want to build a kernel that could potentially
corrupt kernel memory. (it's a security risk as well.) If we make the
err=1 dependent on CONFIG_SECTION_ERRORS then we'll have this
configurable.
Ingo
---------------->
Subject: x86: link mismatch error
From: Ingo Molnar <mingo@elte.hu>
turn the build warning into a build error.
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
scripts/mod/modpost.c | 63 ++++++++++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 24 deletions(-)
Index: linux/scripts/mod/modpost.c
===================================================================
--- linux.orig/scripts/mod/modpost.c
+++ linux/scripts/mod/modpost.c
@@ -863,8 +863,8 @@ static void find_symbols_between(struct
* Try to find symbols near it so user can find it.
* Check whitelist before warning - it may be a false positive.
**/
-static void warn_sec_mismatch(const char *modname, const char *fromsec,
- struct elf_info *elf, Elf_Sym *sym, Elf_Rela r)
+static int error_sec_mismatch(const char *modname, const char *fromsec,
+ struct elf_info *elf, Elf_Sym *sym, Elf_Rela r)
{
const char *refsymname = "";
Elf_Sym *before, *after;
@@ -874,6 +874,7 @@ static void warn_sec_mismatch(const char
const char *secstrings = (void *)hdr +
sechdrs[hdr->e_shstrndx].sh_offset;
const char *secname = secstrings + sechdrs[sym->st_shndx].sh_name;
+ int err = 0;
find_symbols_between(elf, r.r_offset, fromsec, &before, &after);
@@ -885,32 +886,38 @@ static void warn_sec_mismatch(const char
if (secref_whitelist(modname, secname, fromsec,
before ? elf->strtab + before->st_name : "",
refsymname))
- return;
+ goto out;...The plan is to let section mismatch warnings become errors after the merge window - so we hit -mm first. And I will add a config option to: - set -fno-unit-at-a-time - add no-inline to all functions marked __init* - and maybe disable __inline if that gives additional errors Slowly getting there. Need to beat modpost in shape to get much less configuration dependent warnings/errors first. Sam --
A lot of those I look at seem to be not really bugs; also my impression is that they sometimes crop up randomly. e.g. you change something completely unrelated and suddenly you get I was told future gcc versions would remove that. Why do you I sometimes do that for debugging "define static noinline" in specific files or similar because it's easier to make sense of oopses when functions not inlined. Not sure it would work as a global option though because if you do it globally then all the inlines in all .hs would be affected and that might lead to immense code bloat. -Andi --
I have fixed a lot of these warnings. And when I look closer at them they are explainable. The warnings are today very dependent on the configuration and the inlining that gcc uses. With default options to gcc my .config produces ~65 warnings but with -fno-unit-a-time I get 112 warnings. Solely due to less inlining done by gcc. So there are two sources for the 'randomization': a) The actual config b) The sometimes agressive inlining a) will be addressed by having separate sections for each __init* type that is at link time combined where it belongs. b) is addressed by a Kernel Hacking option which 1) uses -fno-unit-at-a-time to get less gcc inlining 2) maybe make all __*init function no-inline Are there any better way to tell gcc no to inline so agressively? Sam --
I haven't tested it, but -fno-inline-functions-called-once alone
cu
Adrian
BTW: I hope it's clear for everyone that disabling such optimizations
should only be used for debugging section mismatches, not for
production kernels.
--
"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
--One problem I ran into the past was that older binutils seem to have some exponential behaviour with a lot of named sections You can either sprinkle noinlines or set specific --params to throttle back the inliner. The later is very gcc version specific unfortunately. -Andi --
Consider:
static int __init foo()
{
// ...
}
static int bar()
{
// ...
if (foo())
// ...
}
gcc will often inline foo into bar - and then all code are suddenly
part of .text and no section mismatch.
But you add anohter call to foo() somewhere so gcc decide no longer
This is more the -ffunction-section issue I guess.
What we are dealig with here is ~20 more sections and the kernel has
~100 section today (or more). So not a huge increase.
Sam
--Half of them are possible Oopses, and the other half are about wasted
memory.
The warnings in the kernel that are not really bugs you can count with
Not all errors are always visible, they might depend on CONFIG_
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
--for example, in current -git, could you tell me why this triggers:
WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to
.init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
and how to resolve it? I had a quick look and it was not obvious to me.
Ingo
--I was confused by your error message - it looked all wrong. process_zones is .text but setup_per_cpu_pageset is __init. I expect you had some local changes. So I tried myself and got this warning: WARNING: mm/built-in.o(.text+0x6864): Section mismatch: reference to .init.text: (between 'process_zones' and 'pageset_cpuup_callback') This made much more sense. So I looked closely at process_zones() and the first thing I always do is to check all the local functions. I noticed that we use the function zone_batchsize() which is marked __devinit. A function marked __cpuinit may use other functions marked __cpuinit, data marked __cpuinitdata and .text/.data. But references to __devinit is not ok. I furthermore noticed that zone_batchsize() were used in anohter function marked __meminit. So the simple fix for this warning is to remove the annotation of zone_batchsize. It looks like a real opps candidate to me.. Why modpost did not pick up the zone_batchsize symbol is anohter matter. It is present in the file: $ objdump --syms vmlinux.o | grep zone_batchsize 0000000000016929 l F .init.text 0000000000000053 zone_batchsize Debugging modpost I could see that we had an addend value of 695, but the addr of the symbol is 699. So somehow we point 4 bytes wrong. Strange... Anyway - here follows the patch. Sam [PATCH] mm: fix section mismatch warning in page_alloc.c With CONFIG_HOTPLUG=n and CONFIG_HOTPLUG_CPU=y we saw following warning: WARNING: mm/built-in.o(.text+0x6864): Section mismatch: reference to .init.text: (between 'process_zones' and 'pageset_cpuup_callback') The culprint was zone_batchsize() which were annotated __devinit but used from process_zones() which is annotated __cpuinit. zone_batchsize() are used from another function annotated __meminit so the only valid option is to drop the annotation of zone_batchsize() so we know it is always valid to use it. Signed-off-by: Sam Ravnborg <sam@ravnborg.org> --- diff --git a/mm/page_alloc.c...
Andrew: a v2.6.24 mm fix i think. Acked-by: Ingo Molnar <mingo@elte.hu> 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
--'make allyesconfig' and disable CONFIG_HOTPLUG=y. Ingo --
Works for me without the warning.
Wait, I remember something @#$%#@$!!!:
'make allyesconfig' on x86 became ambiguously.
If you have by chance a 64bit CPU that would explain why I didn't get it.
@Sam:
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
--make ARCH=x86_64 allyesconfig will set CONFIG_64BIT for you - no? Sam --
sorry - i should have mentioned that it's 64-bit. (and i even mis-remembered having seen it on 32-bit as well) Ingo --
This bug seems to also be present on 32bit, but allyesconfig minus
CONFIG_HOTPLUG is not among the configurations where it's present
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
--Assuming you are on an x86 box... make allyesconfig will give you a config for same OS bit-size as you run. make ARCH=i386 allyesconfig gives you a 32 bit config make ARCH=x86_64 allyesconfig gives you a 64 bit config make ARCH=x86 allyesconfig gives you a 32 bit config where you can select 64 bit. It was seen as the best solution when unifying 32 and 64 bit stuff and introducing support for ARCH=x86. No-one has complained until now so most people seems to get it or maybe they are just silent. Sam --
Or I'm the only person doing kernel development on a 32bit x86 machine?
Even oldconfig is busted since the Kconfig choice is not available
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
--Signed-off-by: Andi Kleen <ak@suse.de> --- arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) Index: linux/arch/x86/Kconfig =================================================================== --- linux.orig/arch/x86/Kconfig +++ linux/arch/x86/Kconfig @@ -295,6 +295,7 @@ config X86_RDC321X select X86_REBOOTFIXUPS select GENERIC_GPIO select LEDS_GPIO + depends on X86_32 help This option is needed for RDC R-321x system-on-chip, also known as R-8610-(G). --
not applied - the RDC321X subarch already has that dependency:
config X86_RDC321X
bool "RDC R-321x SoC"
depends on X86_32
Ingo
--Someone complained that the i386 defconfig contains AS as default IO scheduler. Change that to CFQ. Cc: axboe@kernel.dk Signed-off-by: Andi Kleen <ak@suse.de> --- arch/x86/configs/i386_defconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux/arch/x86/configs/i386_defconfig =================================================================== --- linux.orig/arch/x86/configs/i386_defconfig +++ linux/arch/x86/configs/i386_defconfig @@ -99,9 +99,9 @@ CONFIG_IOSCHED_NOOP=y CONFIG_IOSCHED_AS=y CONFIG_IOSCHED_DEADLINE=y CONFIG_IOSCHED_CFQ=y -CONFIG_DEFAULT_AS=y +# CONFIG_DEFAULT_AS is not set # CONFIG_DEFAULT_DEADLINE is not set -# CONFIG_DEFAULT_CFQ is not set +CONFIG_DEFAULT_CFQ=y # CONFIG_DEFAULT_NOOP is not set CONFIG_DEFAULT_IOSCHED="anticipatory" --
thanks, applied. Ingo --
On VMs implemented using JITs that cache translated code changing the lock
prefixes is a quite costly operation that forces the JIT to throw away and
retranslate a lot of code.
Previously a SMP kernel would rewrite the locks once for each CPU which
is quite unnecessary. This patch changes the code to never switch at boot in
the normal case (SMP kernel booting with >1 CPU) or only once for SMP kernel
on UP.
This makes a significant difference in boot up performance on AMD SimNow!
Also I expect it to be a little faster on native systems too because a smp
switch does a lot of text_poke()s which each synchronize the pipeline.
Signed-off-by: Andi Kleen <ak@suse.de>
---
arch/x86/kernel/alternative.c | 16 ++++++++++++++--
include/linux/smp.h | 2 ++
init/main.c | 2 +-
3 files changed, 17 insertions(+), 3 deletions(-)
Index: linux/arch/x86/kernel/alternative.c
===================================================================
--- linux.orig/arch/x86/kernel/alternative.c
+++ linux/arch/x86/kernel/alternative.c
@@ -273,6 +273,7 @@ struct smp_alt_module {
};
static LIST_HEAD(smp_alt_modules);
static DEFINE_SPINLOCK(smp_alt);
+static int smp_mode = 1; /* protected by smp_alt */
void alternatives_smp_module_add(struct module *mod, char *name,
void *locks, void *locks_end,
@@ -354,7 +355,14 @@ void alternatives_smp_switch(int smp)
BUG_ON(!smp && (num_online_cpus() > 1));
spin_lock_irqsave(&smp_alt, flags);
- if (smp) {
+
+ /*
+ * Avoid unnecessary switches because it forces JIT based VMs to
+ * throw away all cached translations, which can be quite costly.
+ */
+ if (smp == smp_mode) {
+ /* nothing */
+ } else if (smp) {
printk(KERN_INFO "SMP alternatives: switching to SMP code\n");
clear_cpu_cap(&boot_cpu_data, X86_FEATURE_UP);
clear_cpu_cap(&cpu_data(0), X86_FEATURE_UP);
@@ -369,6 +377,7 @@ void alternatives_smp_switch(int smp)
alternatives_smp_unlock(mod-&g...Please run your patches through checkpatch.pl. ERROR: use tabs not spaces I'm a bit wary about making max_cpus global. max_cpus is used all over the place as a local variable name. Can we please rename it to setup_max_cpus or something like that? Thanks, tglx --
I saw a lot of these warnings, but disregarded them as obviously silly. I don't Hmm, I didn't see any warnings from this so surely it's not a big issue? -Andi --
Andi, this behaviour is obviously silly.
I know that you do not care about white space and consistent coding
style, but others do.
The kernel process request that _all_ contributors run their patches
through checkpath.pl and fix the problems. The review process is the
same for _all_ contributors and I'm not going to add an extra Andi
It's not a question of what you think. It's a question of what the
code does and what the meaning of the command line parameter is:
/*
* Setup routine for controlling SMP activation
*
* Command-line option of "nosmp" or "maxcpus=0" will disable SMP
* activation entirely (the MPS table probe still happens, though).
*
* Command-line option of "maxcpus=<NUM>", where <NUM> is an integer
* greater than 0, limits the maximum number of CPUs activated in
* SMP mode to <NUM>.
*/
There is no "one off" use in smp_init():
for_each_present_cpu(cpu) {
if (num_online_cpus() >= max_cpus)
break;
It's not about warnings. It's about name spaces and it makes the
purpose of the global variable clear to the reader.
tglx
--That's new. When and by whom was that rule been introduced? And with what rationale? Even checkpatch.pl itself says to only fix the warnings that make sense. This one didn't. And it has so many false positives in my experiences that a lot of its warnings are useless. So even if you require checkpatch.pl compliance it will be always subjective because checkpatch.pl is not always correct. Anyways if you care so much about space<->tabs why don't you just write a filter that converts spaces to tabs for incoming patches? Like it is traditionally done for trailing white spaces? Would be a trivial perl script. I wish you guys would care more about actually broken logic in patches than just checkpatch.pl output. When I occasionally check git-x86 I always tend to find some obviously broken patch that should have been caught during review before merge. -Andi --
This applies to warnings and not to errors and what I pointed out to
Sorry, but all the 65 other contributors to x86.git (and the other
1000 upstream contributors) are following the same rules. Most of them
just use the tools and provide clean patches upfront and none of them
ever complained about a polite request to fix things up which slipped
through.
One real reason why you should be special ? (Aside the reverse logic
of your argument: you are the only human and all others are silly
mindless hackers.)
The only thing which is beyond ridiculous is your absolute refusal to
I just pointed out broken logic in your patch (vs. max_cpus) and your
answer is just sloppy hand waving ignoring my completely valid
point. After I pointed it out to you, you do not even have the modesty
of acknowledging your error. No, a second later you claim that we care
only about coding style and not the patch content itself.
Your findings of broken patches in the x86.git tree just prove that we
are all human beings, but at least the people responsible for the
x86.git tree are able to admit their mistakes and fix them without
arguing. On the other side I'm waiting since month for any response
from you on a review for a pile of obviously broken patches which you
wanted to push into 2.6.24.
Your hyprocritical crusade is annoying the hell out of me, but feel
free to ridicule yourself further.
tglx
--That says " You should be able to justify all violations that remain in your patch." That's the wrong way to see it. See it this way: Is forcing humans to convert spaces to tabs an useful activity? Is rejecting patches for that and requiring repost a useful activity that improves Linux in any way? Will it help Linux to let people spend time to convert spaces to tabs instead of writing patches or testing? I don't think it is. Arguably having tabs is nicer than spaces (I wont' disagree with that). But since it's something a simple script can do it's best to just handle it automatically. Then everybody can forget about that and it won't bother anymore again. ftp://ftp.firstfloor.org/pub/ak/perl/converttabs Anyways I could run converttabs over my pile and repost if you want, but as pointed out elsewhere I think it would be better to just integrate it into the merge flow -- then people could just forget about this forever. I question newly introduced rules that don't make sense to me. e.g the absolute Sure that's expected and I missed issues too when I was reviewing x86 patches (the sentence came out perhaps a more harsh than originally intend) But my impression from reading the reviews was that a lot of the rejections were based only on checkpatch.pl and not on logic checks and there were certainly a few patches that clearly weren't logic reviewed. If you do logic checks then that's fine of course -- feel free to prove me wrong on that front in the future. To be honest I was also a little frustrated for patches that I consider important cleanups (like the early-reserve code) I just get some checkpatch.pl I would prefer calling it "trying to improve inefficient newly introduced procedures I'm sorry that you don't see my points. I guess I'll need to do a better job at explaining them, but probably not tonight. -Andi --
i dont think you did. A good number of bona fide style problems were pointed out in your patch and you werent even aware of them even after the fact was pointed out to you. In fact we are at silly mail #10 already that argues about this. I said in the very first mail that your patch looks good to me in principle, just please clean it up a bit. I also applied a lot of your other, clean patches. I dont know why you make such a big fuss about it, have you never been requested to clean up your code before? I too make cleanliness errors quite often and get asked to fix them up. That's what the half dozen style problems i listed in your patch are very much not just about spaces to tabs, and the answer is a resounding: YES, fixing them helps Linux in general. Why? Because the most important asset an outstanding programmer can have (besides the ability to abstract) is a good eye for small details. It's _very_ easy for humans to slip over small details. Like the sign of a variable. Or parentheses in the wrong place. Or a missing check for NULL. Or implicit type conversions. Small details that a different system - like a computer - will _never_ get wrong. And having a good eye for details is a VERY non-human ability - just like mathematics. Millions of years of evolution have taught our brain that all that matters is statistics and fuzzy rules - momentary details rarely matter in the physical world. Being able to pick an apple 80% of the time is plenty good as a survival tactic in the jungle. On the other hand, being correct 99.9% of the time in a computer program makes it virtually unusable most of the time. Getting code alignment wrong, introducing various visible whitespace problems, getting convention wrong and thus making it harder for kernel developers to read each other's code shows a lack for attention to details - be that temporary or permanent inability. And lack of attention to details - even if it's only for a seemingly unimportant thing lik...
Do you think they did refer to code style (as in indentation etc.) here? When I was talking to such people my impression was that their complaints usually came from other issues. e.g. I know from talking to a few people that they consider the BSD kernels cleaner just because they have a very consistent printk style for driver probing so it looks cleaner at booting. Sounds silly (although I can sympathize a little bit) but true. That's the problem I have with your position. You equate quality with coding style or checkpatch.pl output. I think it's only a small part of it. If we follow your description above to the logical conclusion then we could get the perfect kernel(tm) very quickly just by running everything through Lindent or recruit a few hundred people who run all files through checkpatch.pl and fix all warnings and then we end up with the perfect kernel with "improved code quality" Great idea! And is it ok for me to refer to that plan as "the Ingo Molnar plan" (TIMP) in the future? I'm sure a short term for it will come handy in future discussions ;-) -Andi --
Hi Andi, Not be new to anyone trying to sneak a patch past Andrew for the past few months... --
I got several in and I don't remember anybody complaining about this before Ingo/Thomas. -Andi --
Previously the complete files were #ifdef'ed, but now handle that in the Makefile. May save a minor bit of compilation time. Cc: hpa@rpath.com Signed-off-by: Andi Kleen <ak@suse.de> --- arch/x86/Kconfig | 5 +++++ arch/x86/boot/Makefile | 6 ++++-- arch/x86/boot/apm.c | 3 --- arch/x86/boot/voyager.c | 4 ---- 4 files changed, 9 insertions(+), 9 deletions(-) Index: linux/arch/x86/Kconfig =================================================================== --- linux.orig/arch/x86/Kconfig +++ linux/arch/x86/Kconfig @@ -1219,6 +1219,11 @@ source "kernel/power/Kconfig" source "drivers/acpi/Kconfig" +config X86_APM_BOOT + bool + default y + depends on APM + menuconfig APM tristate "APM (Advanced Power Management) BIOS support" depends on X86_32 && PM_SLEEP && !X86_VISWS Index: linux/arch/x86/boot/Makefile =================================================================== --- linux.orig/arch/x86/boot/Makefile +++ linux/arch/x86/boot/Makefile @@ -28,9 +28,11 @@ SVGA_MODE := -DSVGA_MODE=NORMAL_VGA targets := vmlinux.bin setup.bin setup.elf zImage bzImage subdir- := compressed -setup-y += a20.o apm.o cmdline.o copy.o cpu.o cpucheck.o edd.o +setup-y += a20.o cmdline.o copy.o cpu.o cpucheck.o edd.o setup-y += header.o main.o mca.o memory.o pm.o pmjump.o -setup-y += printf.o string.o tty.o video.o version.o voyager.o +setup-y += printf.o string.o tty.o video.o version.o +setup-$(CONFIG_X86_APM_BOOT) += apm.o +setup-$(CONFIG_X86_VOYAGER) += voyager.o # The link order of the video-*.o modules can matter. In particular, # video-vga.o *must* be listed first, followed by video-vesa.o. Index: linux/arch/x86/boot/apm.c =================================================================== --- linux.orig/arch/x86/boot/apm.c +++ linux/arch/x86/boot/apm.c @@ -19,8 +19,6 @@ #include "boot.h" -#if defined(CONFIG_APM) || defined(CONFIG_APM_MODULE) - int query_apm_bios(void) { u16 ax, bx, cx...