Re: [PATCH sched-devel 0/7] CPU isolation extensions

[Mark Hounschell <markh@compro.net>]

Peter Zijlstra wrote:
> 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..
>  

Max, could you also post them for 2.6.24.2 stable please. Thanks


>> 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.
> 

Peter, what about when I am NOT using cpusets and are disabled in my config but
I still want to use this?

> 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.
> 

Have things changed since since my first bad encounter with Workqueues.
I am referring to this thread. 

http://kerneltrap.org/mailarchive/linux-kernel/2007/5/29/97039 

> (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.
> 
> 

Mark