Re: [PATCH] sched: fix a bug in sched domain degenerate

[Ingo Molnar <mingo@elte.hu>]

* Li Zefan <lizf@cn.fujitsu.com> wrote:

> Hi Ingo,
> 
> I just read the modified changelog in the git-log, and it is
> wrong (or maybe my fix is wrong?), I should have explained
> the bug clearer. :(
> 
> I'm writing this mail to confirm if my thought and fix is
> right or not.
> 
> > commit f29c9b1ccb52904ee442a933cf3dee628f9f4e62
> > Author: Li Zefan <lizf@cn.fujitsu.com>
> > Date:   Thu Nov 6 09:45:16 2008 +0800
> > 
> >     sched: fix a bug in sched domain degenerate
> >     
> >     Impact: re-add incorrectly eliminated sched domain layers
> >     
> 
> This statement is wrong..

that's OK, because the patch is correct :-)

> >     (1) on i386 with SCHED_SMT and SCHED_MC enabled
> >     	# mount -t cgroup -o cpuset xxx /mnt
> >     	# echo 0 > /mnt/cpuset.sched_load_balance
> >     	# mkdir /mnt/0
> >     	# echo 0 > /mnt/0/cpuset.cpus
> >     	# dmesg
> >     	CPU0 attaching sched-domain:
> >     	 domain 0: span 0 level CPU
> >     	  groups: 0
> >     
> 
> I think this behavior is wrong.
> 
> >     (2) on i386 with SCHED_MC enabled but SCHED_SMT disabled
> >     	# same with (1)
> >     	# dmesg
> >     	CPU0 attaching NULL sched-domain.
> >     
> 
> And this is right. CPU domain has only 1 cpu so it does not contribute
> to scheduling, so it can be removed.
> 
> >     The bug is that some sched domains may be skipped unintentionally when
> >     degenerating (optimizing) sched domains.
> >     
> 
> The bug is, some sched domains won't be checked in the for loop due
> to the bug, so they have no chance to be removed.
> 
> In the for loop, we check if the parents domains can be removed:
> 
> cur_ptr
>  |
>  v
> SMT--->MC--->CPU--->NULL
> 
> (parent MC is checked and can be removed)
> 
> =>
> 
>        cur_ptr
>         |
>         v
> SMT--->CPU--->NULL
> 
> (break out of the for loop, because cur_ptr->parent == NULL)
> 
> so CPU domain won't be checked. When we delete MC domain, the pointer
> should not move forwards, so the fix is:
> 
> cur_ptr
>  |
>  v
> SMT--->CPU--->NULL

ah, ok - i misunderstood the direction of the fix. So it strengthens 
degeneration - which is a valid fix too. And the commit message 
remains there to shame my reading skills forever ;-)

	Ingo