Re: [PATCH v10 00/12] sched, steal_governor: Introduce preferred CPUs and steal-driven vCPU backoff

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

Hi.

In addition to what's currently planned for v11 which was posted here,
https://lore.kernel.org/all/895a058a-475e-42ca-a7a3-2c854598eea4@linux.ibm.com/

I was going through sashiko's comments at:
https://sashiko.dev/#/patchset/20260812054033.95658-1-sshegde%40linux.ibm.com
This has revealed some gaps. Thanks to some really nice insights too.
Report quality improving day by day!


Vincent, Dietmar, please check the 32-bit task issue fix on ARM64.


On 8/12/26 11:10 AM, Shrikanth Hegde wrote:
> If you have already read v8,v9 cover-letter then see only revision
> changes. everything else is pretty much same. :)
> 

> v9->v10:
> - Introduce kcpustat_field_total helper. (Yury Norov)
> - Always do the design checks. This helps to avoid placing design
>    constraints in core hotplug code.
> - Remove cpu_preferred check in idle balancing. This helps to naturally
>    take care update of nohz.next_balance.
> - find_new_ilb changes are deferred as it isn't applicable for most
>    common use cases.
> - Move scheduler documentation to sched-paravirt.rst. (Yury Norov)
> - Add details of limitation of default values in documentation. (Yury Norov)
> - Remove task_can_sched_on_preferred out of sched.h (Mete Durlu)
> - Updated suggested-by tags for few patches. (I know i should have
>    done it earlier, sorry about that)
> - Minor polish of all changelogs.
> 



++++ Patch [1]: [PATCH v10 01/12] sched/cputime: Add kcpustat_field_total helper ++++
Issue:
======
>>  	int cpus, cpu;
>Does this code leave the local variable cpu unused?
>Since the for_each_cpu loop was replaced with a call to kcpustat_field_total,
>cpu does not appear to be referenced anymore in hd_calculate_steal_percentage.

Fix:
====
I did miss to notice it. Will fix it in v11.

