[PATCH 1/2] sched: Factor out nr_iowait and nr_iowait_cpu

[PATCH 1/2] sched: Factor out nr_iowait and nr_iowait_cpu

[Daniel Lezcano]

The function nr_iowait_cpu() can be used directly by nr_iowait() instead
of duplicating code.

Call nr_iowait_cpu() from nr_iowait()

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/core.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 625bc98..b88a145 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2873,6 +2873,24 @@ unsigned long long nr_context_switches(void)
 
 	return sum;
 }
+/*
+ * Consumers of these two interfaces, like for example the cpufreq menu
+ * governor are using nonsensical data. Boosting frequency for a CPU that has
+ * IO-wait which might not even end up running the task when it does become
+ * runnable.
+ */
+
+unsigned long nr_iowait_cpu(int cpu)
+{
+	return atomic_read(&cpu_rq(cpu)->nr_iowait);
+}
+
+void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
+{
+	struct rq *rq = this_rq();
+	*nr_waiters = atomic_read(&rq->nr_iowait);
+	*load = rq->load.weight;
+}
 
 /*
  * IO-wait accounting, and how its mostly bollocks (on SMP).
@@ -2909,31 +2927,11 @@ unsigned long nr_iowait(void)
 	unsigned long i, sum = 0;
 
 	for_each_possible_cpu(i)
-		sum += atomic_read(&cpu_rq(i)->nr_iowait);
+		sum += nr_iowait_cpu(i);
 
 	return sum;
 }
 
-/*
- * Consumers of these two interfaces, like for example the cpufreq menu
- * governor are using nonsensical data. Boosting frequency for a CPU that has
- * IO-wait which might not even end up running the task when it does become
- * runnable.
- */
-
-unsigned long nr_iowait_cpu(int cpu)
-{
-	struct rq *this = cpu_rq(cpu);
-	return atomic_read(&this->nr_iowait);
-}
-
-void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
-{
-	struct rq *rq = this_rq();
-	*nr_waiters = atomic_read(&rq->nr_iowait);
-	*load = rq->load.weight;
-}
-
 #ifdef CONFIG_SMP
 
 /*
-- 
2.7.4

[PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Daniel Lezcano]

The function get_loadavg() returns almost always zero. To be more
precise, statistically speaking for a total of 1023379 times passing
in the function, the load is equal to zero 1020728 times, greater than
100, 610 times, the remaining is between 0 and 5.

In 2011, the get_loadavg() was removed from the Android tree because
of the above [1]. At this time, the load was:

unsigned long this_cpu_load(void)
{
        struct rq *this = this_rq();
        return this->cpu_load[0];
}

In 2014, the code was changed by commit 372ba8cb46b2 (cpuidle: menu: Lookup CPU
runqueues less) and the load is:

void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
{
        struct rq *rq = this_rq();
        *nr_waiters = atomic_read(&rq->nr_iowait);
        *load = rq->load.weight;
}

with the same result.

Both measurements show using the load in this code path does no matter
anymore. Removing it.

[1] https://android.googlesource.com/kernel/common/+/4dedd9f124703207895777ac6e91dacde0f7cc17

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Todd Kjos <tkjos@google.com>
Cc: Joel Fernandes <joelaf@google.com>
Cc: Colin Cross <ccross@android.com>
Cc: Ramesh Thomas <ramesh.thomas@intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governors/menu.c | 26 +++++++-------------------
 include/linux/sched/stat.h       |  1 -
 kernel/sched/core.c              | 13 -------------
 3 files changed, 7 insertions(+), 33 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index e26a409..066b01f 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -135,11 +135,6 @@ struct menu_device {
 #define LOAD_INT(x) ((x) >> FSHIFT)
 #define LOAD_FRAC(x) LOAD_INT(((x) & (FIXED_1-1)) * 100)
 
-static inline int get_loadavg(unsigned long load)
-{
-	return LOAD_INT(load) * 10 + LOAD_FRAC(load) / 10;
-}
-
 static inline int which_bucket(unsigned int duration, unsigned long nr_iowaiters)
 {
 	int bucket = 0;
@@ -173,18 +168,10 @@ static inline int which_bucket(unsigned int duration, unsigned long nr_iowaiters
  * to be, the higher this multiplier, and thus the higher
  * the barrier to go to an expensive C state.
  */
