[RFC PATCH v1 0/2] sched: unified sched_powersavings tunables

[RFC PATCH v1 0/2] sched: unified sched_powersavings tunables

[Vaidyanathan Srinivasan]

Hi,

I have created the following RFC patch based on the recent discussions
and consensus on simplifying the power aware scheduler in the kernel.

Ref: LWN: Rethinking power-aware scheduling
     http://lwn.net/Articles/474915/

The goal of the unified tunable are as follows:

* Simple and single tunable for different (all) topology
* Good default powersavings for kernel
* Potential to map this setting to other subsystems like cpufreq and
  cpuidle    

What this patch does: (first step)

* Create a single sched_powersavings tunable in sysfs
* Enable current sched_mc and sched_smt features based on 
  value of this single tunable

What this patch is yet to do:

* Tune the default power savings to pack packages only till
  a threshold of say 50%
* Add notifiers to change setting on battery/AC transitions
* Feed the thresholds from arch specific code so that different archs
  can do 'optimal' packing based on topology  
* May be move this to /sys/device/system/powersavings and add additional
  platform tunables like x86_energy_perf_policy?

This RFC has only x86 changes and has been tested on dual-socket,
quad-core,HT configuration.

Please let me know your comments and feedback.

Thanks,
Vaidy

---

Vaidyanathan Srinivasan (2):
      sched: unified sched_powersavings sysfs tunable
      sched: fix group_capacity for thread level consolidation


 arch/x86/Kconfig          |   20 ++++--------
 arch/x86/kernel/smpboot.c |    2 +
 block/blk.h               |   11 ++++---
 drivers/base/cpu.c        |    2 +
 include/linux/sched.h     |   29 +++++++++--------
 include/linux/topology.h  |    9 +----
 kernel/sched/core.c       |   75 +++++++++++----------------------------------
 kernel/sched/fair.c       |   38 ++++++++++++++++-------
 8 files changed, 77 insertions(+), 109 deletions(-)

[RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Vaidyanathan Srinivasan]

Combine the sched_mc_powersavings and sched_smt_powersavings sysfs
tunables into a single sysfs tunable:

/sys/devices/system/cpu/sched_powersavings={0,1,2}

		0 - Power savings disabled (performance mode)
		1 - Default kernel settings.  Automatic powersave
		    vs performance tradeoff by the kernel
		2 - Maximum power savings

The kernel will default to '1' which is equivalent to
sched_mc_powersavings=1 or consolidate at package level.

Max power saving setting '2' would consolidate to sibling threads and
also do aggressive active balancing.

Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
---
 arch/x86/Kconfig          |   20 ++++--------
 arch/x86/kernel/smpboot.c |    2 +
 block/blk.h               |   11 ++++---
 drivers/base/cpu.c        |    2 +
 include/linux/sched.h     |   29 +++++++++--------
 include/linux/topology.h  |    9 +----
 kernel/sched/core.c       |   75 +++++++++++----------------------------------
 kernel/sched/fair.c       |   23 +++++++-------
 8 files changed, 62 insertions(+), 109 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 6c14ecd..ee615af 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -788,23 +788,15 @@ config NR_CPUS
 	  This is purely to save memory - each supported CPU adds
 	  approximately eight kilobytes to the kernel image.
 
-config SCHED_SMT
-	bool "SMT (Hyperthreading) scheduler support"
-	depends on X86_HT
-	---help---
-	  SMT scheduler support improves the CPU scheduler's decision making
-	  when dealing with Intel Pentium 4 chips with HyperThreading at a
-	  cost of slightly increased overhead in some places. If unsure say
-	  N here.
-
-config SCHED_MC
+config SCHED_POWERSAVE
 	def_bool y
-	prompt "Multi-core scheduler support"
+	prompt "Power save support in scheduler"
 	depends on X86_HT
 	---help---
-	  Multi-core scheduler support improves the CPU scheduler's decision
-	  making when dealing with multi-core CPU chips at a cost of slightly
-	  increased overhead in some places. If unsure say N here.
+	  Power saving feature in scheduler optimizes task placement
+	  in a multi-core or mulit-threaded system whenever possible.
+	  Default kernel settings would suit most applications, while
+	  sysfs tunables can be used to control this feature at runtime.
 
 config IRQ_TIME_ACCOUNTING
 	bool "Fine granularity task level IRQ time accounting"
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 66d250c..1d60cdd 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -414,7 +414,7 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
 	 * For perf, we return last level cache shared map.
 	 * And for power savings, we return cpu_core_map
 	 */
