[PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Adam Li]

Load imbalance is observed when the workload frequently forks new threads.
Due to CPU affinity, the workload can run on CPU 0-7 in the first
group, and only on CPU 8-11 in the second group. CPU 12-15 are always idle.

{ 0 1 2 3 4 5 6 7 } {8 9 10 11 12 13 14 15}
  * * * * * * * *    * * *  *

When looking for dst group for newly forked threads, in many times
update_sg_wakeup_stats() reports the second group has more idle CPUs
than the first group. The scheduler thinks the second group is less
busy. Then it selects least busy CPUs among CPU 8-11. Therefore CPU 8-11
can be crowded with newly forked threads, at the same time CPU 0-7
can be idle.

A task may not use all the CPUs in a schedule group due to CPU affinity.
Only update schedule group statistics for allowed CPUs.

Signed-off-by: Adam Li <adamli@os.amperecomputing.com>
---
Resending this patch from the patchset:
https://lore.kernel.org/lkml/20250717062036.432243-2-adamli@os.amperecomputing.com/

Only changed commit message. The single patch may be easier for reviewing.
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index bc0b7ce8a65d..d5ec15050ebc 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10671,7 +10671,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
 	if (sd->flags & SD_ASYM_CPUCAPACITY)
 		sgs->group_misfit_task_load = 1;
 