-static inline int performance_multiplier(unsigned long nr_iowaiters, unsigned long load)
+static inline int performance_multiplier(unsigned long nr_iowaiters)
 {
-	int mult = 1;
-
-	/* for higher loadavg, we are more reluctant */
-
-	mult += 2 * get_loadavg(load);
-
-	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowaiters;
-
-	return mult;
+	/* for IO wait tasks (per cpu!) we add 10x each */
+	return 1 + 10 * nr_iowaiters;
 }
 
 static DEFINE_PER_CPU(struct menu_device, menu_devices);
@@ -290,7 +277,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 	int idx;
 	unsigned int interactivity_req;
 	unsigned int expected_interval;
-	unsigned long nr_iowaiters, cpu_load;
+	unsigned long nr_iowaiters;
 	ktime_t delta_next;
 
 	if (data->needs_update) {
@@ -307,7 +294,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 	/* determine the expected residency time, round up */
 	data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length(&delta_next));
 
-	get_iowait_load(&nr_iowaiters, &cpu_load);
+	nr_iowaiters = nr_iowait_cpu(dev->cpu);
 	data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
 
 	/*
@@ -359,7 +346,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		 * Use the performance multiplier and the user-configurable
 		 * latency_req to determine the maximum exit latency.
 		 */
-		interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
+		interactivity_req = data->predicted_us /
+			performance_multiplier(nr_iowaiters);
 		if (latency_req > interactivity_req)
 			latency_req = interactivity_req;
 	}
diff --git a/include/linux/sched/stat.h b/include/linux/sched/stat.h
index 04f1321..f30954c 100644
--- a/include/linux/sched/stat.h
+++ b/include/linux/sched/stat.h
@@ -20,7 +20,6 @@ extern unsigned long nr_running(void);
 extern bool single_task_running(void);
 extern unsigned long nr_iowait(void);
 extern unsigned long nr_iowait_cpu(int cpu);
-extern void get_iowait_load(unsigned long *nr_waiters, unsigned long *load);
 
 static inline int sched_info_on(void)
 {
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b88a145..5605f03 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
 
 	return sum;
 }
-/*
- * Consumers of these two interfaces, like for example the cpufreq menu
- * governor are using nonsensical data. Boosting frequency for a CPU that has
- * IO-wait which might not even end up running the task when it does become
- * runnable.
- */
 
 unsigned long nr_iowait_cpu(int cpu)
 {
 	return atomic_read(&cpu_rq(cpu)->nr_iowait);
 }
 
-void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
-{
-	struct rq *rq = this_rq();
-	*nr_waiters = atomic_read(&rq->nr_iowait);
-	*load = rq->load.weight;
-}
-
 /*
  * IO-wait accounting, and how its mostly bollocks (on SMP).
  *
-- 
2.7.4

Re: [PATCH 1/2] sched: Factor out nr_iowait and nr_iowait_cpu

[Rafael J. Wysocki]

On Thu, Oct 4, 2018 at 9:42 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The function nr_iowait_cpu() can be used directly by nr_iowait() instead
> of duplicating code.
>
> Call nr_iowait_cpu() from nr_iowait()
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  kernel/sched/core.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 625bc98..b88a145 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2873,6 +2873,24 @@ unsigned long long nr_context_switches(void)
>
>         return sum;
>  }
> +/*
> + * Consumers of these two interfaces, like for example the cpufreq menu
> + * governor are using nonsensical data. Boosting frequency for a CPU that has
> + * IO-wait which might not even end up running the task when it does become
> + * runnable.
> + */
> +
> +unsigned long nr_iowait_cpu(int cpu)
> +{
> +       return atomic_read(&cpu_rq(cpu)->nr_iowait);
> +}
> +
> +void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
> +{
> +       struct rq *rq = this_rq();
> +       *nr_waiters = atomic_read(&rq->nr_iowait);
> +       *load = rq->load.weight;
> +}
>
>  /*
>   * IO-wait accounting, and how its mostly bollocks (on SMP).
> @@ -2909,31 +2927,11 @@ unsigned long nr_iowait(void)
>         unsigned long i, sum = 0;
>
>         for_each_possible_cpu(i)
> -               sum += atomic_read(&cpu_rq(i)->nr_iowait);
> +               sum += nr_iowait_cpu(i);
>
>         return sum;
>  }
>
> -/*
> - * Consumers of these two interfaces, like for example the cpufreq menu
> - * governor are using nonsensical data. Boosting frequency for a CPU that has
> - * IO-wait which might not even end up running the task when it does become
> - * runnable.
> - */
> -
> -unsigned long nr_iowait_cpu(int cpu)
> -{
> -       struct rq *this = cpu_rq(cpu);
> -       return atomic_read(&this->nr_iowait);
> -}
> -
> -void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
> -{
> -       struct rq *rq = this_rq();
> -       *nr_waiters = atomic_read(&rq->nr_iowait);
> -       *load = rq->load.weight;
> -}
> -
>  #ifdef CONFIG_SMP
>
>  /*
> --
> 2.7.4
>

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Peter Zijlstra]

On Thu, Oct 04, 2018 at 09:42:07AM +0200, Daniel Lezcano wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b88a145..5605f03 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
>  
>  	return sum;
>  }
> -/*
> - * Consumers of these two interfaces, like for example the cpufreq menu
> - * governor are using nonsensical data. Boosting frequency for a CPU that has
> - * IO-wait which might not even end up running the task when it does become
> - * runnable.
> - */
>  
>  unsigned long nr_iowait_cpu(int cpu)
 
