[PATCH 0/7] sched: cleanup trigger_load_balance

[PATCH 0/7] sched: cleanup trigger_load_balance

[Daniel Lezcano]

This patchset does a cleanup on the parameters passed from the function
'trigger_load_balance' to the underneath functions.

The cpu is passed as parameter to the different functions as well as the struct
rq but this one contains already the cpu information. Moreover, in the call
stack for these functions, we have the struct rq retrieved from the cpu, and then
the cpu retrieve from the struct rq, etc ...

The patchset unifies all these functions to have a struct rq parameter and
removes the pointless parameters.

-static inline int find_new_ilb(int call_cpu)
+static inline int find_new_ilb(void)

-static void nohz_balancer_kick(int cpu)
+static void nohz_balancer_kick(void)

-static void rebalance_domains(int cpu, enum cpu_idle_type idle)
+static void rebalance_domains(struct rq *rq, enum cpu_idle_type idle)

-static void nohz_idle_balance(int this_cpu, enum cpu_idle_type idle)
+static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)

-static inline int nohz_kick_needed(struct rq *rq, int cpu)
+static inline int nohz_kick_needed(struct rq *rq)

-static inline int on_null_domain(int cpu)
+static inline int on_null_domain(struct rq *rq)

Daniel Lezcano (7):
  sched: reduce nohz_kick_needed parameters
  sched: pass struct rq to on_null_domain function
  sched: remove unused parameter for find_new_ilb
  sched: remove unused parameter in nohz_balancer_kick function
  sched: pass struct rq to rebalance_domains function
  sched: pass struct rq to nohz_idle_balance function
  sched: factor out on_null_domain check in trigger_load_balance
    function

 kernel/sched/fair.c |   45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[PATCH 1/7] sched: reduce nohz_kick_needed parameters

[Daniel Lezcano]

The cpu information is already stored in the struct rq, so no need to pass it
as parameter to the nohz_kick_needed function.

The caller of this function just called idle_cpu() before to fill the
rq->idle_balance field.

Use rq->cpu and rq->idle_balance.

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

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 600301c..4cb414a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6781,14 +6781,14 @@ end:
  *   - For SD_ASYM_PACKING, if the lower numbered cpu's in the scheduler
  *     domain span are idle.
  */
-static inline int nohz_kick_needed(struct rq *rq, int cpu)
+static inline int nohz_kick_needed(struct rq *rq)
 {
 	unsigned long now = jiffies;
 	struct sched_domain *sd;
 	struct sched_group_power *sgp;
-	int nr_busy;
+	int nr_busy, cpu = rq->cpu;
 
-	if (unlikely(idle_cpu(cpu)))
+	if (unlikely(rq->idle_balance))
 		return 0;
 
        /*
@@ -6878,7 +6878,7 @@ void trigger_load_balance(struct rq *rq)
 	    likely(!on_null_domain(cpu)))
 		raise_softirq(SCHED_SOFTIRQ);
 #ifdef CONFIG_NO_HZ_COMMON
-	if (nohz_kick_needed(rq, cpu) && likely(!on_null_domain(cpu)))
+	if (nohz_kick_needed(rq) && likely(!on_null_domain(cpu)))
 		nohz_balancer_kick(cpu);
 #endif
 }
-- 
1.7.9.5

[PATCH 2/7] sched: pass struct rq to on_null_domain function

[Daniel Lezcano]

The on_null_domain function is getting the cpu to retrieve the struct rq
associated with it.

Pass the struct rq directly to the function as the caller has already the info.

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

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4cb414a..82dd145 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6861,9 +6861,9 @@ static void run_rebalance_domains(struct softirq_action *h)
 	nohz_idle_balance(this_cpu, idle);
 }
 
-static inline int on_null_domain(int cpu)
+static inline int on_null_domain(struct rq *rq)
 {
-	return !rcu_dereference_sched(cpu_rq(cpu)->sd);
+	return !rcu_dereference_sched(rq->sd);
 }
 
 /*
@@ -6875,10 +6875,10 @@ void trigger_load_balance(struct rq *rq)
 
 	/* Don't need to rebalance while attached to NULL domain */
 	if (time_after_eq(jiffies, rq->next_balance) &&
-	    likely(!on_null_domain(cpu)))
+	    likely(!on_null_domain(rq)))
 		raise_softirq(SCHED_SOFTIRQ);
 #ifdef CONFIG_NO_HZ_COMMON
