linux-kernel - Re: [PATCH 2/3] sched/fair: don't set LBF_ALL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <jhjzh1m2j4k.mognet@arm.com>
Date:   Wed, 06 Jan 2021 15:13:47 +0000
From:   Valentin Schneider <valentin.schneider@....com>
To:     Vincent Guittot <vincent.guittot@...aro.org>, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, linux-kernel@...r.kernel.org
Cc:     Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH 2/3] sched/fair: don't set LBF_ALL_PINNED unnecessarily

On 06/01/21 14:34, Vincent Guittot wrote:
> Setting LBF_ALL_PINNED during active load balance is only valid when there
> is only 1 running task on the rq otherwise this ends up increasing the
> balance interval whereas other tasks could migrate after the next interval
> once they become cache-cold as an example.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>
> ---
>  kernel/sched/fair.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 5428b8723e61..69a455113b10 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9759,7 +9759,8 @@ static int load_balance(int this_cpu, struct rq *this_rq,
>                       if (!cpumask_test_cpu(this_cpu, busiest->curr->cpus_ptr)) {
>                               raw_spin_unlock_irqrestore(&busiest->lock,
>                                                           flags);
> -				env.flags |= LBF_ALL_PINNED;
> +				if (busiest->nr_running == 1)
> +					env.flags |= LBF_ALL_PINNED;

So LBF_ALL_PINNED *can* be set if busiest->nr_running > 1, because
before we get there we have:

  if (nr_running > 1) {
      env.flags |= LBF_ALL_PINNED;
      detach_tasks(&env); // Removes LBF_ALL_PINNED if > 0 tasks can be pulled
      ...
  }

What about following the logic used by detach_tasks() and only clear the
flag? Say something like the below. if nr_running > 1, then we'll have
gone through detach_tasks() and will have cleared the flag (if
possible).
---
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 04a3ce20da67..211c86ba3f5b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9623,6 +9623,8 @@ static int load_balance(int this_cpu, struct rq *this_rq,
 	env.src_rq = busiest;
 
 	ld_moved = 0;
+	/* Clear this as soon as we find a single pullable task */
+	env.flags |= LBF_ALL_PINNED;
 	if (busiest->nr_running > 1) {
 		/*
 		 * Attempt to move tasks. If find_busiest_group has found
@@ -9630,7 +9632,6 @@ static int load_balance(int this_cpu, struct rq *this_rq,
 		 * still unbalanced. ld_moved simply stays zero, so it is
 		 * correctly treated as an imbalance.
 		 */
-		env.flags |= LBF_ALL_PINNED;
 		env.loop_max  = min(sysctl_sched_nr_migrate, busiest->nr_running);
 
 more_balance:
@@ -9756,10 +9757,11 @@ static int load_balance(int this_cpu, struct rq *this_rq,
 			if (!cpumask_test_cpu(this_cpu, busiest->curr->cpus_ptr)) {
 				raw_spin_unlock_irqrestore(&busiest->lock,
 							    flags);
-				env.flags |= LBF_ALL_PINNED;
 				goto out_one_pinned;
 			}
 
+			env.flags &= ~LBF_ALL_PINNED;
+
 			/*
 			 * ->active_balance synchronizes accesses to
 			 * ->active_balance_work.  Once set, it's cleared
---