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 <[email protected]> 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. [ ... ] > @@ -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? 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: 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); > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
