linux-kernel - Re: [PATCH] sched/fair: do not scan twice in detach

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: <xhsmho6tl1j2d.mognet@vschneid-thinkpadt14sgen2i.remote.csb>
Date: Tue, 15 Jul 2025 19:04:58 +0200
From: Valentin Schneider <vschneid@...hat.com>
To: Huang Shijie <shijie@...amperecomputing.com>, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org
Cc: patches@...erecomputing.com, cl@...ux.com,
 Shubhang@...amperecomputing.com, dietmar.eggemann@....com,
 rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
 linux-kernel@...r.kernel.org, Huang Shijie <shijie@...amperecomputing.com>
Subject: Re: [PATCH] sched/fair: do not scan twice in detach_tasks()

On 07/07/25 16:36, Huang Shijie wrote:
> When detach_tasks() scans the src_cpu's task list, the task list
> may shrink during the scanning. For example, the task list
> may have four tasks at the beginning, it may becomes to two
> during the scanning in detach_tasks():
>     Task list at beginning : "ABCD"
>     Task list in scanning  : "CD"
>
>     (ABCD stands for differnt tasks.)
>
> In this scenario, the env->loop_max is still four, so
> detach_tasks() may scan twice for some tasks:
>     the scanning order maybe : "DCDC"
>


Huh, a quick hacky test suggests this isn't /too/ hard to trigger; I get
about one occurrence every two default hackbench run (~200ms) on my dummy
QEMU setup.

Have you seen this happen on your workloads or did you find this while
staring at the code?

> The patch introduces "first_back" to record the first task which
> is put back to the task list. If we get a task which is equal to
> first_back, we break the loop, and avoid to scan twice for it.
>

Potentially more than twice, no?

> Signed-off-by: Huang Shijie <shijie@...amperecomputing.com>
> ---
>  kernel/sched/fair.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7e2963efe800..0e9c8ae68cc2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9443,6 +9443,7 @@ static int detach_tasks(struct lb_env *env)
>  {
>       struct list_head *tasks = &env->src_rq->cfs_tasks;
>       unsigned long util, load;
> +	struct task_struct *first_back = NULL;
>       struct task_struct *p;
>       int detached = 0;
>
> @@ -9481,6 +9482,8 @@ static int detach_tasks(struct lb_env *env)
>               }
>
>               p = list_last_entry(tasks, struct task_struct, se.group_node);

Small comment nit:

                /*
                 * We're back to an already visited task that couldn't be
                 * detached, we've seen all there is to see.
                 */

> +		if (p == first_back)
> +			break;
>
>               if (!can_migrate_task(p, env))
>                       goto next;
> @@ -9562,6 +9565,8 @@ static int detach_tasks(struct lb_env *env)
>                       schedstat_inc(p->stats.nr_failed_migrations_hot);
>
>               list_move(&p->se.group_node, tasks);
> +		if (!first_back)
> +			first_back = p;
>       }
>
>       /*
> --
> 2.40.1