+static

>  {
>  	return atomic_read(&cpu_rq(cpu)->nr_iowait);
>  }
>  
> -void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
> -{
> -	struct rq *rq = this_rq();
> -	*nr_waiters = atomic_read(&rq->nr_iowait);
> -	*load = rq->load.weight;
> -}
> -
>  /*
>   * IO-wait accounting, and how its mostly bollocks (on SMP).
>   *

I'm obviously all for this :-)

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Daniel Lezcano]

Hi Peter,

On 04/10/2018 09:57, Peter Zijlstra wrote:
> On Thu, Oct 04, 2018 at 09:42:07AM +0200, Daniel Lezcano wrote:
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index b88a145..5605f03 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
>>  
>>  	return sum;
>>  }
>> -/*
>> - * Consumers of these two interfaces, like for example the cpufreq menu
>> - * governor are using nonsensical data. Boosting frequency for a CPU that has
>> - * IO-wait which might not even end up running the task when it does become
>> - * runnable.
>> - */
>>  
>>  unsigned long nr_iowait_cpu(int cpu)
>  
> +static

The function is exported in include/linux/sched/stat.h and used by
drivers/cpuidle/governors/menu.c

Do you want to declare it static inline in the stat.h file ?

>>  {
>>  	return atomic_read(&cpu_rq(cpu)->nr_iowait);
>>  }
>>  
>> -void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
>> -{
>> -	struct rq *rq = this_rq();
>> -	*nr_waiters = atomic_read(&rq->nr_iowait);
>> -	*load = rq->load.weight;
>> -}
>> -
>>  /*
>>   * IO-wait accounting, and how its mostly bollocks (on SMP).
>>   *
> 
> I'm obviously all for this :-)
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Rafael J. Wysocki]

On Thu, Oct 4, 2018 at 9:42 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> The function get_loadavg() returns almost always zero. To be more
> precise, statistically speaking for a total of 1023379 times passing
> in the function, the load is equal to zero 1020728 times, greater than
> 100, 610 times, the remaining is between 0 and 5.
>
> In 2011, the get_loadavg() was removed from the Android tree because
> of the above [1]. At this time, the load was:
>
> unsigned long this_cpu_load(void)
> {
>         struct rq *this = this_rq();
>         return this->cpu_load[0];
> }
>
> In 2014, the code was changed by commit 372ba8cb46b2 (cpuidle: menu: Lookup CPU
> runqueues less) and the load is:
>
> void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
> {
>         struct rq *rq = this_rq();
>         *nr_waiters = atomic_read(&rq->nr_iowait);
>         *load = rq->load.weight;
> }
>
> with the same result.
>
> Both measurements show using the load in this code path does no matter
> anymore. Removing it.
>
> [1] https://android.googlesource.com/kernel/common/+/4dedd9f124703207895777ac6e91dacde0f7cc17
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Joel Fernandes <joelaf@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Ramesh Thomas <ramesh.thomas@intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/governors/menu.c | 26 +++++++-------------------
>  include/linux/sched/stat.h       |  1 -
>  kernel/sched/core.c              | 13 -------------
>  3 files changed, 7 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index e26a409..066b01f 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -135,11 +135,6 @@ struct menu_device {
>  #define LOAD_INT(x) ((x) >> FSHIFT)
>  #define LOAD_FRAC(x) LOAD_INT(((x) & (FIXED_1-1)) * 100)
>
> -static inline int get_loadavg(unsigned long load)
> -{
> -       return LOAD_INT(load) * 10 + LOAD_FRAC(load) / 10;
> -}
> -
>  static inline int which_bucket(unsigned int duration, unsigned long nr_iowaiters)
>  {
>         int bucket = 0;
> @@ -173,18 +168,10 @@ static inline int which_bucket(unsigned int duration, unsigned long nr_iowaiters
>   * to be, the higher this multiplier, and thus the higher
>   * the barrier to go to an expensive C state.
>   */
> -static inline int performance_multiplier(unsigned long nr_iowaiters, unsigned long load)
> +static inline int performance_multiplier(unsigned long nr_iowaiters)
>  {
> -       int mult = 1;
> -
> -       /* for higher loadavg, we are more reluctant */
> -
> -       mult += 2 * get_loadavg(load);
> -
> -       /* for IO wait tasks (per cpu!) we add 5x each */
> -       mult += 10 * nr_iowaiters;
> -
> -       return mult;
> +       /* for IO wait tasks (per cpu!) we add 10x each */
> +       return 1 + 10 * nr_iowaiters;
>  }
>
>  static DEFINE_PER_CPU(struct menu_device, menu_devices);
> @@ -290,7 +277,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>         int idx;
>         unsigned int interactivity_req;
>         unsigned int expected_interval;
> -       unsigned long nr_iowaiters, cpu_load;
> +       unsigned long nr_iowaiters;
>         ktime_t delta_next;
>
>         if (data->needs_update) {
> @@ -307,7 +294,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>         /* determine the expected residency time, round up */
>         data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length(&delta_next));
>
> -       get_iowait_load(&nr_iowaiters, &cpu_load);
> +       nr_iowaiters = nr_iowait_cpu(dev->cpu);
>         data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
>
>         /*
> @@ -359,7 +346,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>                  * Use the performance multiplier and the user-configurable
>                  * latency_req to determine the maximum exit latency.
>                  */
> -               interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
> +               interactivity_req = data->predicted_us /
> +                       performance_multiplier(nr_iowaiters);