-	if (nohz_kick_needed(rq) && likely(!on_null_domain(cpu)))
+	if (nohz_kick_needed(rq) && likely(!on_null_domain(rq)))
 		nohz_balancer_kick(cpu);
 #endif
 }
-- 
1.7.9.5

[PATCH 3/7] sched: remove unused parameter for find_new_ilb

[Daniel Lezcano]

The 'call_cpu' is never used in the function. Remove it.

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

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 82dd145..1f7ed1a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6502,7 +6502,7 @@ static struct {
 	unsigned long next_balance;     /* in jiffy units */
 } nohz ____cacheline_aligned;
 
-static inline int find_new_ilb(int call_cpu)
+static inline int find_new_ilb(void)
 {
 	int ilb = cpumask_first(nohz.idle_cpus_mask);
 
@@ -6523,7 +6523,7 @@ static void nohz_balancer_kick(int cpu)
 
 	nohz.next_balance++;
 
-	ilb_cpu = find_new_ilb(cpu);
+	ilb_cpu = find_new_ilb();
 
 	if (ilb_cpu >= nr_cpu_ids)
 		return;
-- 
1.7.9.5

[PATCH 4/7] sched: remove unused parameter in nohz_balancer_kick function

[Daniel Lezcano]

The cpu parameter is no longer needed in nohz_balancer_kick, let's remove
the parameter.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/fair.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1f7ed1a..c647c45 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6517,7 +6517,7 @@ static inline int find_new_ilb(void)
  * nohz_load_balancer CPU (if there is one) otherwise fallback to any idle
  * CPU (if there is one).
  */
-static void nohz_balancer_kick(int cpu)
+static void nohz_balancer_kick(void)
 {
 	int ilb_cpu;
 
@@ -6871,15 +6871,13 @@ static inline int on_null_domain(struct rq *rq)
  */
 void trigger_load_balance(struct rq *rq)
 {
-	int cpu = rq->cpu;
-
 	/* Don't need to rebalance while attached to NULL domain */
 	if (time_after_eq(jiffies, rq->next_balance) &&
 	    likely(!on_null_domain(rq)))
 		raise_softirq(SCHED_SOFTIRQ);
 #ifdef CONFIG_NO_HZ_COMMON
 	if (nohz_kick_needed(rq) && likely(!on_null_domain(rq)))
-		nohz_balancer_kick(cpu);
+		nohz_balancer_kick();
 #endif
 }
 
-- 
1.7.9.5

[PATCH 5/7] sched: pass struct rq to rebalance_domains function

[Daniel Lezcano]

The cpu information is stored in the struct rq and the caller of the
rebalance_domains function pass the cpu to retrieve the struct rq but
it already has the struct rq info. Replace the cpu parameter with the
struct rq.

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

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index c647c45..0fbd8d0 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6633,10 +6633,10 @@ void update_max_interval(void)
  *
  * Balancing parameters are set up in init_sched_domains.
  */
-static void rebalance_domains(int cpu, enum cpu_idle_type idle)
+static void rebalance_domains(struct rq *rq, enum cpu_idle_type idle)
 {
 	int continue_balancing = 1;
-	struct rq *rq = cpu_rq(cpu);
+	int cpu = rq->cpu;
 	unsigned long interval;
 	struct sched_domain *sd;
 	/* Earliest time when we have to do rebalance again */
@@ -6762,7 +6762,7 @@ static void nohz_idle_balance(int this_cpu, enum cpu_idle_type idle)
 		update_idle_cpu_load(rq);
 		raw_spin_unlock_irq(&rq->lock);
 
-		rebalance_domains(balance_cpu, CPU_IDLE);
+		rebalance_domains(rq, CPU_IDLE);
 
 		if (time_after(this_rq->next_balance, rq->next_balance))
 			this_rq->next_balance = rq->next_balance;
@@ -6851,7 +6851,7 @@ static void run_rebalance_domains(struct softirq_action *h)
 	enum cpu_idle_type idle = this_rq->idle_balance ?
 						CPU_IDLE : CPU_NOT_IDLE;
 
-	rebalance_domains(this_cpu, idle);
+	rebalance_domains(this_rq, idle);
 
 	/*
 	 * If this cpu has a pending nohz_balance_kick, then do the
-- 
1.7.9.5

[PATCH 6/7] sched: pass struct rq to nohz_idle_balance function

[Daniel Lezcano]

The cpu information is stored in the struct rq. Pass the struct rq to
nohz_idle_balance, so all the functions called in run_rebalance_domains have
the same parameters and the 'this_cpu' variable becomes pointless.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/fair.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0fbd8d0..59c57e7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6733,9 +6733,9 @@ out:
  * In CONFIG_NO_HZ_COMMON case, the idle balance kickee will do the
  * rebalancing for all the cpus for whom scheduler ticks are stopped.
  */
-static void nohz_idle_balance(int this_cpu, enum cpu_idle_type idle)
+static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
 {
-	struct rq *this_rq = cpu_rq(this_cpu);
+	int this_cpu = this_rq->cpu;
 	struct rq *rq;
 	int balance_cpu;
 
@@ -6846,8 +6846,7 @@ static void nohz_idle_balance(int this_cpu, enum cpu_idle_type idle) { }
  */
 static void run_rebalance_domains(struct softirq_action *h)
 {
-	int this_cpu = smp_processor_id();
-	struct rq *this_rq = cpu_rq(this_cpu);
+	struct rq *this_rq = this_rq();
 	enum cpu_idle_type idle = this_rq->idle_balance ?
 						CPU_IDLE : CPU_NOT_IDLE;
 
@@ -6858,7 +6857,7 @@ static void run_rebalance_domains(struct softirq_action *h)
 	 * balancing on behalf of the other idle cpus whose ticks are
 	 * stopped.
 	 */
-	nohz_idle_balance(this_cpu, idle);
+	nohz_idle_balance(this_rq, idle);
 }
 
 static inline int on_null_domain(struct rq *rq)
-- 
1.7.9.5

[PATCH 7/7] sched: factor out on_null_domain check in trigger_load_balance function

[Daniel Lezcano]

The test on_null_domain is done twice in the trigger_load_balance function.

Move the test at the begin of the function, so there is only one check.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/fair.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 59c57e7..ef95ddf 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6871,11 +6871,13 @@ static inline int on_null_domain(struct rq *rq)
 void trigger_load_balance(struct rq *rq)
 {
 	/* Don't need to rebalance while attached to NULL domain */
-	if (time_after_eq(jiffies, rq->next_balance) &&
-	    likely(!on_null_domain(rq)))
+	if (unlikely(on_null_domain(rq)))
+		return;
+
+	if (time_after_eq(jiffies, rq->next_balance))
 		raise_softirq(SCHED_SOFTIRQ);
 #ifdef CONFIG_NO_HZ_COMMON
-	if (nohz_kick_needed(rq) && likely(!on_null_domain(rq)))
+	if (nohz_kick_needed(rq))
 		nohz_balancer_kick();
 #endif
 }
-- 
1.7.9.5

Re: [PATCH 4/7] sched: remove unused parameter in nohz_balancer_kick function

[Daniel Lezcano]

On 12/31/2013 12:17 PM, Preeti Murthy wrote:
> Hi Daniel,
>
> This patch looks correct to me.
> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com

Thanks Preeti for the review.

   -- Daniel

> <mailto:preeti@linux.vnet.ibm.com>>
>
>
> On Mon, Dec 30, 2013 at 7:14 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org <mailto:daniel.lezcano@linaro.org>> wrote:
>
>     The cpu parameter is no longer needed in nohz_balancer_kick, let's
>     remove
>     the parameter.
>
>     Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org
>     <mailto:daniel.lezcano@linaro.org>>
>     ---
>       kernel/sched/fair.c |    6 ++----
>       1 file changed, 2 insertions(+), 4 deletions(-)
>
>     diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>     index 1f7ed1a..c647c45 100644
>     --- a/kernel/sched/fair.c
>     +++ b/kernel/sched/fair.c
>     @@ -6517,7 +6517,7 @@ static inline int find_new_ilb(void)
>        * nohz_load_balancer CPU (if there is one) otherwise fallback to
>     any idle
>        * CPU (if there is one).
>        */
>     -static void nohz_balancer_kick(int cpu)
>     +static void nohz_balancer_kick(void)
>       {
>              int ilb_cpu;
>
>     @@ -6871,15 +6871,13 @@ static inline int on_null_domain(struct rq *rq)
>        */
>       void trigger_load_balance(struct rq *rq)
>       {
>     -       int cpu = rq->cpu;
>     -
>              /* Don't need to rebalance while attached to NULL domain */
>              if (time_after_eq(jiffies, rq->next_balance) &&
>                  likely(!on_null_domain(rq)))
>                      raise_softirq(SCHED_SOFTIRQ);
>       #ifdef CONFIG_NO_HZ_COMMON
>              if (nohz_kick_needed(rq) && likely(!on_null_domain(rq)))
>     -               nohz_balancer_kick(cpu);
>     +               nohz_balancer_kick();
>       #endif
>       }
>
>     --
>     1.7.9.5
>
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-kernel" in
>     the body of a message to majordomo@vger.kernel.org
>     <mailto:majordomo@vger.kernel.org>
>     More majordomo info at http://vger.kernel.org/majordomo-info.html
>     Please read the FAQ at http://www.tux.org/lkml/
>
>


-- 
  <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 0/7] sched: cleanup trigger_load_balance

[Peter Zijlstra]

On Mon, Dec 30, 2013 at 02:44:46PM +0100, Daniel Lezcano wrote:
> This patchset does a cleanup on the parameters passed from the function
> 'trigger_load_balance' to the underneath functions.

Doesn't actually apply right.. will maybe spend more time next week :-)

Re: [PATCH 0/7] sched: cleanup trigger_load_balance

[Daniel Lezcano]

On 12/31/2013 05:28 PM, Peter Zijlstra wrote:
> On Mon, Dec 30, 2013 at 02:44:46PM +0100, Daniel Lezcano wrote:
>> This patchset does a cleanup on the parameters passed from the function
>> 'trigger_load_balance' to the underneath functions.
>
> Doesn't actually apply right.. will maybe spend more time next week :-)

I understand. Happy new year !

   -- Daniel

ps : the patchset is based on tip/sched/core


-- 
  <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 0/7] sched: cleanup trigger_load_balance

[Peter Zijlstra]

On Tue, Dec 31, 2013 at 06:19:04PM +0100, Daniel Lezcano wrote:
> I understand. Happy new year !

Happy new year to you too! :-)

> ps : the patchset is based on tip/sched/core

Weird; because tip/sched/core as per today looks like:


029632fbb7b7c kernel/sched_fair.c (Peter Zijlstra       2011-10-25 10:00:11 +0200 6872) void trigger_load_balance(struct rq *rq, int cpu)

And your patches assume it looks like:

Patch 1: @@ -6878,7 +6878,7 @@ void trigger_load_balance(struct rq *rq)
Patch 2: @@ -6875,10 +6875,10 @@ void trigger_load_balance(struct rq *rq)
Patch 4:
 void trigger_load_balance(struct rq *rq)
 {
-       int cpu = rq->cpu;
-

Which obviously doesn't quite work..


So I can make it fit.. but I do wonder what I'm missing here..

Re: [PATCH 0/7] sched: cleanup trigger_load_balance

[Daniel Lezcano]

On 01/06/2014 12:03 PM, Peter Zijlstra wrote:
> On Tue, Dec 31, 2013 at 06:19:04PM +0100, Daniel Lezcano wrote:
>> I understand. Happy new year !
>
> Happy new year to you too! :-)
>
>> ps : the patchset is based on tip/sched/core
>
> Weird; because tip/sched/core as per today looks like:
>
>
> 029632fbb7b7c kernel/sched_fair.c (Peter Zijlstra       2011-10-25 10:00:11 +0200 6872) void trigger_load_balance(struct rq *rq, int cpu)
>
> And your patches assume it looks like:
>
> Patch 1: @@ -6878,7 +6878,7 @@ void trigger_load_balance(struct rq *rq)
> Patch 2: @@ -6875,10 +6875,10 @@ void trigger_load_balance(struct rq *rq)
> Patch 4:
>   void trigger_load_balance(struct rq *rq)
>   {
> -       int cpu = rq->cpu;
> -
>
> Which obviously doesn't quite work..
>
>
> So I can make it fit.. but I do wonder what I'm missing here..


Ah, ok. I got it. I missed to send the first patch of my patchset:

sched: reduce trigger_load_balance parameters

Let me resend the patchset with the missing patch.

Sorry for the inconvenience

   -- Daniel



-- 
  <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 0/7] sched: cleanup trigger_load_balance

[Peter Zijlstra]

On Mon, Jan 06, 2014 at 12:25:55PM +0100, Daniel Lezcano wrote:
> Ah, ok. I got it. I missed to send the first patch of my patchset:

:-)