-	if ((sched_mc_power_savings || sched_smt_power_savings) &&
+	if ((sched_power_savings) &&
 	    !(cpu_has(c, X86_FEATURE_AMD_DCM)))
 		return cpu_core_mask(cpu);
 	else
diff --git a/block/blk.h b/block/blk.h
index 7efd772..1457107 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -167,14 +167,15 @@ static inline int queue_congestion_off_threshold(struct request_queue *q)
 static inline int blk_cpu_to_group(int cpu)
 {
 	int group = NR_CPUS;
-#ifdef CONFIG_SCHED_MC
-	const struct cpumask *mask = cpu_coregroup_mask(cpu);
-	group = cpumask_first(mask);
-#elif defined(CONFIG_SCHED_SMT)
-	group = cpumask_first(topology_thread_cpumask(cpu));
+#ifdef CONFIG_SCHED_POWERSAVE
+	if (smt_capable())
+		group = cpumask_first(topology_thread_cpumask(cpu));
+	else	
+		group = cpumask_first(cpu_coregroup_mask(cpu));
 #else
 	return cpu;
 #endif
+	/* Possible dead code?? */
 	if (likely(group < NR_CPUS))
 		return group;
 	return cpu;
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index db87e78..dbaa35f 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -299,7 +299,7 @@ void __init cpu_dev_init(void)
 
 	cpu_dev_register_generic();
 
-#if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
+#ifdef CONFIG_SCHED_POWERSAVE
 	sched_create_sysfs_power_savings_entries(cpu_subsys.dev_root);
 #endif
 }
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 4032ec1..5c33bbc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -850,33 +850,34 @@ enum cpu_idle_type {
 #define SD_PREFER_SIBLING	0x1000	/* Prefer to place tasks in a sibling domain */
 #define SD_OVERLAP		0x2000	/* sched_domains of this level overlap */
 
-enum powersavings_balance_level {
-	POWERSAVINGS_BALANCE_NONE = 0,  /* No power saving load balance */
-	POWERSAVINGS_BALANCE_BASIC,	/* Fill one thread/core/package
-					 * first for long running threads
-					 */
-	POWERSAVINGS_BALANCE_WAKEUP,	/* Also bias task wakeups to semi-idle
-					 * cpu package for power savings
-					 */
-	MAX_POWERSAVINGS_BALANCE_LEVELS
+enum powersavings_level {
+	POWERSAVINGS_DISABLED = 0,	/* Max performance */
+	POWERSAVINGS_DEFAULT,		/* Kernel default policy, automatic powersave */
+					/* vs performance tradeoff */
+	POWERSAVINGS_MAX		/* Favour power savings over peformance */
 };
 
-extern int sched_mc_power_savings, sched_smt_power_savings;
+extern int sched_power_savings;
 
 static inline int sd_balance_for_mc_power(void)
 {
-	if (sched_smt_power_savings)
+	switch (sched_power_savings) {
+	case POWERSAVINGS_MAX:
 		return SD_POWERSAVINGS_BALANCE;
 
-	if (!sched_mc_power_savings)
+	case POWERSAVINGS_DISABLED:
 		return SD_PREFER_SIBLING;
 
+	default:
+		break;
+	}
+
 	return 0;
 }
 
 static inline int sd_balance_for_package_power(void)
 {
-	if (sched_mc_power_savings | sched_smt_power_savings)
+	if (sched_power_savings != POWERSAVINGS_DISABLED)
 		return SD_POWERSAVINGS_BALANCE;
 
 	return SD_PREFER_SIBLING;
@@ -892,7 +893,7 @@ extern int __weak arch_sd_sibiling_asym_packing(void);
 
 static inline int sd_power_saving_flags(void)
 {
-	if (sched_mc_power_savings | sched_smt_power_savings)
+	if (sched_power_savings != POWERSAVINGS_DISABLED)
 		return SD_BALANCE_NEWIDLE;
 
 	return 0;
diff --git a/include/linux/topology.h b/include/linux/topology.h
index e26db03..61f3659 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -79,10 +79,7 @@ int arch_update_cpu_topology(void);
  * (Only non-zero and non-null fields need be specified.)
  */
 
-#ifdef CONFIG_SCHED_SMT
-/* MCD - Do we really need this?  It is always on if CONFIG_SCHED_SMT is,
- * so can't we drop this in favor of CONFIG_SCHED_SMT?
- */
+#ifdef CONFIG_SCHED_POWERSAVE
 #define ARCH_HAS_SCHED_WAKE_IDLE
 /* Common values for SMT siblings */
 #ifndef SD_SIBLING_INIT
@@ -110,9 +107,7 @@ int arch_update_cpu_topology(void);
 	.smt_gain		= 1178,	/* 15% */			\
 }
 #endif
-#endif /* CONFIG_SCHED_SMT */
 
-#ifdef CONFIG_SCHED_MC
 /* Common values for MC siblings. for now mostly derived from SD_CPU_INIT */
 #ifndef SD_MC_INIT
 #define SD_MC_INIT (struct sched_domain) {				\
@@ -142,7 +137,7 @@ int arch_update_cpu_topology(void);
 	.balance_interval	= 1,					\
 }
 #endif
-#endif /* CONFIG_SCHED_MC */
+#endif /* CONFIG_SCHED_POWERSAVE */
 
 /* Common values for CPUs */
 #ifndef SD_CPU_INIT
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index df00cb0..f303db8 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5924,7 +5924,7 @@ static const struct cpumask *cpu_cpu_mask(int cpu)
 	return cpumask_of_node(cpu_to_node(cpu));
 }
 
-int sched_smt_power_savings = 0, sched_mc_power_savings = 0;
+int sched_power_savings = POWERSAVINGS_DEFAULT;
 
 struct sd_data {
 	struct sched_domain **__percpu sd;
@@ -6150,10 +6150,8 @@ SD_INIT_FUNC(CPU)
  SD_INIT_FUNC(ALLNODES)
  SD_INIT_FUNC(NODE)
 #endif
-#ifdef CONFIG_SCHED_SMT
+#ifdef CONFIG_SCHED_POWERSAVE
  SD_INIT_FUNC(SIBLING)
-#endif
-#ifdef CONFIG_SCHED_MC
  SD_INIT_FUNC(MC)
 #endif
 #ifdef CONFIG_SCHED_BOOK
@@ -6250,7 +6248,7 @@ static void claim_allocations(int cpu, struct sched_domain *sd)
 		*per_cpu_ptr(sdd->sgp, cpu) = NULL;
 }
 
-#ifdef CONFIG_SCHED_SMT
+#ifdef CONFIG_SCHED_POWERSAVE
 static const struct cpumask *cpu_smt_mask(int cpu)
 {
 	return topology_thread_cpumask(cpu);
@@ -6261,10 +6259,8 @@ static const struct cpumask *cpu_smt_mask(int cpu)
  * Topology list, bottom-up.
  */
 static struct sched_domain_topology_level default_topology[] = {
-#ifdef CONFIG_SCHED_SMT
+#ifdef CONFIG_SCHED_POWERSAVE
 	{ sd_init_SIBLING, cpu_smt_mask, },
-#endif
-#ifdef CONFIG_SCHED_MC
 	{ sd_init_MC, cpu_coregroup_mask, },
 #endif
 #ifdef CONFIG_SCHED_BOOK
@@ -6635,7 +6631,7 @@ match2:
 	mutex_unlock(&sched_domains_mutex);
 }
 
-#if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
+#if defined(CONFIG_SCHED_POWERSAVE)
 static void reinit_sched_domains(void)
 {
 	get_online_cpus();
@@ -6647,7 +6643,9 @@ static void reinit_sched_domains(void)
 	put_online_cpus();
 }
 
-static ssize_t sched_power_savings_store(const char *buf, size_t count, int smt)
+static ssize_t sched_power_savings_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t count)
 {
 	unsigned int level = 0;
 
@@ -6656,75 +6654,40 @@ static ssize_t sched_power_savings_store(const char *buf, size_t count, int smt)
 
 	/*
 	 * level is always be positive so don't check for
-	 * level < POWERSAVINGS_BALANCE_NONE which is 0
+	 * level < POWERSAVINGS_DEFAULT which is 0
 	 * What happens on 0 or 1 byte write,
 	 * need to check for count as well?
 	 */
 
-	if (level >= MAX_POWERSAVINGS_BALANCE_LEVELS)
+	if (level > POWERSAVINGS_MAX)
 		return -EINVAL;
 
-	if (smt)
-		sched_smt_power_savings = level;
-	else
-		sched_mc_power_savings = level;
+		sched_power_savings = level;
 
 	reinit_sched_domains();
 
 	return count;
 }
 
-#ifdef CONFIG_SCHED_MC
-static ssize_t sched_mc_power_savings_show(struct device *dev,
+static ssize_t sched_power_savings_show(struct device *dev,
 					   struct device_attribute *attr,
 					   char *buf)
 {
-	return sprintf(buf, "%u\n", sched_mc_power_savings);
-}
-static ssize_t sched_mc_power_savings_store(struct device *dev,
-					    struct device_attribute *attr,
-					    const char *buf, size_t count)
-{
-	return sched_power_savings_store(buf, count, 0);
-}
-static DEVICE_ATTR(sched_mc_power_savings, 0644,
-		   sched_mc_power_savings_show,
-		   sched_mc_power_savings_store);
-#endif
-
-#ifdef CONFIG_SCHED_SMT
-static ssize_t sched_smt_power_savings_show(struct device *dev,
-					    struct device_attribute *attr,
-					    char *buf)
-{
-	return sprintf(buf, "%u\n", sched_smt_power_savings);
-}
-static ssize_t sched_smt_power_savings_store(struct device *dev,
-					    struct device_attribute *attr,
-					     const char *buf, size_t count)
-{
-	return sched_power_savings_store(buf, count, 1);
+	return sprintf(buf, "%u\n", sched_power_savings);
 }
-static DEVICE_ATTR(sched_smt_power_savings, 0644,
-		   sched_smt_power_savings_show,
-		   sched_smt_power_savings_store);
-#endif
+static DEVICE_ATTR(sched_power_savings, 0644,
+		   sched_power_savings_show,
+		   sched_power_savings_store);
 
 int __init sched_create_sysfs_power_savings_entries(struct device *dev)
 {
 	int err = 0;
 
-#ifdef CONFIG_SCHED_SMT
-	if (smt_capable())
-		err = device_create_file(dev, &dev_attr_sched_smt_power_savings);
-#endif
-#ifdef CONFIG_SCHED_MC
-	if (!err && mc_capable())
-		err = device_create_file(dev, &dev_attr_sched_mc_power_savings);
-#endif
+	if (mc_capable() || smt_capable())
+		err = device_create_file(dev, &dev_attr_sched_power_savings);
 	return err;
 }
-#endif /* CONFIG_SCHED_MC || CONFIG_SCHED_SMT */
+#endif /* CONFIG_SCHED_POWERSAVE */
 
 /*
  * Update cpusets according to cpu_active mask.  If cpusets are
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 84adb2d..bae6ec8 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3497,7 +3497,7 @@ struct sd_lb_stats {
 	unsigned int  busiest_group_weight;
 
 	int group_imb; /* Is there imbalance in this sd */
-#if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
+#ifdef CONFIG_SCHED_POWERSAVE
 	int power_savings_balance; /* Is powersave balance needed for this sd */
 	struct sched_group *group_min; /* Least loaded group in sd */
 	struct sched_group *group_leader; /* Group which relieves group_min */
@@ -3549,7 +3549,7 @@ static inline int get_sd_load_idx(struct sched_domain *sd,
 }
 
 
-#if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
+#ifdef CONFIG_SCHED_POWERSAVE
 /**
  * init_sd_power_savings_stats - Initialize power savings statistics for
  * the given sched_domain, during load balancing.
@@ -3669,7 +3669,7 @@ static inline int check_power_save_busiest_group(struct sd_lb_stats *sds,
 	return 1;
 
 }
-#else /* CONFIG_SCHED_MC || CONFIG_SCHED_SMT */
+#else /* CONFIG_SCHED_POWERSAVE */
 static inline void init_sd_power_savings_stats(struct sched_domain *sd,
 	struct sd_lb_stats *sds, enum cpu_idle_type idle)
 {
@@ -3687,7 +3687,7 @@ static inline int check_power_save_busiest_group(struct sd_lb_stats *sds,
 {
 	return 0;
 }
-#endif /* CONFIG_SCHED_MC || CONFIG_SCHED_SMT */
+#endif /* CONFIG_SCHED_POWERSAVE */
 
 
 unsigned long default_scale_freq_power(struct sched_domain *sd, int cpu)
@@ -4422,9 +4422,10 @@ static int need_active_balance(struct sched_domain *sd, int idle,
 		 *
 		 * The package power saving logic comes from
 		 * find_busiest_group(). If there are no imbalance, then
-		 * f_b_g() will return NULL. However when sched_mc={1,2} then
-		 * f_b_g() will select a group from which a running task may be
-		 * pulled to this cpu in order to make the other package idle.
+		 * f_b_g() will return NULL. However when
+		 * sched_powersavings={1,2} then f_b_g() will select a group
+		 * from which a running task may be pulled to this cpu
+		 * in order to make the other package idle.
 		 * If there is no opportunity to make a package idle and if
 		 * there are no imbalance, then f_b_g() will return NULL and no
 		 * action will be taken in load_balance_newidle().
@@ -4434,7 +4435,7 @@ static int need_active_balance(struct sched_domain *sd, int idle,
 		 * move_tasks() will succeed.  ld_moved will be true and this
 		 * active balance code will not be triggered.
 		 */
-		if (sched_mc_power_savings < POWERSAVINGS_BALANCE_WAKEUP)
+		if (sched_power_savings < POWERSAVINGS_MAX)
 			return 0;
 	}
 
@@ -4739,7 +4740,7 @@ static struct {
 	unsigned long next_balance;     /* in jiffy units */
 } nohz ____cacheline_aligned;
 
-#if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
+#ifdef CONFIG_SCHED_POWERSAVE
 /**
  * lowest_flag_domain - Return lowest sched_domain containing flag.
  * @cpu:	The cpu whose lowest level of sched domain is to
@@ -4796,7 +4797,7 @@ static int find_new_ilb(int cpu)
 	 * Have idle load balancer selection from semi-idle packages only
 	 * when power-aware load balancing is enabled
 	 */
-	if (!(sched_smt_power_savings || sched_mc_power_savings))
+	if (!(sched_power_savings))
 		goto out_done;
 
 	/*
@@ -4831,7 +4832,7 @@ out_done:
 
 	return nr_cpu_ids;
 }
-#else /*  (CONFIG_SCHED_MC || CONFIG_SCHED_SMT) */
+#else /*  (CONFIG_SCHED_POWERSAVE) */
 static inline int find_new_ilb(int call_cpu)
 {
 	return nr_cpu_ids;

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> +++ b/block/blk.h
> @@ -167,14 +167,15 @@ static inline int queue_congestion_off_threshold(struct request_queue *q)
>  static inline int blk_cpu_to_group(int cpu)
>  {
>         int group = NR_CPUS;
> -#ifdef CONFIG_SCHED_MC
> -       const struct cpumask *mask = cpu_coregroup_mask(cpu);
> -       group = cpumask_first(mask);
> -#elif defined(CONFIG_SCHED_SMT)
> -       group = cpumask_first(topology_thread_cpumask(cpu));
> +#ifdef CONFIG_SCHED_POWERSAVE
> +       if (smt_capable())
> +               group = cpumask_first(topology_thread_cpumask(cpu));
> +       else    
> +               group = cpumask_first(cpu_coregroup_mask(cpu));
>  #else
>         return cpu;
>  #endif
> +       /* Possible dead code?? */
>         if (likely(group < NR_CPUS))
>                 return group;
>         return cpu; 

After going, WTF is block doing! I had a closer look and this doesn't
seem right at all. The old code would use coregroup_mask when SCHED_MC
&& SCHED_SMT, the new code does something else.

Jens, what is this thing trying to do?

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Jens Axboe]

On 01/25/2012 03:53 PM, Peter Zijlstra wrote:
> On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
>> +++ b/block/blk.h
>> @@ -167,14 +167,15 @@ static inline int queue_congestion_off_threshold(struct request_queue *q)
>>  static inline int blk_cpu_to_group(int cpu)
>>  {
>>         int group = NR_CPUS;
>> -#ifdef CONFIG_SCHED_MC
>> -       const struct cpumask *mask = cpu_coregroup_mask(cpu);
>> -       group = cpumask_first(mask);
>> -#elif defined(CONFIG_SCHED_SMT)
>> -       group = cpumask_first(topology_thread_cpumask(cpu));
>> +#ifdef CONFIG_SCHED_POWERSAVE
>> +       if (smt_capable())
>> +               group = cpumask_first(topology_thread_cpumask(cpu));
>> +       else    
>> +               group = cpumask_first(cpu_coregroup_mask(cpu));
>>  #else
>>         return cpu;
>>  #endif
>> +       /* Possible dead code?? */
>>         if (likely(group < NR_CPUS))
>>                 return group;
>>         return cpu; 
> 
> After going, WTF is block doing! I had a closer look and this doesn't
> seem right at all. The old code would use coregroup_mask when SCHED_MC
> && SCHED_SMT, the new code does something else.
> 
> Jens, what is this thing trying to do?

Not surprised that it's broken for some configs. The intent of the code
is to return the first CPU in the "group" that the passed in core/thread
belongs to. This is used to decide whether to perform a completion
locally, or to send it off to a different "group".

-- 
Jens Axboe

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Thu, 2012-01-26 at 11:42 +0100, Jens Axboe wrote:
> On 01/25/2012 03:53 PM, Peter Zijlstra wrote:

> > Jens, what is this thing trying to do?
> 
>The intent of the code
> is to return the first CPU in the "group" that the passed in core/thread
> belongs to. This is used to decide whether to perform a completion
> locally, or to send it off to a different "group".

Would you perhaps have meant to identify some shared cache domain?

In the scheduler core code we have (for CONFIG_SMP):

  static int ttwu_share_cache(int this_cpu, int that_cpu);

which returns true if this and that share a cache and false otherwise.
Would that suffice or do you need a slightly different form? That is, we
should provide you with some API and avoid you having to poke around
with CONFIG_SCHED* and topology bits methinks.

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Jens Axboe]

On 01/26/2012 12:08 PM, Peter Zijlstra wrote:
> On Thu, 2012-01-26 at 11:42 +0100, Jens Axboe wrote:
>> On 01/25/2012 03:53 PM, Peter Zijlstra wrote:
> 
>>> Jens, what is this thing trying to do?
>>
>> The intent of the code
>> is to return the first CPU in the "group" that the passed in core/thread
>> belongs to. This is used to decide whether to perform a completion
>> locally, or to send it off to a different "group".
> 
> Would you perhaps have meant to identify some shared cache domain?
> 
> In the scheduler core code we have (for CONFIG_SMP):
> 
>   static int ttwu_share_cache(int this_cpu, int that_cpu);
> 
> which returns true if this and that share a cache and false otherwise.
> Would that suffice or do you need a slightly different form? That is, we
> should provide you with some API and avoid you having to poke around
> with CONFIG_SCHED* and topology bits methinks.

Yeah, I think that would suit my purpose nicely, in fact. What level of
cache sharing is being used here? The block code wanted a per-socket
type operation, but since it's a heuristic, perhaps the above is even
better (or equivelant, perhaps).

-- 
Jens Axboe

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Thu, 2012-01-26 at 12:26 +0100, Jens Axboe wrote:
> O
> Yeah, I think that would suit my purpose nicely, in fact. What level of
> cache sharing is being used here? The block code wanted a per-socket
> type operation, but since it's a heuristic, perhaps the above is even
> better (or equivelant, perhaps).

It uses the biggest shared cache exposed in the topology information the
scheduler has (which is currently somewhat funny but is on the todo list
for improvements).

Effectively it ends up being the socket wide LLC for modern Intel chips
though.

Would something like the below work for you (compile tested only).

---
Subject: sched, block: Unify cache detection
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Thu Jan 26 12:44:34 CET 2012

The block layer has some code trying to determine if two CPUs share a
cache, the scheduler has a similar function. Expose the function used
by the scheduler and make the block layer use it, thereby removing the
block layers usage of CONFIG_SCHED* and topology bits.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 block/blk-softirq.c   |   16 ++++++++--------
 block/blk.h           |   16 ----------------
 include/linux/sched.h |    8 ++++++++
 kernel/sched/core.c   |    6 +++---
 4 files changed, 19 insertions(+), 27 deletions(-)

--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -8,6 +8,7 @@
 #include <linux/blkdev.h>
 #include <linux/interrupt.h>
 #include <linux/cpu.h>
+#include <linux/sched.h>
 
 #include "blk.h"
 
@@ -103,9 +104,10 @@ static struct notifier_block __cpuinitda
 
 void __blk_complete_request(struct request *req)
 {
-	int ccpu, cpu, group_cpu = NR_CPUS;
+	int ccpu, cpu;
 	struct request_queue *q = req->q;
 	unsigned long flags;
+	bool shared = false;
 
 	BUG_ON(!q->softirq_done_fn);
 
@@ -117,22 +119,20 @@ void __blk_complete_request(struct reque
 	 */
 	if (req->cpu != -1) {
 		ccpu = req->cpu;
-		if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags)) {
-			ccpu = blk_cpu_to_group(ccpu);
-			group_cpu = blk_cpu_to_group(cpu);
-		}
+		if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags))
+			shared = cpus_share_cache(cpu, ccpu);
 	} else
 		ccpu = cpu;
 
 	/*
-	 * If current CPU and requested CPU are in the same group, running
-	 * softirq in current CPU. One might concern this is just like
+	 * If current CPU and requested CPU share a cache, run the softirq on
+	 * the current CPU. One might concern this is just like
 	 * QUEUE_FLAG_SAME_FORCE, but actually not. blk_complete_request() is
 	 * running in interrupt handler, and currently I/O controller doesn't
 	 * support multiple interrupts, so current CPU is unique actually. This
 	 * avoids IPI sending from current CPU to the first CPU of a group.
 	 */
-	if (ccpu == cpu || ccpu == group_cpu) {
+	if (ccpu == cpu || shared) {
 		struct list_head *list;
 do_local:
 		list = &__get_cpu_var(blk_cpu_done);
--- a/block/blk.h
+++ b/block/blk.h
@@ -164,22 +164,6 @@ static inline int queue_congestion_off_t
 	return q->nr_congestion_off;
 }
 
-static inline int blk_cpu_to_group(int cpu)
-{
-	int group = NR_CPUS;
-#ifdef CONFIG_SCHED_MC
-	const struct cpumask *mask = cpu_coregroup_mask(cpu);
-	group = cpumask_first(mask);
-#elif defined(CONFIG_SCHED_SMT)
-	group = cpumask_first(topology_thread_cpumask(cpu));
-#else
-	return cpu;
-#endif
-	if (likely(group < NR_CPUS))
-		return group;
-	return cpu;
-}
-
 /*
  * Contribute to IO statistics IFF:
  *
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1052,6 +1052,8 @@ static inline int test_sd_parent(struct
 unsigned long default_scale_freq_power(struct sched_domain *sd, int cpu);
 unsigned long default_scale_smt_power(struct sched_domain *sd, int cpu);
 
+bool cpus_share_cache(int this_cpu, int that_cpu);
+
 #else /* CONFIG_SMP */
 
 struct sched_domain_attr;
@@ -1061,6 +1063,12 @@ partition_sched_domains(int ndoms_new, c
 			struct sched_domain_attr *dattr_new)
 {
 }
+
+static inline bool cpus_share_cache(int this_cpu, int that_cpu)
+{
+	return true;
+}
+
 #endif	/* !CONFIG_SMP */
 
 
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1512,7 +1512,7 @@ static int ttwu_activate_remote(struct t
 }
 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
 
-static inline int ttwu_share_cache(int this_cpu, int that_cpu)
+bool cpus_share_cache(int this_cpu, int that_cpu)
 {
 	return per_cpu(sd_llc_id, this_cpu) == per_cpu(sd_llc_id, that_cpu);
 }
@@ -1523,7 +1523,7 @@ static void ttwu_queue(struct task_struc
 	struct rq *rq = cpu_rq(cpu);
 
 #if defined(CONFIG_SMP)
-	if (sched_feat(TTWU_QUEUE) && !ttwu_share_cache(smp_processor_id(), cpu)) {
+	if (sched_feat(TTWU_QUEUE) && !cpus_share_cache(smp_processor_id(), cpu)) {
 		sched_clock_cpu(cpu); /* sync clocks x-cpu */
 		ttwu_queue_remote(p, cpu);
 		return;
@@ -5759,7 +5759,7 @@ static void destroy_sched_domains(struct
  *
  * Also keep a unique ID per domain (we use the first cpu number in
  * the cpumask of the domain), this allows us to quickly tell if
- * two cpus are in the same cache domain, see ttwu_share_cache().
+ * two cpus are in the same cache domain, see cpus_share_cache().
  */
 DEFINE_PER_CPU(struct sched_domain *, sd_llc);
 DEFINE_PER_CPU(int, sd_llc_id);

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Jens Axboe]

On 01/26/2012 01:04 PM, Peter Zijlstra wrote:
> On Thu, 2012-01-26 at 12:26 +0100, Jens Axboe wrote:
>> O
>> Yeah, I think that would suit my purpose nicely, in fact. What level of
>> cache sharing is being used here? The block code wanted a per-socket
>> type operation, but since it's a heuristic, perhaps the above is even
>> better (or equivelant, perhaps).
> 
> It uses the biggest shared cache exposed in the topology information the
> scheduler has (which is currently somewhat funny but is on the todo list
> for improvements).
> 
> Effectively it ends up being the socket wide LLC for modern Intel chips
> though.
> 
> Would something like the below work for you (compile tested only).

Yep, looks good to me, and an improvement for me not to carry this code
that doesn't really belong there.

-- 
Jens Axboe

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Thu, 2012-01-26 at 13:13 +0100, Jens Axboe wrote:
> Yep, looks good to me, and an improvement for me not to carry this
> code
> that doesn't really belong there. 

I'll take this as an ACK from you and slam the patch in my 3.4 queue.
Yell if I misconstrued your intent.

Thanks!
 

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Jens Axboe]

On 01/26/2012 01:39 PM, Peter Zijlstra wrote:
> On Thu, 2012-01-26 at 13:13 +0100, Jens Axboe wrote:
>> Yep, looks good to me, and an improvement for me not to carry this
>> code
>> that doesn't really belong there. 
> 
> I'll take this as an ACK from you and slam the patch in my 3.4 queue.
> Yell if I misconstrued your intent.

Sorry, should have been clearer. Yes, you can add my acked-by. Thanks
Peter!

-- 
Jens Axboe

[tip:sched/core] sched, block: Unify cache detection

[tip-bot for Peter Zijlstra]

Commit-ID:  39be350127ec60a078edffe5b4915dafba4ba514
Gitweb:     http://git.kernel.org/tip/39be350127ec60a078edffe5b4915dafba4ba514
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Thu, 26 Jan 2012 12:44:34 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 27 Jan 2012 13:28:48 +0100

sched, block: Unify cache detection

The block layer has some code trying to determine if two CPUs share a
cache, the scheduler has a similar function. Expose the function used
by the scheduler and make the block layer use it, thereby removing the
block layers usage of CONFIG_SCHED* and topology bits.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Jens Axboe <axboe@kernel.dk>
Link: http://lkml.kernel.org/r/1327579450.2446.95.camel@twins
---
 block/blk-softirq.c   |   16 ++++++++--------
 block/blk.h           |   16 ----------------
 include/linux/sched.h |    8 ++++++++
 kernel/sched/core.c   |    6 +++---
 4 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index 1366a89..467c8de 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -8,6 +8,7 @@
 #include <linux/blkdev.h>
 #include <linux/interrupt.h>
 #include <linux/cpu.h>
+#include <linux/sched.h>
 
 #include "blk.h"
 
@@ -103,9 +104,10 @@ static struct notifier_block __cpuinitdata blk_cpu_notifier = {
 
 void __blk_complete_request(struct request *req)
 {
-	int ccpu, cpu, group_cpu = NR_CPUS;
+	int ccpu, cpu;
 	struct request_queue *q = req->q;
 	unsigned long flags;
+	bool shared = false;
 
 	BUG_ON(!q->softirq_done_fn);
 
@@ -117,22 +119,20 @@ void __blk_complete_request(struct request *req)
 	 */
 	if (req->cpu != -1) {
 		ccpu = req->cpu;
-		if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags)) {
-			ccpu = blk_cpu_to_group(ccpu);
-			group_cpu = blk_cpu_to_group(cpu);
-		}
+		if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags))
+			shared = cpus_share_cache(cpu, ccpu);
 	} else
 		ccpu = cpu;
 
 	/*
-	 * If current CPU and requested CPU are in the same group, running
-	 * softirq in current CPU. One might concern this is just like
+	 * If current CPU and requested CPU share a cache, run the softirq on
+	 * the current CPU. One might concern this is just like
 	 * QUEUE_FLAG_SAME_FORCE, but actually not. blk_complete_request() is
 	 * running in interrupt handler, and currently I/O controller doesn't
 	 * support multiple interrupts, so current CPU is unique actually. This
 	 * avoids IPI sending from current CPU to the first CPU of a group.
 	 */
-	if (ccpu == cpu || ccpu == group_cpu) {
+	if (ccpu == cpu || shared) {
 		struct list_head *list;
 do_local:
 		list = &__get_cpu_var(blk_cpu_done);
diff --git a/block/blk.h b/block/blk.h
index 7efd772..df5b59a 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -164,22 +164,6 @@ static inline int queue_congestion_off_threshold(struct request_queue *q)
 	return q->nr_congestion_off;
 }
 
-static inline int blk_cpu_to_group(int cpu)
-{
-	int group = NR_CPUS;
-#ifdef CONFIG_SCHED_MC
-	const struct cpumask *mask = cpu_coregroup_mask(cpu);
-	group = cpumask_first(mask);
-#elif defined(CONFIG_SCHED_SMT)
-	group = cpumask_first(topology_thread_cpumask(cpu));
-#else
-	return cpu;
-#endif
-	if (likely(group < NR_CPUS))
-		return group;
-	return cpu;
-}
-
 /*
  * Contribute to IO statistics IFF:
  *
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 513f524..0e19595 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1052,6 +1052,8 @@ static inline int test_sd_parent(struct sched_domain *sd, int flag)
 unsigned long default_scale_freq_power(struct sched_domain *sd, int cpu);
 unsigned long default_scale_smt_power(struct sched_domain *sd, int cpu);
 
+bool cpus_share_cache(int this_cpu, int that_cpu);
+
 #else /* CONFIG_SMP */
 
 struct sched_domain_attr;
@@ -1061,6 +1063,12 @@ partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
 			struct sched_domain_attr *dattr_new)
 {
 }
+
+static inline bool cpus_share_cache(int this_cpu, int that_cpu)
+{
+	return true;
+}
+
 #endif	/* !CONFIG_SMP */
 
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5255c9d..d7c4322 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1507,7 +1507,7 @@ static int ttwu_activate_remote(struct task_struct *p, int wake_flags)
 }
 #endif /* __ARCH_WANT_INTERRUPTS_ON_CTXSW */
 
-static inline int ttwu_share_cache(int this_cpu, int that_cpu)
+bool cpus_share_cache(int this_cpu, int that_cpu)
 {
 	return per_cpu(sd_llc_id, this_cpu) == per_cpu(sd_llc_id, that_cpu);
 }
@@ -1518,7 +1518,7 @@ static void ttwu_queue(struct task_struct *p, int cpu)
 	struct rq *rq = cpu_rq(cpu);
 
 #if defined(CONFIG_SMP)
-	if (sched_feat(TTWU_QUEUE) && !ttwu_share_cache(smp_processor_id(), cpu)) {
+	if (sched_feat(TTWU_QUEUE) && !cpus_share_cache(smp_processor_id(), cpu)) {
 		sched_clock_cpu(cpu); /* sync clocks x-cpu */
 		ttwu_queue_remote(p, cpu);
 		return;
@@ -5754,7 +5754,7 @@ static void destroy_sched_domains(struct sched_domain *sd, int cpu)
  *
  * Also keep a unique ID per domain (we use the first cpu number in
  * the cpumask of the domain), this allows us to quickly tell if
- * two cpus are in the same cache domain, see ttwu_share_cache().
+ * two cpus are in the same cache domain, see cpus_share_cache().
  */
 DEFINE_PER_CPU(struct sched_domain *, sd_llc);
 DEFINE_PER_CPU(int, sd_llc_id);

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Vaidyanathan Srinivasan]

* Peter Zijlstra <a.p.zijlstra@chello.nl> [2012-01-25 15:53:01]:

> On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> > +++ b/block/blk.h
> > @@ -167,14 +167,15 @@ static inline int queue_congestion_off_threshold(struct request_queue *q)
> >  static inline int blk_cpu_to_group(int cpu)
> >  {
> >         int group = NR_CPUS;
> > -#ifdef CONFIG_SCHED_MC
> > -       const struct cpumask *mask = cpu_coregroup_mask(cpu);
> > -       group = cpumask_first(mask);
> > -#elif defined(CONFIG_SCHED_SMT)
> > -       group = cpumask_first(topology_thread_cpumask(cpu));
> > +#ifdef CONFIG_SCHED_POWERSAVE
> > +       if (smt_capable())
> > +               group = cpumask_first(topology_thread_cpumask(cpu));
> > +       else    
> > +               group = cpumask_first(cpu_coregroup_mask(cpu));
> >  #else
> >         return cpu;
> >  #endif
> > +       /* Possible dead code?? */
> >         if (likely(group < NR_CPUS))
> >                 return group;
> >         return cpu; 
> 
> After going, WTF is block doing! I had a closer look and this doesn't
> seem right at all. The old code would use coregroup_mask when SCHED_MC
> && SCHED_SMT, the new code does something else.
> 
> Jens, what is this thing trying to do?

I understood the requirement as get first cpu in the 'core' on
a hyper-threaded system and first cpu in the 'socket' on
a non-threaded system for best cache affinity.  Based on Jens
explanation and Peter's patch, identifying last-level shared cache
works best for this case.

--Vaidy

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> +enum powersavings_level {
> +       POWERSAVINGS_DISABLED = 0,      /* Max performance */
> +       POWERSAVINGS_DEFAULT,           /* Kernel default policy, automatic powersave */
> +                                       /* vs performance tradeoff */
> +       POWERSAVINGS_MAX                /* Favour power savings over peformance */
>  }; 

I don't like that, I can get, OFF, AUTO, ON, but to overload that with
different policies for AUTO and ON just reeks.

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Vaidyanathan Srinivasan]

* Peter Zijlstra <a.p.zijlstra@chello.nl> [2012-01-25 15:57:26]:

> On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> > +enum powersavings_level {
> > +       POWERSAVINGS_DISABLED = 0,      /* Max performance */
> > +       POWERSAVINGS_DEFAULT,           /* Kernel default policy, automatic powersave */
> > +                                       /* vs performance tradeoff */
> > +       POWERSAVINGS_MAX                /* Favour power savings over peformance */
> >  }; 
> 
> I don't like that, I can get, OFF, AUTO, ON, but to overload that with
> different policies for AUTO and ON just reeks.

How about this:

enum powersavings_level {
       POWERSAVINGS_OFF = 0,           /* Max performance */
       POWERSAVINGS_AUTO,              /* Kernel default policy, automatic powersave */
                                       /* vs performance tradeoff */
       POWERSAVINGS_ON                 /* Max power savings */
}; 
 
Basically AUTO is where we have a 'policy' or heuristics, but simple
straight forward decisions for OFF and ON case. 

--Vaidy

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> @@ -6150,10 +6150,8 @@ SD_INIT_FUNC(CPU)
>   SD_INIT_FUNC(ALLNODES)
>   SD_INIT_FUNC(NODE)
>  #endif
> -#ifdef CONFIG_SCHED_SMT
> +#ifdef CONFIG_SCHED_POWERSAVE
>   SD_INIT_FUNC(SIBLING)
> -#endif
> -#ifdef CONFIG_SCHED_MC
>   SD_INIT_FUNC(MC)
>  #endif
>  #ifdef CONFIG_SCHED_BOOK
> @@ -6250,7 +6248,7 @@ static void claim_allocations(int cpu, struct sched_domain *sd)
>                 *per_cpu_ptr(sdd->sgp, cpu) = NULL;
>  }
>  
> -#ifdef CONFIG_SCHED_SMT
> +#ifdef CONFIG_SCHED_POWERSAVE
>  static const struct cpumask *cpu_smt_mask(int cpu)
>  {
>         return topology_thread_cpumask(cpu);
> @@ -6261,10 +6259,8 @@ static const struct cpumask *cpu_smt_mask(int cpu)
>   * Topology list, bottom-up.
>   */
>  static struct sched_domain_topology_level default_topology[] = {
> -#ifdef CONFIG_SCHED_SMT
> +#ifdef CONFIG_SCHED_POWERSAVE
>         { sd_init_SIBLING, cpu_smt_mask, },
> -#endif
> -#ifdef CONFIG_SCHED_MC
>         { sd_init_MC, cpu_coregroup_mask, },
>  #endif
>  #ifdef CONFIG_SCHED_BOOK 

I don't like this either, SCHED_{MC,SMT} here have nothing to do with
powersavings, its topology support.

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Arjan van de Ven]

On 1/25/2012 7:10 AM, Peter Zijlstra wrote:


btw... any reason why this feature is a config option with tons of
ifdefs... why not just have this available all the time?
it shouldn't be all that much code in the first place.

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Wed, 2012-01-25 at 07:12 -0800, Arjan van de Ven wrote:
> On 1/25/2012 7:10 AM, Peter Zijlstra wrote:
> 
> 
> btw... any reason why this feature is a config option with tons of
> ifdefs... why not just have this available all the time?
> it shouldn't be all that much code in the first place.

I guess its because SCHED_SMT and SCHED_MC also indicate the
availability of the topology functions like cpu_coregroup_mask() etc.

The whole powersave stuff got intermixed with all that.

But yes, I agree, we should kill all that and sort the topology stuff
differently.

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Vaidyanathan Srinivasan]

* Peter Zijlstra <a.p.zijlstra@chello.nl> [2012-01-25 16:10:13]:

> On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> > @@ -6150,10 +6150,8 @@ SD_INIT_FUNC(CPU)
> >   SD_INIT_FUNC(ALLNODES)
> >   SD_INIT_FUNC(NODE)
> >  #endif
> > -#ifdef CONFIG_SCHED_SMT
> > +#ifdef CONFIG_SCHED_POWERSAVE
> >   SD_INIT_FUNC(SIBLING)
> > -#endif
> > -#ifdef CONFIG_SCHED_MC
> >   SD_INIT_FUNC(MC)
> >  #endif
> >  #ifdef CONFIG_SCHED_BOOK
> > @@ -6250,7 +6248,7 @@ static void claim_allocations(int cpu, struct sched_domain *sd)
> >                 *per_cpu_ptr(sdd->sgp, cpu) = NULL;
> >  }
> >  
> > -#ifdef CONFIG_SCHED_SMT
> > +#ifdef CONFIG_SCHED_POWERSAVE
> >  static const struct cpumask *cpu_smt_mask(int cpu)
> >  {
> >         return topology_thread_cpumask(cpu);
> > @@ -6261,10 +6259,8 @@ static const struct cpumask *cpu_smt_mask(int cpu)
> >   * Topology list, bottom-up.
> >   */
> >  static struct sched_domain_topology_level default_topology[] = {
> > -#ifdef CONFIG_SCHED_SMT
> > +#ifdef CONFIG_SCHED_POWERSAVE
> >         { sd_init_SIBLING, cpu_smt_mask, },
> > -#endif
> > -#ifdef CONFIG_SCHED_MC
> >         { sd_init_MC, cpu_coregroup_mask, },
> >  #endif
> >  #ifdef CONFIG_SCHED_BOOK 
> 
> I don't like this either, SCHED_{MC,SMT} here have nothing to do with
> powersavings, its topology support.
 
Yes, but we don't need these domains for any other purpose other than
powersave balance.  The code overheads are not high,  I will remove
the config option and check.

--Vaidy

Re: [RFC PATCH v1 1/2] sched: unified sched_powersavings sysfs tunable

[Peter Zijlstra]

On Fri, 2012-01-27 at 14:52 +0530, Vaidyanathan Srinivasan wrote:
> * Peter Zijlstra <a.p.zijlstra@chello.nl> [2012-01-25 16:10:13]:
> 
> > On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> > > @@ -6150,10 +6150,8 @@ SD_INIT_FUNC(CPU)
> > >   SD_INIT_FUNC(ALLNODES)
> > >   SD_INIT_FUNC(NODE)
> > >  #endif
> > > -#ifdef CONFIG_SCHED_SMT
> > > +#ifdef CONFIG_SCHED_POWERSAVE
> > >   SD_INIT_FUNC(SIBLING)
> > > -#endif
> > > -#ifdef CONFIG_SCHED_MC
> > >   SD_INIT_FUNC(MC)
> > >  #endif
> > >  #ifdef CONFIG_SCHED_BOOK
> > > @@ -6250,7 +6248,7 @@ static void claim_allocations(int cpu, struct sched_domain *sd)
> > >                 *per_cpu_ptr(sdd->sgp, cpu) = NULL;
> > >  }
> > >  
> > > -#ifdef CONFIG_SCHED_SMT
> > > +#ifdef CONFIG_SCHED_POWERSAVE
> > >  static const struct cpumask *cpu_smt_mask(int cpu)
> > >  {
> > >         return topology_thread_cpumask(cpu);
> > > @@ -6261,10 +6259,8 @@ static const struct cpumask *cpu_smt_mask(int cpu)
> > >   * Topology list, bottom-up.
> > >   */
> > >  static struct sched_domain_topology_level default_topology[] = {
> > > -#ifdef CONFIG_SCHED_SMT
> > > +#ifdef CONFIG_SCHED_POWERSAVE
> > >         { sd_init_SIBLING, cpu_smt_mask, },
> > > -#endif
> > > -#ifdef CONFIG_SCHED_MC
> > >         { sd_init_MC, cpu_coregroup_mask, },
> > >  #endif
> > >  #ifdef CONFIG_SCHED_BOOK 
> > 
> > I don't like this either, SCHED_{MC,SMT} here have nothing to do with
> > powersavings, its topology support.
>  
> Yes, but we don't need these domains for any other purpose other than
> powersave balance.  The code overheads are not high,  I will remove
> the config option and check.

Yes we do, SMT perf skips siblings and only loads them when there's only
siblings left idle. Similarly for MC, we prefer to run tasks on separate
sockets when possible.

[RFC PATCH v1 2/2] sched: fix group_capacity for thread level consolidation

[Vaidyanathan Srinivasan]

sched_powersavings for threaded systems need this fix for
consolidation to sibling threads to work.  Since threads have
fractional capacity, group_capacity will turn out to be one
always and not accommodate another task in the sibling thread.

This fix makes group_capacity a function of cpumask_weight that
will enable the power saving load balancer to pack tasks among
sibling threads and keep more cores idle.

Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
---
 kernel/sched/fair.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index bae6ec8..c94e768 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4012,6 +4012,21 @@ static inline void update_sd_lb_stats(struct sched_domain *sd, int this_cpu,
 		 */
 		if (prefer_sibling && !local_group && sds->this_has_capacity)
 			sgs.group_capacity = min(sgs.group_capacity, 1UL);
+		/*
+		 * If power savings balance is set at this domain, then
+		 * make capacity equal to number of hardware threads to
+		 * accommodate more tasks until capacity is reached.
+		 */
+		else if (sd->flags & SD_POWERSAVINGS_BALANCE)
+			sgs.group_capacity =
+				cpumask_weight(sched_group_cpus(sg));
+
+			/*
+			 * The default group_capacity is rounded from sum of
+			 * fractional cpu_powers of sibling hardware threads
+			 * in order to enable fair use of available hardware
+			 * resources.
+			 */
 
 		if (local_group) {
 			sds->this_load = sgs.avg_load;

Re: [RFC PATCH v1 2/2] sched: fix group_capacity for thread level consolidation

[Peter Zijlstra]

On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> +               /*
> +                * If power savings balance is set at this domain, then
> +                * make capacity equal to number of hardware threads to
> +                * accommodate more tasks until capacity is reached.
> +                */
> +               else if (sd->flags & SD_POWERSAVINGS_BALANCE)
> +                       sgs.group_capacity =
> +                               cpumask_weight(sched_group_cpus(sg)); 

sg->group_weight perhaps?

Re: [RFC PATCH v1 2/2] sched: fix group_capacity for thread level consolidation

[Vaidyanathan Srinivasan]

* Peter Zijlstra <a.p.zijlstra@chello.nl> [2012-01-25 16:38:06]:

> On Mon, 2012-01-16 at 21:52 +0530, Vaidyanathan Srinivasan wrote:
> > +               /*
> > +                * If power savings balance is set at this domain, then
> > +                * make capacity equal to number of hardware threads to
> > +                * accommodate more tasks until capacity is reached.
> > +                */
> > +               else if (sd->flags & SD_POWERSAVINGS_BALANCE)
> > +                       sgs.group_capacity =
> > +                               cpumask_weight(sched_group_cpus(sg)); 
> 
> sg->group_weight perhaps?

Yes, I will correct this.  sg->group_weight is updated to the correct
value on sched_domain rebuild in init_sched_groups_power().

--Vaidy

Re: [RFC PATCH v1 0/2] sched: unified sched_powersavings tunables

[Vaidyanathan Srinivasan]

* Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> [2012-01-16 21:52:26]:

> Hi,
> 
> I have created the following RFC patch based on the recent discussions
> and consensus on simplifying the power aware scheduler in the kernel.
> 
> Ref: LWN: Rethinking power-aware scheduling
>      http://lwn.net/Articles/474915/

       https://lkml.org/lkml/2012/1/10/61