Re: current linux-2.6.git: cpusets completely broken

[Max Krasnyansky <maxk@qualcomm.com>]

Ingo Molnar wrote:
> * Max Krasnyansky <maxk@qualcomm.com> wrote:
> 
>> Ingo Molnar wrote:
>>> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>
>>>> On Mon, 14 Jul 2008, Max Krasnyansky wrote:
>>>>> Did you guys an updated patch ? Dmitry pointed out several things that Linus
>>>>> missed in his original version. I guess I can go through the thread and
>>>>> reconstruct that but if you have a patch I can try let me know.
>>>> I didn't update it, and right now I'm just merging too much (and  
>>>> discussing the merges) to have time.
>>>>
>>>> The patch really needs to have some scheduler person look at the use  
>>>> fo cpu_active_map - I was kind of hoping that Ingo would.
>>> yeah - it's very high on our TODO list :-) Peter, Dmitry and me are  
>>> looking into it.
>>>
>>> I didnt touch most of -tip in the past few days to get a rock solid QA  
>>> track record for all items we have.
>> I just sent you guys a patch. Please take a look. I've probably missed 
>> something but it should close (I think). Also we'd probably at least 
>> want the bits that streamline the domain reinitialization because it 
>> helps with cpusets (ie uses same exact path for all cases).
> 
> thanks Max. Since upstream already has Dmitry's it conflicted with your 
> patch - i fixed the interactions up, see it below. (completely untested)
Hmm, I did it on top of 2.6.26 final which has Dmitry's patch. It must have
been something in the -tip.
Anyway, I'll apply it here and retest.

> It's not ready for inclusion yet though - these new
> #ifdefs are quite ugly:
> 
>  +#if !defined(CONFIG_CPUSETS)
>  +       partition_sched_domains(0, NULL, NULL);
>  +#else
>  +       rebuild_sched_domains();
>  +#endif
> 
> we should just have a single method for refreshing sched domains 
> hierarchy, and in the !CONFIG_CPUSETS case that should simply fall back 
> to partition_sched_domains().
> 
> We can do that by making rebuild_sched_domains() the primary method that 
> is called - and in the !CPUSETS case it's an inline that calls 
> partition_sched_domains().

Actually that's exactly what I have in the cpuset part of the patch. But as
I mentioned there is some circular locking issues with rebuild_sched_domains().
(cgroup_lock and get_online_cpus()) and I wanted to fix that fix. Now that I
think about it's kind of unrelated. So in other words I totally agree. I'll
go ahead fix it and resend.

btw While we're at it. Does arch_init_sched_domains() and arch_reinit_sched_domains()
still make sense. I'm talking about naming here. I suppose arch_ part means that it
can be replaced by the arch code. But it's not the case with those two guys.
What do you think ?

> also, small nits:
> 
> use #ifdef CONFIG_CPUSETS instead of "#if !defined(CONFIG_CPUSETS)".
Will do.

> and while at it:
> 
>  +#if !defined(CONFIG_CPUSETS)
>          /* XXX: Theoretical race here - CPU may be hotplugged now */
>          hotcpu_notifier(update_sched_domains, 0);
>  +#endif
> 
> that race should be closed now, hm?
Yeah, I stared at that comment for a second and decided to leave it.
I guess it's still there. I mean in theory cpu can still be hotplugged just
before sched or cpuset register their notifier callbacks.

Max