Re: : [PATCH v8 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/31 14:58, Chen, Yu C wrote:
> 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
> 
> 
Thank you for your clarifying explanation :)

Gengkun
>