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

["Chen, Yu C" <yu.c.chen@intel.com>]

On 7/31/2026 12:57 AM, Tim Chen wrote:

[ ... ]

>>
>> When talking about mm->sc_stat.lock, I found an issue that may be
>> worth paying attention. Below is the relevant code snippet:
>>
>> task_tick_cache()
>> {
>> 	...
>>           /* avoid moving backwards */
>>           if (time_after_eq(mm->sc_stat.epoch, epoch))
>>                   return;
>> 				
>>           guard(raw_spinlock)(&mm->sc_stat.lock);
>>
>>           if (work->next == work) {
>>                   task_work_add(p, work, TWA_RESUME);
>>                   WRITE_ONCE(mm->sc_stat.epoch, epoch);
>>           }
>> 	..
>> }
>> Actually, I don't think time_after_eq() can effectively avoid moving
>> backwards because this check is performed entirely outside the
>> protection of the spinlock. The following sequence diagram describes
>> this race condition in detail:
>>
>>        Thread A (CPU 0)                             Thread B (CPU 1)
>>        ============================                 ============================
>>        [ Initial State: mm->sc_stat.epoch = 90 ]
>>       
>>        task_tick_cache()                            task_tick_cache()
>>       
>>          |                                            |
>>          +-> Read rq->cpu_epoch = 100                 +-> Read rq->cpu_epoch = 101
>>       
>>          |                                            |
>>          +-> Lockless Check (90, 100) -> PASS         +-> Lockless Check (90, 101) -> PASS
>>       
>>          |                                            |
>>          |                                            +-> Acquires lock first
>>          |                                            +-> Writes mm->sc_stat.epoch = 101
>>          |                                            +-> Releases lock
>>          |                                                [ mm->sc_stat.epoch is now 101 ]
>>          |
>>          +-> Acquires lock
>>          |
>>          +-> work->next == work (STILL TRUE!
>>          |   Because cache_work is per-thread, Thread B's
>>          |   submission cannot clear Thread A's local state)
>>          |
>>          +-> WRITE_ONCE(mm->sc_stat.epoch, 100) !!! <-- BUG: Epoch moves backwards!
> 
> 
> That would not happen because in __update_mm_sched(), Thread A will be
> reading the updated rq->cpu_epoch and again check
> whether time is moving backwards and will skip the update if
> that's the case.
> 
> 

Maybe the backward comment in the code was a little confusing.
The "avoid moving backwards" logic was introduced to prevent a negative
timeout value in commit df0d98475954:

if (epoch - READ_ONCE(mm->sc_stat.epoch) > EPOCH_LLC_AFFINITY_TIMEOUT)

That is to say, by design, we want mm->sc_stat.epoch to chase after the
CPU's epoch and never jump ahead of any CPU's epoch. Otherwise, the 
subtraction
above could result in a huge value.

Later, in commit c1e7fe5e75ed, that negative delta was avoided by:

if ((long)(epoch - READ_ONCE(mm->sc_stat.epoch))
So now, mm->sc_stat.epoch is only best-effort to not go backward. If it
actually does go backward, in my opinion it’s not a big deal.

thanks,
Chenyu