I wouldn't break this line.

>                 if (latency_req > interactivity_req)
>                         latency_req = interactivity_req;
>         }
> diff --git a/include/linux/sched/stat.h b/include/linux/sched/stat.h
> index 04f1321..f30954c 100644
> --- a/include/linux/sched/stat.h
> +++ b/include/linux/sched/stat.h
> @@ -20,7 +20,6 @@ extern unsigned long nr_running(void);
>  extern bool single_task_running(void);
>  extern unsigned long nr_iowait(void);
>  extern unsigned long nr_iowait_cpu(int cpu);
> -extern void get_iowait_load(unsigned long *nr_waiters, unsigned long *load);
>
>  static inline int sched_info_on(void)
>  {
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b88a145..5605f03 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
>
>         return sum;
>  }
> -/*
> - * Consumers of these two interfaces, like for example the cpufreq menu
> - * governor are using nonsensical data. Boosting frequency for a CPU that has
> - * IO-wait which might not even end up running the task when it does become
> - * runnable.
> - */

Doesn't the comment still apply to nr_iowait_cpu()?

>
>  unsigned long nr_iowait_cpu(int cpu)
>  {
>         return atomic_read(&cpu_rq(cpu)->nr_iowait);
>  }
>
> -void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
> -{
> -       struct rq *rq = this_rq();
> -       *nr_waiters = atomic_read(&rq->nr_iowait);
> -       *load = rq->load.weight;
> -}
> -
>  /*
>   * IO-wait accounting, and how its mostly bollocks (on SMP).
>   *
> --

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Daniel Lezcano]

On 04/10/2018 10:22, Rafael J. Wysocki wrote:
> On Thu, Oct 4, 2018 at 9:42 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> The function get_loadavg() returns almost always zero. To be more
>> precise, statistically speaking for a total of 1023379 times passing
>> in the function, the load is equal to zero 1020728 times, greater than
>> 100, 610 times, the remaining is between 0 and 5.
>>
>> In 2011, the get_loadavg() was removed from the Android tree because
>> of the above [1]. At this time, the load was:
>>
>> unsigned long this_cpu_load(void)
>> {
>>         struct rq *this = this_rq();
>>         return this->cpu_load[0];
>> }
>>
>> In 2014, the code was changed by commit 372ba8cb46b2 (cpuidle: menu: Lookup CPU
>> runqueues less) and the load is:
>>
>> void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
>> {
>>         struct rq *rq = this_rq();
>>         *nr_waiters = atomic_read(&rq->nr_iowait);
>>         *load = rq->load.weight;
>> }
>>
>> with the same result.
>>
>> Both measurements show using the load in this code path does no matter
>> anymore. Removing it.
>>
>> [1] https://android.googlesource.com/kernel/common/+/4dedd9f124703207895777ac6e91dacde0f7cc17
>>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Todd Kjos <tkjos@google.com>
>> Cc: Joel Fernandes <joelaf@google.com>
>> Cc: Colin Cross <ccross@android.com>
>> Cc: Ramesh Thomas <ramesh.thomas@intel.com>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/cpuidle/governors/menu.c | 26 +++++++-------------------
>>  include/linux/sched/stat.h       |  1 -
>>  kernel/sched/core.c              | 13 -------------
>>  3 files changed, 7 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>> index e26a409..066b01f 100644
>> --- a/drivers/cpuidle/governors/menu.c
>> +++ b/drivers/cpuidle/governors/menu.c
>> @@ -135,11 +135,6 @@ struct menu_device {
>>  #define LOAD_INT(x) ((x) >> FSHIFT)
>>  #define LOAD_FRAC(x) LOAD_INT(((x) & (FIXED_1-1)) * 100)
>>
>> -static inline int get_loadavg(unsigned long load)
>> -{
>> -       return LOAD_INT(load) * 10 + LOAD_FRAC(load) / 10;
>> -}
>> -
>>  static inline int which_bucket(unsigned int duration, unsigned long nr_iowaiters)
>>  {
>>         int bucket = 0;
>> @@ -173,18 +168,10 @@ static inline int which_bucket(unsigned int duration, unsigned long nr_iowaiters
>>   * to be, the higher this multiplier, and thus the higher
>>   * the barrier to go to an expensive C state.
>>   */
>> -static inline int performance_multiplier(unsigned long nr_iowaiters, unsigned long load)
>> +static inline int performance_multiplier(unsigned long nr_iowaiters)
>>  {
>> -       int mult = 1;
>> -
>> -       /* for higher loadavg, we are more reluctant */
>> -
>> -       mult += 2 * get_loadavg(load);
>> -
>> -       /* for IO wait tasks (per cpu!) we add 5x each */
>> -       mult += 10 * nr_iowaiters;
>> -
>> -       return mult;
>> +       /* for IO wait tasks (per cpu!) we add 10x each */
>> +       return 1 + 10 * nr_iowaiters;
>>  }
>>
>>  static DEFINE_PER_CPU(struct menu_device, menu_devices);
>> @@ -290,7 +277,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>         int idx;
>>         unsigned int interactivity_req;
>>         unsigned int expected_interval;
>> -       unsigned long nr_iowaiters, cpu_load;
>> +       unsigned long nr_iowaiters;
>>         ktime_t delta_next;
>>
>>         if (data->needs_update) {
>> @@ -307,7 +294,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>         /* determine the expected residency time, round up */
>>         data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length(&delta_next));
>>
>> -       get_iowait_load(&nr_iowaiters, &cpu_load);
>> +       nr_iowaiters = nr_iowait_cpu(dev->cpu);
>>         data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
>>
>>         /*
>> @@ -359,7 +346,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>                  * Use the performance multiplier and the user-configurable
>>                  * latency_req to determine the maximum exit latency.
>>                  */
>> -               interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
>> +               interactivity_req = data->predicted_us /
>> +                       performance_multiplier(nr_iowaiters);
> 
> I wouldn't break this line.

