Re: [PATCH] sched/fair: do not scan twice in detach_tasks()

[Shijie Huang <shijie@amperemail.onmicrosoft.com>]

On 2025/7/16 1:04, Valentin Schneider wrote:
> 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?

I found this issue in my Specjbb2015 test.  It is very easy to trigger.

I noticed many times in the test.

I even found that:

         At the beginning: env->loop_max is 12.

        When the detach_tasks() scans: the real task list shrink to 5.


>> 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?

yes.  it maybe more then twice in theory.



>
>> Signed-off-by: Huang Shijie <shijie@os.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.
>                   */

Thanks, will use it in next version.


Thanks

Huang Shijie