Subject: Re: [PATCH sched-devel 0/7] CPU isolation extensions
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
To: Max Krasnyanskiy <maxk@qualcomm.com>
Cc: Ingo Molnar <mingo@elte.hu>, Andrew Morton <akpm@linux-foundation.org>,
       LKML <linux-kernel@vger.kernel.org>, Paul Jackson <pj@sgi.com>
In-Reply-To: <47BE35B7.2070104@qualcomm.com>
References: <47BE35B7.2070104@qualcomm.com>
Content-Type: text/plain
Date: Fri, 22 Feb 2008 12:43:20 +0100
Message-Id: <1203680600.6242.20.camel@lappy>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4335
Lines: 91


On Thu, 2008-02-21 at 18:38 -0800, Max Krasnyanskiy wrote:

> As you suggested I'm sending CPU isolation patches for review/inclusion into 
> sched-devel tree. They are against 2.6.25-rc2.
> You can also pull them from my GIT tree at
> 	git://git.kernel.org/pub/scm/linux/kernel/git/maxk/cpuisol-2.6.git master
 
Post patches! I can't review a git tree..
 
> Diffstat:
>  b/Documentation/ABI/testing/sysfs-devices-system-cpu |   41 ++++++
>  b/Documentation/cpu-isolation.txt                    |  114 ++++++++++++++++++-
>  b/arch/x86/Kconfig                                   |    1 
>  b/arch/x86/kernel/genapic_flat_64.c                  |    5 
>  b/drivers/base/cpu.c                                 |   48 ++++++++
>  b/include/linux/cpumask.h                            |    3 
>  b/kernel/Kconfig.cpuisol                             |   15 ++
>  b/kernel/Makefile                                    |    4 
>  b/kernel/cpu.c                                       |   49 ++++++++
>  b/kernel/sched.c                                     |   37 ------
>  b/kernel/stop_machine.c                              |    9 +
>  b/kernel/workqueue.c                                 |   31 +++--
>  kernel/Kconfig.cpuisol                               |   56 ++++++---
>  kernel/cpu.c                                         |   16 +-
>  14 files changed, 356 insertions(+), 73 deletions(-)
> 
> List of commits
>    cpuisol: Make cpu isolation configrable and export isolated map
 
cpu_isolated_map was a bad hack when it was introduced, I feel we should
deprecate it and fully integrate the functionality into cpusets. That would
give a much more flexible end-result.

CPU-sets can already isolate cpus by either creating a cpu outside of any set,
or a set with a single cpu not shared by any other sets.

This also allows for isolated groups, there are good reasons to isolate groups,
esp. now that we have a stronger RT balancer. SMP and hard RT are not
exclusive. A design that does not take that into account is too rigid.

>    cpuisol: Do not route IRQs to the CPUs isolated at boot

>From the diffstat you're not touching the genirq stuff, but instead hack a
single architecture to support this feature. Sounds like an ill designed hack.

A better approach would be to add a flag to the cpuset infrastructure that says
whether its a system set or not. A system set would be one that services the
general purpose OS and would include things like the IRQ affinity and unbound
kernel threads (including unbound workqueues - or single workqueues). This flag
would default to on, and by switching it off for the root set, and a select
subset you would push the System away from those cpus, thereby isolating them.

>    cpuisol: Do not schedule workqueues on the isolated CPUs
 
(per-cpu workqueues, the single ones are treated in the previous section)

I still strongly disagree with this approach. Workqueues are passive, they
don't do anything unless work is provided to them. By blindly not starting them
you handicap the system and services that rely on them.

(you even acknowledged this problem, by saying it breaks oprofile for instance
- still trying to push a change that knowingly breaks a lot of stuff is bad
manners on lkml and not acceptable for mainline)

The way to do this is to avoid the generation of work, not the execution of it.

>    cpuisol: Move on-stack array used for boot cmd parsing into __initdata
>    cpuisol: Documentation updates
>    cpuisol: Minor updates to the Kconfig options
 
No idea about these patches,... 

>    cpuisol: Do not halt isolated CPUs with Stop Machine

Very strong NACK on this one, it breaks a lot of functionality in non-obvious
ways, as has been pointed out to you numerous times. Such patches are just not
acceptable for mainline - full stop.


Please, the ideas are not bad and have been suggested in the context of -rt a
number of times, its just the execution. Design features so that they are
flexible and can be used in ways you never imagined them. But more
importantly so that they don't knowingly break tons of stuff.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/