index e5c7c818c178..c21496f0a141 100644
--- a/arch/s390/kernel/hiperdispatch.c
+++ b/arch/s390/kernel/hiperdispatch.c
@@ -207,7 +207,7 @@ static unsigned long hd_calculate_steal_percentage(void)
  {
  	unsigned long time_delta, steal_delta, steal, percentage;
  	static ktime_t prev;
-	int cpus, cpu;
+	int cpus;
  	ktime_t now;
  

++++  Patch [5]: [PATCH v10 05/12] sched/core: Try to use a preferred CPU in is_cpu_allowed ++++

Issue1: Possible crash on 32-bit tasks on ARM64.
=======
>> +static inline bool task_can_sched_on_preferred(int cpu, struct task_struct *p)
>> +{
>> +	if (cpu_preferred(cpu))
>> +		return false;
>> +
>> +	/* Only FAIR tasks honor preferred CPU state */
>> +	if (unlikely(p->sched_class != &fair_sched_class))
>> +		return false;
>> +
>> +	return cpumask_intersects(p->cpus_ptr, cpu_preferred_mask);
>> +}
>Does this intersection check need to account for the architectural CPU mask?
>On asymmetric systems, 32-bit tasks are architecturally restricted by
>task_cpu_possible_mask(). If a 32-bit task's mask intersects with
>64-bit-only preferred CPUs, this function might return true, causing
>is_cpu_allowed() to falsely return false for valid 32-bit non-preferred CPUs.
>Since 64-bit CPUs are rightfully rejected by task_allowed_on_cpu(), all CPUs
>end up rejected. Could this regression cause the select_fallback_rq() loop
>to exhaust all options and hit the BUG() case for 32-bit tasks?

Fix:
====
I wasn;t aware of this case, thanks to sashiko for bring it up.
Yes, it could potentially cause a BUG in select_fallback_rq.

Do a simple check if mask differ from possible mask which indicates we are on 32-bit task on 64 bit
kernel. Do the below. I think that should solve it.

  static inline bool task_can_sched_on_preferred(int cpu, struct task_struct *p)
  {
+	const struct cpumask *valid_mask;
+	int i;
[...]
+	valid_mask = task_cpu_possible_mask(p);
+	if (likely(valid_mask == cpu_possible_mask))
+		return cpumask_intersects(p->cpus_ptr, cpu_preferred_mask);
+
+	/* 32-bit task */
+	for_each_cpu_and(i, p->cpus_ptr, cpu_preferred_mask) {
+		if (cpumask_test_cpu(i, valid_mask))
+			return true;
+	}


Issue2:
=======
>How does this impact the migration stopper thread during sched_setaffinity?
>When sched_setaffinity updates p->cpus_ptr, it schedules a stopper thread
>to migrate the task. The destination CPU is selected without knowledge of the
>new preference logic in __set_cpus_allowed_ptr_locked():
>    dest_cpu = cpumask_any_and_distribute(cpu_valid_mask, ctx->new_mask);
>If this randomly chosen destination is a non-preferred CPU, __migrate_task()
>will observe is_cpu_allowed() returning false:
>kernel/sched/core.c:__migrate_task() {
>    ...
>    if (!is_cpu_allowed(p, dest_cpu))
>        return rq;
>    ...
>}
>Will this regression cause __migrate_task() to silently abort, leaving the
>task running outside its new explicitly allowed cpumask?

Fix:
====
This is a possible race with script which tries to change affinity at the same window
where preferred CPU mask has changed, but sched_tick hasn't run yet and new affinity
mask is spanning across both preferred and non-preferred CPUs and dest_cpu chosen by
cpumask_any_and_distribute is a non-preferred CPU.
So it is a very narrow case, but a possible one. This results in a
task which never gives up CPU, might run outside of its affinity.

Fix it by checking new affinity allows it run where it is currently running.
If not, don't make decision based on preferred CPU state.
cpus_ptr is updated before __migrate_task.

@@ -2503,7 +2506,20 @@ static inline bool task_can_sched_on_preferred(int cpu, struct task_struct *p)
  	if (unlikely(p->sched_class != &fair_sched_class))
  		return false;
  
+	if (unlikely(!cpumask_test_cpu(task_cpu(p), p->cpus_ptr)))
+		return false;


++++ Patch [7]: [PATCH v10 07/12] sched/core: Push current task from non preferred CPU ++++

Issue1:
>> +#ifdef CONFIG_PREFERRED_CPU
>> +static DEFINE_PER_CPU(struct cpu_stop_work, npc_push_task_work);
>> +
>> +static int sched_non_preferred_cpu_push_stop(void *arg)
>> +{
>> +	struct task_struct *p = arg;
>> +	struct rq *rq = this_rq();
>> +	struct rq_flags rf;
>> +	int cpu;
>> +
>> +	if (cpu_preferred(rq->cpu)) {
>> +		scoped_guard(rq_lock, rq)
>Does acquiring rq_lock here without disabling local interrupts create a
>deadlock window?
>The stopper thread executes this callback with preemption disabled but local
>interrupts enabled. If a timer interrupt fires while this lock is held, the
>interrupt handler will call sched_tick() which attempts to acquire the local
>rq->lock via rq_lock(rq, &rf), leading to an immediate self-deadlock.
>Should this early exit path use rq_lock_irq or rq_lock_irqsave instead?

Fix:
===
Yes a possible window exists. Disable irq when taking rq lock.

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b2c011824a7d..51cf0b40a7dd 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -11335,7 +11335,7 @@ static int sched_non_preferred_cpu_push_stop(void *arg)
         int cpu;
  
         if (cpu_preferred(rq->cpu)) {
-               scoped_guard(rq_lock, rq)
+               scoped_guard(rq_lock_irqsave, rq)
                         rq->push_task_work_done = false;
                 put_task_struct(p);
                 return 0;


Issue2:
======
>> +void sched_push_current_non_preferred_cpu(struct rq *rq)
>> +{
>> +	struct task_struct *push_task = rq->curr;
>> +
>> +	scoped_guard(rq_lock, rq) {
>> +		/* Push the task if its explicit affinity allows */
>> +		if (!task_can_sched_on_preferred(rq->cpu, push_task))
>> +			return;
>> +
>> +		/* There is already a stopper thread. Don't race with it. */
>> +		if (rq->push_task_work_done)
>> +			return;
>> +
>> +		rq->push_task_work_done = true;
>> +	}
>Could missing a check for is_migration_disabled(push_task) here cause a
>severe preemption storm?
>Since the stopper thread unconditionally queues without verifying if the task's
>migration is temporarily disabled, it will preempt the task. When the stopper
>observes !is_migration_disabled(p) is false, it resets rq->push_task_work_done
>and exits without migrating the task.
>On the very next timer tick, sched_tick() will observe the task is still on a
>non-preferred CPU and queue the stopper thread again. This continuous queueing
>of the highest-priority stopper thread every tick while migration is disabled
>defeats the purpose of the migration-disabled section.

Fix:
====
Do the same check before queuing the stopper too.
i.e. same check that happens in stopper.

@@ -11378,6 +11394,9 @@ void sched_push_current_non_preferred_cpu(struct rq *rq)
  		if (rq->push_task_work_done)
  			return;
  
+		if (is_migration_disabled(push_task))
+			return;
+
  		rq->push_task_work_done = true;



============================================================

Other comments which are worth noting, but are not a concern.

- time of use, time of check issue in select_fallback_rq w.r.t to preferred
   mask change. As explained in earlier changeset, this cannot happen since
   select_fallback_rq does two loop. First of nodemask, and then cpus_ptr.
   Lets due to concurrent mask change, first one fails, then by second loop, mask
   will be stable, and cannot race again. Mask updates by 100ms at least.

- Overloading of preferred CPUs. That is expected by design.

- Could the __read_mostly annotation on __cpu_preferred_mask cause cache line
   bouncing and false sharing? Kept as __read_mostly as majority of the time is
   isn't changing.

- Ping-pong doesn't happen since load balance doesn't push tasks onto preferred
   CPUs.

- Does triggering select_fallback_rq() on the hot wakeup path introduce a
   lock contention bottleneck? - yes but not too much, but adding more
   checks there, add more overhead in generic case.
   So it is optimization that is avoided at the moment.

- A non-preferred CPU isn't expected to pull any load and there is no load balancing
   among non-preferred CPUs as said in the changelog.