[patch] sched: fix sched domain degenerate

[patch] sched: fix sched domain degenerate

[Siddha, Suresh B]

Appended patch makes sched domain degenerate really work.

For example, now NUMA domain really gets removed on a non-NUMA system.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>


--- linux-2.6.12-rc2-mm3/kernel/sched.c	2005-04-13 11:15:00.942609504 -0700
+++ linux-mc/kernel/sched.c	2005-04-13 10:44:37.400829992 -0700
@@ -4777,7 +4777,7 @@ static int __devinit sd_parent_degenerat
 	/* WAKE_BALANCE is a subset of WAKE_AFFINE */
 	if (cflags & SD_WAKE_AFFINE)
 		pflags &= ~SD_WAKE_BALANCE;
-	if ((~sd->flags) & parent->flags)
+	if (~cflags & pflags)
 		return 0;
 
 	return 1;
--- linux-2.6.12-rc2-mm3/include/linux/topology.h	2005-04-11 23:08:30.843108376 -0700
+++ linux-mc/include/linux/topology.h	2005-04-13 11:14:44.591095312 -0700
@@ -97,6 +97,7 @@
 	.flags			= SD_LOAD_BALANCE	\
 				| SD_BALANCE_NEWIDLE	\
 				| SD_BALANCE_EXEC	\
+				| SD_BALANCE_FORK	\
 				| SD_WAKE_AFFINE	\
 				| SD_WAKE_IDLE		\
 				| SD_SHARE_CPUPOWER,	\
@@ -128,6 +129,7 @@
 	.flags			= SD_LOAD_BALANCE	\
 				| SD_BALANCE_NEWIDLE	\
 				| SD_BALANCE_EXEC	\
+				| SD_BALANCE_FORK	\
 				| SD_WAKE_AFFINE,	\
 	.last_balance		= jiffies,		\
 	.balance_interval	= 1,			\

Re: [patch] sched: fix sched domain degenerate

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

Siddha, Suresh B wrote:
> Appended patch makes sched domain degenerate really work.
> 
> For example, now NUMA domain really gets removed on a non-NUMA system.
> 
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> 
> 
> --- linux-2.6.12-rc2-mm3/kernel/sched.c	2005-04-13 11:15:00.942609504 -0700
> +++ linux-mc/kernel/sched.c	2005-04-13 10:44:37.400829992 -0700
> @@ -4777,7 +4777,7 @@ static int __devinit sd_parent_degenerat
>  	/* WAKE_BALANCE is a subset of WAKE_AFFINE */
>  	if (cflags & SD_WAKE_AFFINE)
>  		pflags &= ~SD_WAKE_BALANCE;
> -	if ((~sd->flags) & parent->flags)
> +	if (~cflags & pflags)
>  		return 0;
>  
>  	return 1;

Thanks, I need a brown paper bag.

I think instead of the other 2 hunks in your patch I would like
to do it the following way (I hope I get this right, finally).

-- 
SUSE Labs, Novell Inc.

