Re: : [PATCH v8 1/2] sched/cache: Reduce the overhead of task_cache_work by only scan the visisted cpus
From: Luo Gengkun
Date: Fri Jul 31 2026 - 04:40:05 EST
On 2026/7/31 14:58, Chen, Yu C wrote:
On 7/31/2026 12:57 AM, Tim Chen wrote:Thank you for your clarifying explanation :)
[ ... ]
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
Gengkun