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

[Tim Chen]

On Wed, 2026-07-15 at 14:41 +0800, Luo Gengkun wrote:
> 
> On 2026/7/15 6:40, Tim Chen wrote:
> > 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())?
> > 
> Good question.
> During previous testing, I found that pcpu_sched->epoch was being flushed
> by calls to fraction_mm_sched(), even though the task was not running on
> that specific CPU. Because the flush frequency was higher than the eviction
> frequency, the eviction mechanism failed. The newly introduced epoch_timeout
> ensures that the epoch_timeout is only flushed when the task is actually
> running on the corresponding CPU, thereby preventing the aforementioned issue.

Okay. pcpu_sched->epoch is refreshed when the accounting is done on the cpu and
pcpu_sched->epoch_timeout is refreshed when a task in the process was last run
on the CPU. I think renaming to pcpu_sched->epoch_last_visit makes things clearer.

> 
> > 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
> 
> If a CPU was just visited, there are two possible conditions:
> 
> 1. The CPU is not yet set in visited_cpus, meaning account_mm_sched() has not been
> called yet. In this case, fraction_mm_sched() won't be triggered either. This is
> acceptable because it implies pcpu_sched->runtime hasn't been updated for a long
> time too, and this negligible contribution can be safely ignored.
> 
> 2. The CPU is already set in visited_cpus. In this case, we can simply accept this
> contribution instead of performing an early return. We can just clear the CPU
> from the mask and proceed, as our primary goal is simply to limit the number of
> scanned CPUs. As shown in the code below. What do you think?
> 
> @@ -1651,7 +1651,6 @@ static unsigned long fraction_mm_sched(int cpu,
>          if (sched_feat(SC_VISIT) &&
>              (rq->cpu_epoch - pcpu_sched->epoch_timeout) > llc_epoch_affinity_timeout) {
>                  cpumask_clear_cpu(cpu, mm->sc_stat.visited_cpus);
> -               return 0;
>          }

You need to call __update_mm_sched(rq, pcpu_sched) to have rq->epoch updated to current
time before you perform the above comparison and see if we exceed
llc_epoch_affinity_timeout.  I think that's what Chen Yu pointed out
in his previous mail.

Tim

>   
>          /*
> @@ -1913,8 +1912,6 @@ static void task_cache_work(struct callback_head *work)
>   
>                                  scanned++;
>                                  occ = fraction_mm_sched(i, mm);
> -                               if (occ == 0)
> -                                       continue;
>   
>                                  a_occ += occ;
> 
> 
> thanks,
> Gengkun
> 
> > 
> > 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
> > >