[-- Attachment #2: sched-degen-fix.patch --]
[-- Type: text/plain, Size: 927 bytes --]

Make sched-remove-degenerate-domains.patch actually work.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>


Catch more (hopefully all) cases.

Signed-off-by: Nick Piggin <nickpiggin@yahoo.com.au>

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c	2005-04-15 22:52:25.000000000 +1000
+++ linux-2.6/kernel/sched.c	2005-04-15 22:58:54.000000000 +1000
@@ -4844,7 +4844,14 @@ static int __devinit sd_parent_degenerat
 	/* WAKE_BALANCE is a subset of WAKE_AFFINE */
 	if (cflags & SD_WAKE_AFFINE)
 		pflags &= ~SD_WAKE_BALANCE;
-	if ((~sd->flags) & parent->flags)
+	/* Flags needing groups don't count if only 1 group in parent */
+	if (parent->groups == parent->groups->next) {
+		pflags &= ~(SD_LOAD_BALANCE |
+				SD_BALANCE_NEWIDLE |
+				SD_BALANCE_FORK |
+				SD_BALANCE_EXEC);
+	}
+	if (~cflags & pflags)
 		return 0;
 
 	return 1;

Re: [patch] sched: fix sched domain degenerate

[Siddha, Suresh B]

On Fri, Apr 15, 2005 at 11:03:20PM +1000, Nick Piggin wrote:
> Index: linux-2.6/kernel/sched.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched.c	2005-04-15 22:52:25.000000000 +1000
> +++ linux-2.6/kernel/sched.c	2005-04-15 22:58:54.000000000 +1000
> @@ -4844,7 +4844,14 @@ static int __devinit sd_parent_degenerat
>  	/* WAKE_BALANCE is a subset of WAKE_AFFINE */
>  	if (cflags & SD_WAKE_AFFINE)
>  		pflags &= ~SD_WAKE_BALANCE;
> -	if ((~sd->flags) & parent->flags)
> +	/* Flags needing groups don't count if only 1 group in parent */
> +	if (parent->groups == parent->groups->next) {
> +		pflags &= ~(SD_LOAD_BALANCE |
> +				SD_BALANCE_NEWIDLE |
> +				SD_BALANCE_FORK |
> +				SD_BALANCE_EXEC);
> +	}

This patch works fine and I like this fix. But should n't we be adding 
SD_WAKE_AFFINE and SD_WAKE_BALANCE to this list?

And about SD_BALANCE_FORK, now that we have multi level sbe/sbf, we should 
add this flag to SD_CPU/SIBLING_INIT too..

thanks,
suresh

Re: [patch] sched: fix sched domain degenerate

[Nick Piggin]

Siddha, Suresh B wrote:
> On Fri, Apr 15, 2005 at 11:03:20PM +1000, Nick Piggin wrote:
> 
>>Index: linux-2.6/kernel/sched.c
>>===================================================================
>>--- linux-2.6.orig/kernel/sched.c	2005-04-15 22:52:25.000000000 +1000
>>+++ linux-2.6/kernel/sched.c	2005-04-15 22:58:54.000000000 +1000
>>@@ -4844,7 +4844,14 @@ static int __devinit sd_parent_degenerat
>> 	/* WAKE_BALANCE is a subset of WAKE_AFFINE */
>> 	if (cflags & SD_WAKE_AFFINE)
>> 		pflags &= ~SD_WAKE_BALANCE;
>>-	if ((~sd->flags) & parent->flags)
>>+	/* Flags needing groups don't count if only 1 group in parent */
>>+	if (parent->groups == parent->groups->next) {
>>+		pflags &= ~(SD_LOAD_BALANCE |
>>+				SD_BALANCE_NEWIDLE |
>>+				SD_BALANCE_FORK |
>>+				SD_BALANCE_EXEC);
>>+	}
> 
> 
> This patch works fine and I like this fix. But should n't we be adding 
> SD_WAKE_AFFINE and SD_WAKE_BALANCE to this list?
> 

Hmm, well they don't use groups, but I guess they can be excluded,
because if the parent span is the same as the child span (and that
is true at this point), then SD_WAKE_AFFFINE/BALANCE in the parent
will never be executed. Good point.

wake_idle should be doing a similar thing too, but that needs a bit
of work.

> And about SD_BALANCE_FORK, now that we have multi level sbe/sbf, we should 
> add this flag to SD_CPU/SIBLING_INIT too..
> 

I guess we should think about it. It depends - does SD_BALANCE_FORK
make sense on a plain SMP machine? If so, then it probably makes
sense to be in the 'SMP' domain on a NUMA system, otherwise not.

I suspect that for BALANCE_FORK, the answer may be no. On an SMP, it
is far less disastrous to misplace tasks and have them picked up by
the periodic rebalancer. What's more, BALANCE_FORK does add a non
trivial overhead when moving tasks to other CPUs.

But it's open for debate. I haven't done comprehensive tests.

-- 
SUSE Labs, Novell Inc.