Ok, mind if I break the line in a separate patch before ? (my git
pre-commit hook runs checkpatch and it complains and prevent to commit
the patch because of the line length (94)).

>>                 if (latency_req > interactivity_req)
>>                         latency_req = interactivity_req;
>>         }
>> diff --git a/include/linux/sched/stat.h b/include/linux/sched/stat.h
>> index 04f1321..f30954c 100644
>> --- a/include/linux/sched/stat.h
>> +++ b/include/linux/sched/stat.h
>> @@ -20,7 +20,6 @@ extern unsigned long nr_running(void);
>>  extern bool single_task_running(void);
>>  extern unsigned long nr_iowait(void);
>>  extern unsigned long nr_iowait_cpu(int cpu);
>> -extern void get_iowait_load(unsigned long *nr_waiters, unsigned long *load);
>>
>>  static inline int sched_info_on(void)
>>  {
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index b88a145..5605f03 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
>>
>>         return sum;
>>  }
>> -/*
>> - * Consumers of these two interfaces, like for example the cpufreq menu
>> - * governor are using nonsensical data. Boosting frequency for a CPU that has
>> - * IO-wait which might not even end up running the task when it does become
>> - * runnable.
>> - */
> 
> Doesn't the comment still apply to nr_iowait_cpu()?

