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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8fd5189f-112b-4f30-8fde-10843b6a7fa8@os.amperecomputing.com>
Date: Wed, 15 Oct 2025 18:10:13 +0800
From: Adam Li <adamli@...amperecomputing.com>
To: "Chen, Yu C" <yu.c.chen@...el.com>
Cc: dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, vschneid@...hat.com, cl@...ux.com,
 linux-kernel@...r.kernel.org, patches@...erecomputing.com,
 shkaushik@...erecomputing.com, mingo@...hat.com, peterz@...radead.org,
 juri.lelli@...hat.com, vincent.guittot@...aro.org
Subject: Re: [PATCH RESEND] sched/fair: Only update stats for allowed CPUs
 when looking for dst group

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@...el.com>
> 
Thanks for your review.

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

-adam