Re: : [PATCH v8 1/2] sched/cache: Reduce the overhead of task_cache_work by only scan the visisted cpus
From: Luo Gengkun
Date: Thu Jul 30 2026 - 09:50:05 EST
On 2026/7/30 10:22, Tim Chen wrote:
On Wed, 2026-07-29 at 11:29 -0700, Tim Chen wrote:
On Wed, 2026-07-29 at 17:19 +0800, Luo Gengkun wrote:
On 2026/7/28 20:08, Chen Yu wrote:
On Tue, Jul 28, 2026 at 04:53:59PM +0800, Luo Gengkun wrote:
On 2026/7/27 9:09, Chen, Yu C wrote:
On 7/23/2026 12:04 PM, Luo Gengkun wrote:I don't think we need it back. Here is why:
[ ... ]
guard(raw_spinlock_irqsave)(&rq->cpu_epoch_lock);
__update_mm_sched(rq, pcpu_sched);
+ /* Skip the rq that has not been hit for a long time */
+ if ((rq->cpu_epoch - pcpu_sched->epoch_last_visit) > llc_epoch_affinity_timeout) {
In v2 there is a check if the cpu has been set before writing:
cpumask_test_cpu(cpu_of(rq), &mm->sc_stat.visited_cpus)
https://lore.kernel.org/all/20260414150745.225416-1-luogengkun2@xxxxxxxxxx/
do we need to bring that back?
In v2, for_each_cpu was used instead of for_each_cpu_and in the inner loop,
meaning some CPUs being checked might not have been set. Therefore,
cpumask_test_cpu was necessary to filter out those cases.
Now, with for_each_cpu_and(i, sched_domain_span(sd), &mm->sc_stat.visited_cpus),
we can ensure each scanned CPU is set, so the issue no longer exists.
Furthermore, the only place where the visited_cpus bits are cleared is
task_cache_work(), which is only called once per scan period, there is no
risk of the bit being cleared concurrently mid-loop.
Make sense.
However, is there a possibility that the current task_cache_work() execution
hasn't finished yet when the next scan window arrives? For instance, if the
current task work is heavily delayed or preempted by unexpected interrupt,
jiffies could advance past next_scan before the loop completes.
If we move the `work->next = work;` to the very end of task_cache_work(),
would that resolve this issue? By doing so, the existing `work->next == work`
check in task_tick_cache() should fail and no new task work will be submitted.
Please let me know if I'm missing something.
There are two layers of protection: first a cheap timeout gate (time_before)
that skips scanning until the next period, and then a try_cmpxchg that atomically
picks a single winner among the threads that pass the timeout — this actually
guarantees only one scanner per mm at a time, no?
What I am worried about is the following scenario:
Thread A (CPU 0) Thread B (CPU 1)
================ ================
task_cache_work()
|
+-> try_cmpxchg() == true
| (Sets next_scan = now + 10)
|
+-> Enters Scanning Loop (jiffies = 100)
| [ Delayed / Preempted ]
| jiffies advances 100 -> 115.
| Thread A STILL in the loop!
| task_cache_work() (jiffies = 115)
| |
| +-> next_scan == 110 (pass timeout check)
| |
| +-> try_cmpxchg() == true
| | (Sets next_scan = 115 + 10)
| |
In other words, concurrent execution of task_cache_work() from two adjacent periods can
occur under extreme conditions; do we need to take this scenario into consideration?
Perhaps we need an explicit state flag (e.g., using test_and_set_bit) to ensure
that the previous task_cache_work() execution has fully completed before allowing
a new thread to proceed, regardless of whether the next scan window has arrived.
What do you think?
First, the EPOCH_PERIOD is fairly long (10 msec) so it is unlikely that Thread A
hasn't completed its work.
Second, suppose the above scenario happened, the two thread above
are serialized in the work on updating occupancy
and visited cpus under the cpus_read_lock when looping through the cpus
in task_cache_work().
So I think we should be okay without an explicit state flag.
That said, I think we should use mm->sc_stat.lock instead of cpus_read_lock
in task_cache_work()'s cpu loop. That will improve scalability.
It slipped my mind the cpus_read_lock is read lock, so the two
threads could indeed go in parallel.
But the update on each cpu is protected by rq->cpu_epoch_lock, and
epochs don't go backwards. So we should still get consistent updates on
epochs and occupancy stats.
Tim
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!
|
The solution is straightforward: we should validate the epoch sequence
under the protection of the lock.
Furthermore, we should use a trylock here instead of a blocking spinlock
to eliminate busy-waiting in the sensitive timer tick path. Since the
lock being held implies that another thread is already submitting the
cache work, it is safe to skip this tick and return immediately if the
trylock fails.
Below is output from lock stat:
lock_stat version 0.4
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
class name con-bounces contentions waittime-min waittime-max waittime-total waittime-avg acq-bounces acquisitions holdtime-min holdtime-max holdtime-total holdtime-avg
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
&mm->sc_stat.lock: 122711 122883 0.12 136.54 1493110.82 12.15 152643 167402 0.12 12.87 99842.76 0.60
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1763,15 +1763,23 @@ static void task_tick_cache(struct rq *rq, struct task_struct *p)
return;
epoch = rq->cpu_epoch;
- /* avoid moving backwards */
- if (time_after_eq(mm->sc_stat.epoch, epoch))
- return;
- guard(raw_spinlock)(&mm->sc_stat.lock);
+ /*
+ * Use trylock instead of a blocking lock to avoid spinning in the
+ * sensitive timer tick/scheduler path. If the lock is contended,
+ * which mean another thread is submitting the work. In this case
+ * skip this tick to reduce redundant cache_work submissions within
+ * the same epoch.
+ */
+ scoped_cond_guard(raw_spinlock_try, return, &mm->sc_stat.lock) {
+ /* avoid moving backwards */
+ if (time_after_eq(mm->sc_stat.epoch, epoch))
+ return;
- if (work->next == work) {
- task_work_add(p, work, TWA_RESUME);
- WRITE_ONCE(mm->sc_stat.epoch, epoch);
+ if (work->next == work) {
+ task_work_add(p, work, TWA_RESUME);
+ WRITE_ONCE(mm->sc_stat.epoch, epoch);
+ }
}
}
The above code prevents the epoch from moving backward and
reduces lock contention.
What do you think?
thanks,
Gengkun