The comment is very confusing. I talks about a cpufreq menu governor and
boosting frequency. I don't see this function used in the cpufreq
framework but in:

kernel/time/tick-sched.c
fs/proc/stat.c
drivers/cpuidle/governors/menu.c

The comment is irrelevant as the remaining function is used for
statistics in addition to the perf multiplier. It does exactly what the
function name is.

>>  unsigned long nr_iowait_cpu(int cpu)
>>  {
>>         return atomic_read(&cpu_rq(cpu)->nr_iowait);
>>  }
>>
>> -void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
>> -{
>> -       struct rq *rq = this_rq();
>> -       *nr_waiters = atomic_read(&rq->nr_iowait);
>> -       *load = rq->load.weight;
>> -}
>> -
>>  /*
>>   * IO-wait accounting, and how its mostly bollocks (on SMP).
>>   *
>> --


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Rafael J. Wysocki]

On Thu, Oct 4, 2018 at 10:40 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 04/10/2018 10:22, Rafael J. Wysocki wrote:
> > On Thu, Oct 4, 2018 at 9:42 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

[cut]

> >> -               interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
> >> +               interactivity_req = data->predicted_us /
> >> +                       performance_multiplier(nr_iowaiters);
> >
> > I wouldn't break this line.
>
> Ok, mind if I break the line in a separate patch before ? (my git
> pre-commit hook runs checkpatch and it complains and prevent to commit
> the patch because of the line length (94)).

OK, just keep it the way it is, then.  I can remove the line break
before applying the patch easily enough. :-)

> >>                 if (latency_req > interactivity_req)
> >>                         latency_req = interactivity_req;
> >>         }
> >> diff --git a/include/linux/sched/stat.h b/include/linux/sched/stat.h
> >> index 04f1321..f30954c 100644
> >> --- a/include/linux/sched/stat.h
> >> +++ b/include/linux/sched/stat.h
> >> @@ -20,7 +20,6 @@ extern unsigned long nr_running(void);
> >>  extern bool single_task_running(void);
> >>  extern unsigned long nr_iowait(void);
> >>  extern unsigned long nr_iowait_cpu(int cpu);
> >> -extern void get_iowait_load(unsigned long *nr_waiters, unsigned long *load);
> >>
> >>  static inline int sched_info_on(void)
> >>  {
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index b88a145..5605f03 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
> >>
> >>         return sum;
> >>  }
> >> -/*
> >> - * Consumers of these two interfaces, like for example the cpufreq menu
> >> - * governor are using nonsensical data. Boosting frequency for a CPU that has
> >> - * IO-wait which might not even end up running the task when it does become
> >> - * runnable.
> >> - */
> >
> > Doesn't the comment still apply to nr_iowait_cpu()?
>
> The comment is very confusing. I talks about a cpufreq menu governor and
> boosting frequency.

