All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 1/2] sched: Factor out nr_iowait and nr_iowait_cpu
@ 2018-10-04 12:04 ` Daniel Lezcano
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Lezcano @ 2018-10-04 12:04 UTC (permalink / raw)
  To: rafael; +Cc: rjw, linux-pm, Ingo Molnar, Peter Zijlstra, open list:SCHEDULER

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

Call nr_iowait_cpu() from nr_iowait()

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/core.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 625bc98..43efb74 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2875,6 +2875,25 @@ unsigned long long nr_context_switches(void)
 }
 
 /*
+ * 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).
  *
  * The idea behind IO-wait account is to account the idle time that we could
@@ -2909,31 +2928,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V2 1/2] sched: Factor out nr_iowait and nr_iowait_cpu
@ 2018-10-04 12:04 ` Daniel Lezcano
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Lezcano @ 2018-10-04 12:04 UTC (permalink / raw)
  To: rafael; +Cc: rjw, linux-pm, Ingo Molnar, Peter Zijlstra, open list:SCHEDULER

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

Call nr_iowait_cpu() from nr_iowait()

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/core.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 625bc98..43efb74 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2875,6 +2875,25 @@ unsigned long long nr_context_switches(void)
 }
 
 /*
+ * 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).
  *
  * The idea behind IO-wait account is to account the idle time that we could
@@ -2909,31 +2928,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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V2 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier
  2018-10-04 12:04 ` Daniel Lezcano
@ 2018-10-04 12:04   ` Daniel Lezcano
  -1 siblings, 0 replies; 9+ messages in thread
From: Daniel Lezcano @ 2018-10-04 12:04 UTC (permalink / raw)
  To: rafael
  Cc: rjw, linux-pm, Peter Zijlstra, Todd Kjos, Joel Fernandes,
	Colin Cross, Ramesh Thomas, Mel Gorman, Ingo Molnar,
	Rafael J. Wysocki, Alex Shi, Thomas Gleixner,
	Philippe Ombredanne, Greg Kroah-Hartman, Kate Stewart, open list

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              |  7 -------
 3 files changed, 7 insertions(+), 27 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 43efb74..48a786a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2886,13 +2886,6 @@ 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V2 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier
@ 2018-10-04 12:04   ` Daniel Lezcano
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Lezcano @ 2018-10-04 12:04 UTC (permalink / raw)
  To: rafael
  Cc: rjw, linux-pm, Peter Zijlstra, Todd Kjos, Joel Fernandes,
	Colin Cross, Ramesh Thomas, Mel Gorman, Ingo Molnar,
	Rafael J. Wysocki, Alex Shi, Thomas Gleixner,
	Philippe Ombredanne, Greg Kroah-Hartman, Kate Stewart, open list

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              |  7 -------
 3 files changed, 7 insertions(+), 27 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 43efb74..48a786a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2886,13 +2886,6 @@ 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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier
  2018-10-04 12:04   ` Daniel Lezcano
  (?)
@ 2018-10-04 12:28   ` Mel Gorman
  -1 siblings, 0 replies; 9+ messages in thread
From: Mel Gorman @ 2018-10-04 12:28 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rafael, rjw, linux-pm, Peter Zijlstra, Todd Kjos, Joel Fernandes,
	Colin Cross, Ramesh Thomas, Ingo Molnar, Rafael J. Wysocki,
	Alex Shi, Thomas Gleixner, Philippe Ombredanne,
	Greg Kroah-Hartman, Kate Stewart, open list

On Thu, Oct 04, 2018 at 02:04:03PM +0200, Daniel Lezcano 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>

I agree that removing this is the most sensible option so;

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier
  2018-10-04 12:04   ` Daniel Lezcano
  (?)
  (?)
@ 2018-10-04 15:23   ` Rafael J. Wysocki
  -1 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2018-10-04 15:23 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM, Peter Zijlstra,
	Todd Kjos, Joel Fernandes, Colin Cross, Ramesh Thomas,
	Mel Gorman, Ingo Molnar, Rafael Wysocki, Alex Shi,
	Thomas Gleixner, Philippe Ombredanne, Greg Kroah-Hartman,
	Kate Stewart, Linux Kernel Mailing List

On Thu, Oct 4, 2018 at 2:04 PM 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>

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

> ---
>  drivers/cpuidle/governors/menu.c | 26 +++++++-------------------
>  include/linux/sched/stat.h       |  1 -
>  kernel/sched/core.c              |  7 -------
>  3 files changed, 7 insertions(+), 27 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 43efb74..48a786a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2886,13 +2886,6 @@ 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
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 1/2] sched: Factor out nr_iowait and nr_iowait_cpu
  2018-10-04 12:04 ` Daniel Lezcano
  (?)
  (?)
@ 2018-10-12  8:28 ` Peter Zijlstra
  -1 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2018-10-12  8:28 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: rafael, rjw, linux-pm, Ingo Molnar, open list:SCHEDULER

On Thu, Oct 04, 2018 at 02:04:02PM +0200, Daniel Lezcano wrote:
> The function nr_iowait_cpu() can be used directly by nr_iowait() instead
> of duplicating code.
> 
> Call nr_iowait_cpu() from nr_iowait()
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier
  2018-10-04 12:04   ` Daniel Lezcano
                     ` (2 preceding siblings ...)
  (?)
@ 2018-10-12  8:28   ` Peter Zijlstra
  -1 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2018-10-12  8:28 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rafael, rjw, linux-pm, Todd Kjos, Joel Fernandes, Colin Cross,
	Ramesh Thomas, Mel Gorman, Ingo Molnar, Rafael J. Wysocki,
	Alex Shi, Thomas Gleixner, Philippe Ombredanne,
	Greg Kroah-Hartman, Kate Stewart, open list

On Thu, Oct 04, 2018 at 02:04:03PM +0200, Daniel Lezcano 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>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 1/2] sched: Factor out nr_iowait and nr_iowait_cpu
  2018-10-04 12:04 ` Daniel Lezcano
                   ` (2 preceding siblings ...)
  (?)
@ 2018-10-26  9:22 ` Rafael J. Wysocki
  -1 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2018-10-26  9:22 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rafael, linux-pm, Ingo Molnar, Peter Zijlstra, open list:SCHEDULER

On Thursday, October 4, 2018 2:04:02 PM CEST Daniel Lezcano wrote:
> The function nr_iowait_cpu() can be used directly by nr_iowait() instead
> of duplicating code.
> 
> Call nr_iowait_cpu() from nr_iowait()
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  kernel/sched/core.c | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 625bc98..43efb74 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2875,6 +2875,25 @@ unsigned long long nr_context_switches(void)
>  }
>  
>  /*
> + * 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).
>   *
>   * The idea behind IO-wait account is to account the idle time that we could
> @@ -2909,31 +2928,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
>  
>  /*
> 

Both [1-2/2] applied with Peter's ACKs, thanks!


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-26  9:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-04 12:04 [PATCH V2 1/2] sched: Factor out nr_iowait and nr_iowait_cpu Daniel Lezcano
2018-10-04 12:04 ` Daniel Lezcano
2018-10-04 12:04 ` [PATCH V2 2/2] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier Daniel Lezcano
2018-10-04 12:04   ` Daniel Lezcano
2018-10-04 12:28   ` Mel Gorman
2018-10-04 15:23   ` Rafael J. Wysocki
2018-10-12  8:28   ` Peter Zijlstra
2018-10-12  8:28 ` [PATCH V2 1/2] sched: Factor out nr_iowait and nr_iowait_cpu Peter Zijlstra
2018-10-26  9:22 ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.