Re: [PATCH v5 1/2] sched/cache: Reduce the overhead of task_cache_work by only scan the visisted cpus

[Tim Chen <tim.c.chen@linux.intel.com>]

On Thu, 2026-07-09 at 23:01 +0800, Chen, Yu C wrote:
> Hi Gengkun,
> 
> thanks for the update,
> 
> On 7/9/2026 9:00 PM, Luo Gengkun wrote:
> 
> > +static unsigned long fraction_mm_sched(int cpu,
> > +				       struct mm_struct *mm)
> >   {
> > +	struct sched_cache_time *pcpu_sched =
> > +		per_cpu_ptr(mm->sc_stat.pcpu_sched, cpu);
> > +	struct rq *rq = cpu_rq(cpu);
> > +
> >   	guard(raw_spinlock_irqsave)(&rq->cpu_epoch_lock);
> >   
> > +	/* Skip the rq that has not been hit for a long time */
> > +	if ((rq->cpu_epoch - pcpu_sched->epoch_timeout) > llc_epoch_affinity_timeout) {
> > +		cpumask_clear_cpu(cpu, &mm->sc_stat.visited_cpus);
> > +		return 0;
> > +	}
> > +
> 
> I was wondering if there is a race condition in the scenario
> above: If the rq has just updated its cpu_epoch field, and the
> CPU subsequently remains idle, then rq->cpu_epoch stays unchanged.
> That is to say, a race could occur as follows: if CPU1 becomes idle,
> CPU2's task_cache_work() might compute
> rq->cpu_epoch - pcpu_sched->epoch_timeout == 0 and consequently fail
> to clear CPU1 from visit_cpus.
> 
> CPU_1                                     CPU_2
> ------------------------------------      ---------------------------------
> account_mm_sched(rq_X)
>    set CPU1 in visited_cpus
> 
> 
> (idle: fair task runs here,
>   CPU1.cpu_epoch frozen afterwards,
>   pcpu_sched->epoch_timeout = CPU1.epoch)
>                                            fraction_mm_sched()
>                                              CPU1->cpu_epoch - 
> pcpu_sched->epoch_timeout = 0
>                                              -> bit CPU1 not cleared
>                                              __update_mm_sched() 
> ->update cpu_epoch, too late
> I wonder if we should let __update_mm_sched() be invoked before
> if ((rq->cpu_epoch - pcpu_sched->epoch_timeout) > 
> llc_epoch_affinity_timeout)?

Also, I am not sure why we need to create this new field pcpu_sched->epoch_timeout?
Wouldn't rq->cpu_epoch - pcpu_sched->epoch give the elapsed
epochs since the last time that cpu was visited for this mm
(as in __update_mm_sched())? 

There are some races possible
when cpu_epoch changes before the whole accounting got done.
A CPU could get visited after you think that it hasn't got visited.
But you will also have race if you use epoch_timeout. 

Tim
  

> 
> BTW, in your test data:
> sched_cache_scan: comm=redis-server pid=24660 scan=384
> Is it because NUMA balancing is disabled on your platform that it has
> to scan all the CPUs? It would be helpful to know what the result would
> be with NUMA balancing enabled.
> I found some reports from sashiko here that are worth taking a look at:
> https://sashiko.dev/#/patchset/20260709130053.2749834-1-luogengkun2%40huawei.com
> 
> thanks,
> Chenyu
>