-	for_each_cpu(i, sched_group_span(group)) {
+	for_each_cpu_and(i, sched_group_span(group), p->cpus_ptr) {
 		struct rq *rq = cpu_rq(i);
 		unsigned int local;
 
-- 
2.34.1

Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Chen, Yu C]

On 10/11/2025 2:43 PM, Adam Li wrote:
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index bc0b7ce8a65d..d5ec15050ebc 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10671,7 +10671,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>   	if (sd->flags & SD_ASYM_CPUCAPACITY)
>   		sgs->group_misfit_task_load = 1;
>   
> -	for_each_cpu(i, sched_group_span(group)) {
> +	for_each_cpu_and(i, sched_group_span(group), p->cpus_ptr) {

Looks good to me. One minor question, would pre-calculating the mask be 
better?
Copied from select_idle_cpu():

cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
cpumask_and(cpus, sched_group_span(sd), p->cpus_ptr);
for_each_cpu(i, cpus) {

thanks,
Chenyu

Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Adam Li]

Hi Chenyu,

Thanks for your comments.
On 10/12/2025 1:42 AM, Chen, Yu C wrote:
> On 10/11/2025 2:43 PM, Adam Li wrote:
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index bc0b7ce8a65d..d5ec15050ebc 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -10671,7 +10671,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>>       if (sd->flags & SD_ASYM_CPUCAPACITY)
>>           sgs->group_misfit_task_load = 1;
>>   -    for_each_cpu(i, sched_group_span(group)) {
>> +    for_each_cpu_and(i, sched_group_span(group), p->cpus_ptr) {
> 
> Looks good to me. One minor question, would pre-calculating the mask be better?

I do agree pre-calculating the cpumask can save cpu cycles, without
doing mask AND at each loop.

> Copied from select_idle_cpu():
> 
> cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
> cpumask_and(cpus, sched_group_span(sd), p->cpus_ptr);
> for_each_cpu(i, cpus) {
> 
But I am not sure if it is safe to use the percpu 'select_rq_mask'
in update_sg_wakeup_stats(). Or we have to allocate a 'struct cpumask'.

I tested bellow patch. It can work and fix the bug.
If it is safe to use 'select_rq_mask' , I can submit V2 patch.

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10664,6 +10664,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
                                          struct task_struct *p)
 {
        int i, nr_running;
+       struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);

        memset(sgs, 0, sizeof(*sgs));

@@ -10671,7 +10672,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
        if (sd->flags & SD_ASYM_CPUCAPACITY)
                sgs->group_misfit_task_load = 1;

-       for_each_cpu(i, sched_group_span(group)) {
+       cpumask_and(cpus, sched_group_span(group), p->cpus_ptr);
+       for_each_cpu(i, cpus) {
                struct rq *rq = cpu_rq(i);
                unsigned int local;


Thanks,
-adam

Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Peter Zijlstra]

On Sat, Oct 11, 2025 at 06:43:22AM +0000, Adam Li wrote:
> Load imbalance is observed when the workload frequently forks new threads.
> Due to CPU affinity, the workload can run on CPU 0-7 in the first
> group, and only on CPU 8-11 in the second group. CPU 12-15 are always idle.
> 
> { 0 1 2 3 4 5 6 7 } {8 9 10 11 12 13 14 15}
>   * * * * * * * *    * * *  *
> 
> When looking for dst group for newly forked threads, in many times
> update_sg_wakeup_stats() reports the second group has more idle CPUs
> than the first group. The scheduler thinks the second group is less
> busy. Then it selects least busy CPUs among CPU 8-11. Therefore CPU 8-11
> can be crowded with newly forked threads, at the same time CPU 0-7
> can be idle.
> 
> A task may not use all the CPUs in a schedule group due to CPU affinity.
> Only update schedule group statistics for allowed CPUs.
> 
> Signed-off-by: Adam Li <adamli@os.amperecomputing.com>
> ---
> Resending this patch from the patchset:
> https://lore.kernel.org/lkml/20250717062036.432243-2-adamli@os.amperecomputing.com/
> 

Right, lets start with this then ;-)

No need to do the cpumask_and() thing, that's just more changes vs
update_sg_lb_stats().

Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Chen, Yu C]

On 10/14/2025 6:51 PM, Adam Li wrote:
> Hi Chenyu,
> 
> Thanks for your comments.
> On 10/12/2025 1:42 AM, Chen, Yu C wrote:
>> On 10/11/2025 2:43 PM, Adam Li wrote:
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index bc0b7ce8a65d..d5ec15050ebc 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -10671,7 +10671,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>>>        if (sd->flags & SD_ASYM_CPUCAPACITY)
>>>            sgs->group_misfit_task_load = 1;
>>>    -    for_each_cpu(i, sched_group_span(group)) {
>>> +    for_each_cpu_and(i, sched_group_span(group), p->cpus_ptr) {
>>
>> Looks good to me. One minor question, would pre-calculating the mask be better?
> 
> I do agree pre-calculating the cpumask can save cpu cycles, without
> doing mask AND at each loop.
> 
>> Copied from select_idle_cpu():
>>
>> cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
>> cpumask_and(cpus, sched_group_span(sd), p->cpus_ptr);
>> for_each_cpu(i, cpus) {
>>
> But I am not sure if it is safe to use the percpu 'select_rq_mask'
> in update_sg_wakeup_stats(). Or we have to allocate a 'struct cpumask'.
> 

Allocating dynamically would be costly. Using percpu select_rq_mask is
safe in this scenario: the waker's CPU has already disabled local irq
via raw_spinlock_irqsave(&p->pi_lock), so I suppose no one can modify
it simultaneously. Moreover, if the fast wakeup path select_idle_sibling()
  can use it, the slow path sched_balance_find_dst_cpu() should also be able
to do so IMO.

> I tested bellow patch. It can work and fix the bug.
> If it is safe to use 'select_rq_mask' , I can submit V2 patch.
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10664,6 +10664,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>                                            struct task_struct *p)
>   {
>          int i, nr_running;
> +       struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
> 
>          memset(sgs, 0, sizeof(*sgs));
> 
> @@ -10671,7 +10672,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>          if (sd->flags & SD_ASYM_CPUCAPACITY)
>                  sgs->group_misfit_task_load = 1;
> 
> -       for_each_cpu(i, sched_group_span(group)) {

nice-to-have:
maybe add a comment here that cpus is not empty, because
we have cpumask_intersects() check in sched_balance_find_dst_group(),
(just in case sgs->group_type incorrectly remain 0 which is 
group_has_spare, if
the cpus is empty)

> +       cpumask_and(cpus, sched_group_span(group), p->cpus_ptr);
> +       for_each_cpu(i, cpus) {
>                  struct rq *rq = cpu_rq(i);
>                  unsigned int local;
> 
> 

and from my understanding, for this percpu version,

Reviewed-by: Chen Yu <yu.c.chen@intel.com>

thanks,
Chenyu

Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Chen Yu]

On 10/14/25 19:37, Peter Zijlstra wrote:
> On Sat, Oct 11, 2025 at 06:43:22AM +0000, Adam Li wrote:
>> Load imbalance is observed when the workload frequently forks new threads.
>> Due to CPU affinity, the workload can run on CPU 0-7 in the first
>> group, and only on CPU 8-11 in the second group. CPU 12-15 are always idle.
>>
>> { 0 1 2 3 4 5 6 7 } {8 9 10 11 12 13 14 15}
>>    * * * * * * * *    * * *  *
>>
>> When looking for dst group for newly forked threads, in many times
>> update_sg_wakeup_stats() reports the second group has more idle CPUs
>> than the first group. The scheduler thinks the second group is less
>> busy. Then it selects least busy CPUs among CPU 8-11. Therefore CPU 8-11
>> can be crowded with newly forked threads, at the same time CPU 0-7
>> can be idle.
>>
>> A task may not use all the CPUs in a schedule group due to CPU affinity.
>> Only update schedule group statistics for allowed CPUs.
>>
>> Signed-off-by: Adam Li <adamli@os.amperecomputing.com>
>> ---
>> Resending this patch from the patchset:
>> https://lore.kernel.org/lkml/20250717062036.432243-2-adamli@os.amperecomputing.com/
>>
> 
> Right, lets start with this then ;-)
> 
> No need to do the cpumask_and() thing, that's just more changes vs
> update_sg_lb_stats().
> 


I just saw Peter's comments on this.  I'm OK with the current
version and Adam please feel free to keep my Reviewed-by tag
for this non-cpumask_and version.

thanks,
Chenyu

Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs when looking for dst group

[Adam Li]

On 10/14/2025 8:07 PM, Chen, Yu C wrote:
> On 10/14/2025 6:51 PM, Adam Li wrote:
[...]
>>>
>> But I am not sure if it is safe to use the percpu 'select_rq_mask'
>> in update_sg_wakeup_stats(). Or we have to allocate a 'struct cpumask'.
>>
> 
> Allocating dynamically would be costly. Using percpu select_rq_mask is
> safe in this scenario: the waker's CPU has already disabled local irq
> via raw_spinlock_irqsave(&p->pi_lock), so I suppose no one can modify
> it simultaneously. Moreover, if the fast wakeup path select_idle_sibling()
>  can use it, the slow path sched_balance_find_dst_cpu() should also be able
> to do so IMO.
> 

Yes. Agree.>> I tested bellow patch. It can work and fix the bug.
>> If it is safe to use 'select_rq_mask' , I can submit V2 patch.
>>
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -10664,6 +10664,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>>                                            struct task_struct *p)
>>   {
>>          int i, nr_running;
>> +       struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
>>
>>          memset(sgs, 0, sizeof(*sgs));
>>
>> @@ -10671,7 +10672,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>>          if (sd->flags & SD_ASYM_CPUCAPACITY)
>>                  sgs->group_misfit_task_load = 1;
>>
>> -       for_each_cpu(i, sched_group_span(group)) {
> 
> nice-to-have:
> maybe add a comment here that cpus is not empty, because
> we have cpumask_intersects() check in sched_balance_find_dst_group(),
> (just in case sgs->group_type incorrectly remain 0 which is group_has_spare, if
> the cpus is empty)
> 
OK.

As Peter suggested [1] for this patch I will keep 'for_each_cpu_and()'.

I will try this cpumask pre-calculation optimization as next step,
for both update_sg_lb_stats() and update_sg_wakeup_stats().

>> +       cpumask_and(cpus, sched_group_span(group), p->cpus_ptr);
>> +       for_each_cpu(i, cpus) {
>>                  struct rq *rq = cpu_rq(i);
>>                  unsigned int local;
>>
>>
> 
> and from my understanding, for this percpu version,
> 
> Reviewed-by: Chen Yu <yu.c.chen@intel.com>
> 
Thanks for your review.

[1]: https://lore.kernel.org/all/20251014113731.GO4067720@noisy.programming.kicks-ass.net/

-adam

[tip: sched/core] sched/fair: Only update stats for allowed CPUs when looking for dst group

[tip-bot2 for Adam Li]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     82d6e01a0699800efd8b048eb584c907ccb47b7a
Gitweb:        https://git.kernel.org/tip/82d6e01a0699800efd8b048eb584c907ccb47b7a
Author:        Adam Li <adamli@os.amperecomputing.com>
AuthorDate:    Sat, 11 Oct 2025 06:43:22 
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 16 Oct 2025 11:13:50 +02:00

sched/fair: Only update stats for allowed CPUs when looking for dst group

Load imbalance is observed when the workload frequently forks new threads.
Due to CPU affinity, the workload can run on CPU 0-7 in the first
group, and only on CPU 8-11 in the second group. CPU 12-15 are always idle.

{ 0 1 2 3 4 5 6 7 } {8 9 10 11 12 13 14 15}
  * * * * * * * *    * * *  *

When looking for dst group for newly forked threads, in many times
update_sg_wakeup_stats() reports the second group has more idle CPUs
than the first group. The scheduler thinks the second group is less
busy. Then it selects least busy CPUs among CPU 8-11. Therefore CPU 8-11
can be crowded with newly forked threads, at the same time CPU 0-7
can be idle.

A task may not use all the CPUs in a schedule group due to CPU affinity.
Only update schedule group statistics for allowed CPUs.

Signed-off-by: Adam Li <adamli@os.amperecomputing.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 00f9d6c..ac881df 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10683,7 +10683,7 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
 	if (sd->flags & SD_ASYM_CPUCAPACITY)
 		sgs->group_misfit_task_load = 1;
 
-	for_each_cpu(i, sched_group_span(group)) {
+	for_each_cpu_and(i, sched_group_span(group), p->cpus_ptr) {
 		struct rq *rq = cpu_rq(i);
 		unsigned int local;