Re: [PATCH v14 06/13] sched/core: Try to use a preferred CPU in is_cpu_allowed

[Shrikanth Hegde <sshegde@linux.ibm.com>]

I believe most of the comments have been addressed in the previous rounds.

On 9/28/26 11:30 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] sched/core: O(N^2) wakeup latency and task clumping in fallback path
> - [High] sched/fair: Active load balancing to non-preferred CPUs silently fails
> - [High] sched/core: TOCTOU race with CPU hotplug breaks task affinity
> --
> 
> commit dfa0f159ec643e02807bc453bbfcc49c9f1e7915
> Author: Shrikanth Hegde <sshegde@linux.ibm.com>
> 
> sched/core: Try to use a preferred CPU in is_cpu_allowed
> 
> This commit modifies the scheduler to preferentially select preferred CPUs
> when checking if a task is allowed to run on a given CPU. It introduces a
> helper function to check if a task can migrate to a preferred CPU based on
> its affinity mask.
> 
>> For the majority of cases, this would still keep select_fallback_rq()
>> as O(N). cpumask_intersects_and(), which is O(N), is called only if
>> !cpu_preferred. The task running there is expected to move out.
>> Subsequently, it should run on a preferred CPU. This becomes O(N**2)
>> only for tasks pinned solely to non-preferred CPUs. That is a rare case.
> 
> Is this description accurate?
> 
> Because the fallback loop iterates over the affinity mask sequentially, if
> the allowed preferred CPUs happen to have higher indices than the
> non-preferred CPUs, the loop will perform the intersection operation for
> every non-preferred CPU it checks. This means any task with preferred CPUs
> at higher indices could experience the quadratic latency overhead, rather
> than only tasks pinned solely to non-preferred CPUs.
> 
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 0183c75d7d518..04400f934cc72 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
> [ ... ]
>> @@ -2504,6 +2504,24 @@ static inline bool rq_has_pinned_tasks(struct rq *rq)
>>   	return rq->nr_pinned;
>>   }
>>   
>> +static inline bool task_can_migrate_to_preferred(struct task_struct *p, int cpu)
>> +{
>> +	/* No need to migrate from a preferred CPU */
>> +	if (cpu_preferred(cpu))
>> +		return false;
>> +
>> +	/* Only FAIR tasks honor preferred CPU state */
>> +	if (unlikely(p->sched_class != &fair_sched_class))
>> +		return false;
>> +
>> +	/* Ignore preferred state if task affinity is changing */
>> +	if (unlikely(!cpumask_test_cpu(task_cpu(p), p->cpus_ptr)))
>> +		return false;
>> +
>> +	return cpumask_intersects_and(p->cpus_ptr, cpu_preferred_mask,
>> +				      task_cpu_possible_mask(p));
>> +}
> 
> [Severity: High]
> Does this intersection operation lead to quadratic latency during wakeups
> when combined with the fallback loop?
> 
> When select_task_rq() in kernel/sched/core.c picks a non-preferred CPU
> because it is idle, is_cpu_allowed() will reject it, forcing the task into
> select_fallback_rq(). The fallback loop iterates sequentially over all CPUs:
> 
> kernel/sched/core.c:select_fallback_rq() {
>      ...
>      for_each_cpu(dest_cpu, p->cpus_ptr) {
>          if (!is_cpu_allowed(p, dest_cpu))
>              continue;
>          ...
>          goto out;
>      }
>      ...
> }
> 
> If the allowed preferred CPUs are at the end of the cpumask, wouldn't this
> perform an intersection for every non-preferred CPU it checks, leading to
> severe scheduling latency? Additionally, wouldn't this deterministic loop
> always dump the task on the first preferred CPU it finds, destroying load
> distribution by clumping tasks on the lowest-ID preferred CPU?
> 
> This performance degradation from wakeups on non-preferred CPUs does not
> appear to be resolved in the final state of the patch series.

It is mentioned in the changelog and this is true only when wakeup happens on
a non-preferred CPUs. Even if it runs there is meant to pushed out. So take a
bit more cycles and do the right thing.

> 
> [ ... ]
>> @@ -2519,8 +2537,12 @@ static inline bool is_cpu_allowed(struct task_struct *p, int cpu)
>>   		return cpu_online(cpu);
>>   
>>   	/* Non kernel threads are not allowed during either online or offline. */
>> -	if (!(p->flags & PF_KTHREAD))
>> +	if (!(p->flags & PF_KTHREAD)) {
>> +		/* Try to use preferred CPU if task's affinity allows */
>> +		if (task_can_migrate_to_preferred(p, cpu))
>> +			return false;
> 
> [Severity: High]
> Could returning false for non-preferred CPUs here cause active load
> balancing to silently fail?

Spreading load to non-preferred CPUs is by design and expected.

> 
> During routine background load balancing, if the system attempts to pull
> load from a preferred CPU to an idle non-preferred CPU, the stopper thread
> will run __migrate_task().
> 
> kernel/sched/core.c:__migrate_task() {
>      ...
>      if (!is_cpu_allowed(p, dest_cpu))
>          return rq;
>      ...
> }
> 
> Since the destination is non-preferred, won't is_cpu_allowed() reject it,
> causing the stopper thread to silently abort the migration and break load
> distribution?
> 
> While follow-up patches in the series restrict the busiest CPU to preferred
> CPUs, they do not appear to prevent the destination from being non-preferred,
> leaving this active load balancing path broken.
> 
> [Severity: High]
> Can this rejection introduce a race condition with CPU hotplug that breaks
> a task's user-configured affinity?
> 
> In select_fallback_rq() in kernel/sched/core.c, the loop might skip an
> active, non-preferred CPU because task_can_migrate_to_preferred() indicates
> another preferred CPU is available in the affinity mask. If a concurrent
> CPU hotplug operation removes that preferred CPU from the active mask before
> the loop reaches it, the loop will skip it too:

There is two iterations and steal governor's minimal interval is 100ms.
So it is likely safe.

> 
> kernel/sched/core.c:select_fallback_rq() {
>      ...
>      for_each_cpu(dest_cpu, p->cpus_ptr) {
>          if (!is_cpu_allowed(p, dest_cpu))
>              continue;
>      ...
> }
> 
> Could the loop then terminate without finding any allowed CPU, permanently
> forcing the task outside of its allowed affinity?
> 
>>   		return cpu_active(cpu);
>> +	}
>