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

[Chen, Yu C]

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