So it should be talking about the *cpuidle* menu governor and
preferring shallow idle state selection.

I guess I'll send a patch to update it. :-)

> I don't see this function used in the cpufreq framework but in:
>
> kernel/time/tick-sched.c
> fs/proc/stat.c
> drivers/cpuidle/governors/menu.c
>
> The comment is irrelevant as the remaining function is used for
> statistics in addition to the perf multiplier. It does exactly what the
> function name is.

Which is my point.  It shouldn't be dropped entirely, but updated IMO.

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Peter Zijlstra]

On Thu, Oct 04, 2018 at 10:12:44AM +0200, Daniel Lezcano wrote:
> 
> Hi Peter,
> 
> On 04/10/2018 09:57, Peter Zijlstra wrote:
> > On Thu, Oct 04, 2018 at 09:42:07AM +0200, Daniel Lezcano wrote:
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index b88a145..5605f03 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -2873,25 +2873,12 @@ unsigned long long nr_context_switches(void)
> >>  
> >>  	return sum;
> >>  }
> >> -/*
> >> - * Consumers of these two interfaces, like for example the cpufreq menu
> >> - * governor are using nonsensical data. Boosting frequency for a CPU that has
> >> - * IO-wait which might not even end up running the task when it does become
> >> - * runnable.
> >> - */
> >>  
> >>  unsigned long nr_iowait_cpu(int cpu)
> >  
> > +static
> 
> The function is exported in include/linux/sched/stat.h and used by
> drivers/cpuidle/governors/menu.c
> 
> Do you want to declare it static inline in the stat.h file ?

No, but if there's a user left, that comment needs to stay. The number
it returns is utter crap.

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Peter Zijlstra]

On Thu, Oct 04, 2018 at 10:47:04AM +0200, Rafael J. Wysocki wrote:

> > The comment is irrelevant as the remaining function is used for
> > statistics in addition to the perf multiplier. It does exactly what the
> > function name is.
> 
> Which is my point.  It shouldn't be dropped entirely, but updated IMO.

But the function name isn't the problem.. the problem that any use of
the number is fundamentally broken.

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Rafael J. Wysocki]

On Thursday, October 4, 2018 11:36:44 AM CEST Peter Zijlstra wrote:
> On Thu, Oct 04, 2018 at 10:47:04AM +0200, Rafael J. Wysocki wrote:
> 
> > > The comment is irrelevant as the remaining function is used for
> > > statistics in addition to the perf multiplier. It does exactly what the
> > > function name is.
> > 
> > Which is my point.  It shouldn't be dropped entirely, but updated IMO.
> 
> But the function name isn't the problem.. the problem that any use of
> the number is fundamentally broken.
> 

We're in violent agreement here. :-)

I just wanted to say that the comment was still relevant at least with
respect to this function, but the patch was removing the comment
entirely.

Re: [PATCH 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Joe Perches]

On Thu, 2018-10-04 at 10:40 +0200, Daniel Lezcano wrote:
> On 04/10/2018 10:22, Rafael J. Wysocki wrote:
> > On Thu, Oct 4, 2018 at 9:42 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> > > The function get_loadavg() returns almost always zero. To be more
> > > precise, statistically speaking for a total of 1023379 times passing
> > > in the function, the load is equal to zero 1020728 times, greater than
> > > 100, 610 times, the remaining is between 0 and 5.
[]
> > > diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
[]
> > > @@ -359,7 +346,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > >                  * Use the performance multiplier and the user-configurable
> > >                  * latency_req to determine the maximum exit latency.
> > >                  */
> > > -               interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
> > > +               interactivity_req = data->predicted_us /
> > > +                       performance_multiplier(nr_iowaiters);
> > 
> > I wouldn't break this line.
> 
> Ok, mind if I break the line in a separate patch before ? (my git
> pre-commit hook runs checkpatch and it complains and prevent to commit
> the patch because of the line length (94)).

A lot of this file uses > 80 column lines.
As well, the identifiers here are relatively long.

Long identifier lengths and 80 column lines are generally
incompatible.

I suggest you change your script to allow > 80 column lines
using the checkpatch command-line option --ignore==long_line
or perhaps add --max-line-length=<some higher value> or at
least allow some other senstible interactivity when the silly
script bleats some mindless warning or another.