On Wed, 06 Feb 2008 21:32:55 -0800 Max Krasnyansky <maxk@qualcomm.com> wrote:
The feature as a whole seems useful, and I don't actually oppose the merge
based on what I see here. As long as you're really sure that cpusets are
inappropriate (and bear in mind that Paul has a track record of being wrong
on this :)). But I see a few glitches
- There are two separate and identical implementations of
cpu_unusable(cpu). Please do it once, in a header, preferably with C
function, not macros.
- The Kconfig help is a bit scraggly:
+config CPUISOL_STOPMACHINE
+ bool "Do not halt isolated CPUs with Stop Machine (HIGHLY EXPERIMENTAL)"
+ depends on CPUISOL && STOP_MACHINE && EXPERIMENTAL
+ help
+ If this option is enabled kernel will not halt isolated CPUs when Stop Machine
"the kernel"
text is too wide
+ is triggered.
+ Stop Machine is currently only used by the module insertion and removal logic.
+ Please note that at this point this feature is highly experimental and maybe
+ dangerous. It is not known to really brake anything but can potentially
+ introduce an instability.
s/maybe/may be/
s/brake/break/
Neither this text, nor the changelog nor the code comments tell us what the
potential instability with stopmachine *is*? Or maybe I missed it.
- Adding new sysfs files without updating Documentation/ABI/ makes Greg
cry.
- Why is cpu_isolated_map exported to modules? Just for api consistency,
it appears?
pre-existing problems:
- isolated_cpu_setup() has an on-stack array of NR_CPUS integers. This
will consume 4k of stack on ia64 (at least). We'll just squeak through
for a ittle while, but this needs to be fixed. Just move it into
__initdata.
- isolated_cpu_setup() expects that the user can provide an up-to-1024
character kernel boot parameter. Is this reasonable given cpu command
line limits, and given that NR_CPUS will surely grow beyond 1024 in the
future?
--
