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

[Tim Chen <tim.c.chen@linux.intel.com>]

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:
> > > > > 
> > > > > [ ... ]
> > > > > 
> > > > > >        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@huawei.com/
> > > > > do we need to bring that back?
> > > > > 
> > > > I don't think we need it back. Here is why:
> > > > 
> > > > 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