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

[Luo Gengkun <luogengkun2@huawei.com>]

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.

> 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;
         }
  
         /*
@@ -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
>>