Skip to contentSkip to conversation

SCHED: Trivial scheduler fixes

PATCH In review Andi Kleen 3 participants 8 messages Nov 12, 2008

lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

cover SCHED: Trivial scheduler fixes

This patch series fixes a couple of minor issues I noticed while reading the scheduler code.

Patches against 2.6.28-rc4

-Andi

author
#1 lore raw

PATCH 1/4 SCHED: cache task_hot result

Minor scheduler optimization: cache the result of task_hot() in can migrate task instead of computing it three times.

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
kernel/sched.c +7 -4
Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================
@@ -2907,6 +2907,8 @@
 		     struct sched_domain *sd, enum cpu_idle_type idle,
 		     int *all_pinned)
 {
+	int hot;
+
 	/*
 	 * We do not migrate tasks that are:
 	 * 1) running (obviously), or

@@ -2930,10 +2932,11 @@
 	 * 2) too many balance attempts have failed.
 	 */
 
-	if (!task_hot(p, rq->clock, sd) ||
-			sd->nr_balance_failed > sd->cache_nice_tries) {
+	hot = task_hot(p, rq->clock, sd);
+
+	if (!hot || sd->nr_balance_failed > sd->cache_nice_tries) {
 #ifdef CONFIG_SCHEDSTATS
-		if (task_hot(p, rq->clock, sd)) {
+		if (hot) {
 			schedstat_inc(sd, lb_hot_gained[idle]);
 			schedstat_inc(p, se.nr_forced_migrations);
 		}

@@ -2941,7 +2944,7 @@
 		return 1;
 	}
 
-	if (task_hot(p, rq->clock, sd)) {
+	if (hot) {
 		schedstat_inc(p, se.nr_failed_migrations_hot);
 		return 0;
 	}
author
#2 lore raw

PATCH 2/4 SCHED: Mark all frequently used sysctls __read_mostly

In reply to Andi Kleen

There are more optimizations possible here, but that's for another patch.

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
kernel/sched.c +5 -5
Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================
@@ -686,7 +686,7 @@
 #define SCHED_FEAT(name, enabled)	\
 	(1UL << __SCHED_FEAT_##name) * enabled |
 
-const_debug unsigned int sysctl_sched_features =
+const_debug unsigned int sysctl_sched_features __read_mostly =
 #include "sched_features.h"
 	0;
 

@@ -809,26 +809,26 @@
  * Number of tasks to iterate in a single balance run.
  * Limited because this is done with IRQs disabled.
  */
-const_debug unsigned int sysctl_sched_nr_migrate = 32;
+const_debug unsigned int sysctl_sched_nr_migrate __read_mostly = 32;
 
 /*
  * ratelimit for updating the group shares.
  * default: 0.25ms
  */
-unsigned int sysctl_sched_shares_ratelimit = 250000;
+unsigned int sysctl_sched_shares_ratelimit __read_mostly = 250000;
 
 /*
  * Inject some fuzzyness into changing the per-cpu group shares
  * this avoids remote rq-locks at the expense of fairness.
  * default: 4
  */
-unsigned int sysctl_sched_shares_thresh = 4;
+unsigned int sysctl_sched_shares_thresh __read_mostly = 4;
 
 /*
  * period over which we measure -rt task cpu usage in us.
  * default: 1s
  */
-unsigned int sysctl_sched_rt_period = 1000000;
+unsigned int sysctl_sched_rt_period __read_mostly = 1000000;
 
 static __read_mostly int scheduler_running;
author
#3 lore raw

PATCH 3/4 SCHED: don't inline idle_balance into schedule()

In reply to Andi Kleen

This function is not called under load, so it makes sense to out of line it to get better register allocation in schedule() and make the assembler easier to read.

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched.c +1 -1
Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================
@@ -3696,7 +3696,7 @@
  * idle_balance is called by schedule() if this_cpu is about to become
  * idle. Attempts to pull tasks from other CPUs.
  */
-static void idle_balance(int this_cpu, struct rq *this_rq)
+static void noinline idle_balance(int this_cpu, struct rq *this_rq)
 {
 	struct sched_domain *sd;
 	int pulled_task = -1;
author
#4 lore raw

PATCH 4/4 SCHED: Use a lookup array in sd_level_to_string

In reply to Andi Kleen

Generates better/shorter code. Doesn't matter much because it's only a SCHED_DEBUG function.

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
kernel/sched.c +11 -18
Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================
@@ -6629,24 +6629,17 @@
 
 static inline const char *sd_level_to_string(enum sched_domain_level lvl)
 {
-	switch (lvl) {
-	case SD_LV_NONE:
-			return "NONE";
-	case SD_LV_SIBLING:
-			return "SIBLING";
-	case SD_LV_MC:
-			return "MC";
-	case SD_LV_CPU:
-			return "CPU";
-	case SD_LV_NODE:
-			return "NODE";
-	case SD_LV_ALLNODES:
-			return "ALLNODES";
-	case SD_LV_MAX:
-			return "MAX";
-
-	}
-	return "MAX";
+	static const char *lva[] = {
+#define T(x) [SD_LV_ ## x ] = #T
+		T(NONE),
+		T(SIBLING),
+		T(MC),
+		T(CPU),
+		T(NODE),
+		T(ALLNODES),
+#undef T
+	};
+	return (unsigned)lvl < ARRAY_SIZE(lva) ? lva[lvl] : "MAX";
 }
 
 static int sched_domain_debug_one(struct sched_domain *sd, int cpu, int level,
#5 lore raw

Re: [PATCH] [4/4] SCHED: Use a lookup array in sd_level_to_string

Quoted 28 lines · Andi Kleen

Generates better/shorter code. Doesn't matter much because it's only a SCHED_DEBUG function.

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================

kernel/sched.c +0 -10
@@ -6629,24 +6629,17 @@
 
 static inline const char *sd_level_to_string(enum sched_domain_level lvl)
 {
-	switch (lvl) {
-	case SD_LV_NONE:
-			return "NONE";
-	case SD_LV_SIBLING:
-			return "SIBLING";
-	case SD_LV_MC:
-			return "MC";
-	case SD_LV_CPU:
-			return "CPU";
-	case SD_LV_NODE:

kernel/sched.c line 6633

Quoted 10 lines
kernel/sched.c +2 -8
-			return "NODE";
-	case SD_LV_ALLNODES:
-			return "ALLNODES";
-	case SD_LV_MAX:
-			return "MAX";
-
-	}
-	return "MAX";
+	static const char *lva[] = {
+#define T(x) [SD_LV_ ## x ] = #T

Shouldn't ^ that be x?

Hannes

author
#6 lore raw

Re: [PATCH] [4/4] SCHED: Use a lookup array in sd_level_to_string

Quoted 3 lines · Johannes Weiner
Quoted 1 line

+#define T(x) [SD_LV_ ## x ] = #T

Shouldn't ^ that be x?

Yes good point. Thanks. Revised patch appended.

-Andi

---

SCHED: Use a lookup array in sd_level_to_string v2

Generates better/shorter code. Doesn't matter much because it's only a SCHED_DEBUG function.

v2: Fix stupid typo thanks to Johannes Weiner

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
kernel/sched.c +11 -18
Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================
@@ -6629,24 +6629,17 @@
 
 static inline const char *sd_level_to_string(enum sched_domain_level lvl)
 {
-	switch (lvl) {
-	case SD_LV_NONE:
-			return "NONE";
-	case SD_LV_SIBLING:
-			return "SIBLING";
-	case SD_LV_MC:
-			return "MC";
-	case SD_LV_CPU:
-			return "CPU";
-	case SD_LV_NODE:
-			return "NODE";
-	case SD_LV_ALLNODES:
-			return "ALLNODES";
-	case SD_LV_MAX:
-			return "MAX";
-
-	}
-	return "MAX";
+	static const char *lva[] = {
+#define T(x) [SD_LV_ ## x ] = #x
+		T(NONE),
+		T(SIBLING),
+		T(MC),
+		T(CPU),
+		T(NODE),
+		T(ALLNODES),
+#undef T
+	};
+	return (unsigned)lvl < ARRAY_SIZE(lva) ? lva[lvl] : "MAX";
 }
 
 static int sched_domain_debug_one(struct sched_domain *sd, int cpu, int level,
#7 lore raw

Re: [PATCH] [4/4] SCHED: Use a lookup array in sd_level_to_string

In reply to Johannes Weiner
Quoted 33 lines · Johannes Weiner
Quoted 32 lines · Andi Kleen

Generates better/shorter code. Doesn't matter much because it's only a SCHED_DEBUG function.

Signed-off-by Andi Kleen <[email protected]>
---
 kernel/sched.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

Index: linux-2.6.28-rc4-test/kernel/sched.c
===================================================================

kernel/sched.c +0 -14
@@ -6629,24 +6629,17 @@
 
 static inline const char *sd_level_to_string(enum sched_domain_level lvl)
 {
-	switch (lvl) {
-	case SD_LV_NONE:
-			return "NONE";
-	case SD_LV_SIBLING:
-			return "SIBLING";
-	case SD_LV_MC:
-			return "MC";
-	case SD_LV_CPU:
-			return "CPU";
-	case SD_LV_NODE:
-			return "NODE";
-	case SD_LV_ALLNODES:
-			return "ALLNODES";
-	case SD_LV_MAX:
Quoted 8 lines

kernel/sched.c line 6633

Quoted 6 lines
kernel/sched.c +2 -4
-			return "MAX";
-
-	}
-	return "MAX";
+	static const char *lva[] = {
+#define T(x) [SD_LV_ ## x ] = #T

Shouldn't ^ that be x?

hasnt been tested i guess. Note that the patch is moot: an equivalent change has been done in the scheduler tree already, in a cleaner way. It's all in linux-next.

Ingo