From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.13]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C633638DC67 for ; Thu, 30 Jul 2026 16:57:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.13 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785430659; cv=none; b=LZvkfXAhmH6v6WLYQQP11AuRbJFrH+3teZV9FSa3oiPfNUk9q7OOecAcd4sPEjJp6tjMuQ6rkFLiawNMqUQ+kBrK7wPq15sDSrUWqeWwfSrIx180BUgY0fUlNxeA96+yzVMnReRy1aadaShADFr6r4XeWZX1Abg5m22kJlSRm2U= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785430659; c=relaxed/simple; bh=yI+MeSwdkd0u5iX3StCqUDKccaiOypz++fsnEVL1VMQ=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=SlytZHle2oR2b0LhfxwkIIuupdLdLyNJ5uRyts7MkSClDGa/NgGCGsqE5vPosKsH8XuKEb2G+M7upBDapWogeueUz4px5rMkIKyPlDNoO8QVhgQpg1tIoPx1RHIOyWk6+MgLwd2k2WGs9vb472SJRwcR0ULaTwU4jCPRi7gd8No= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=ENk170Te; arc=none smtp.client-ip=192.198.163.13 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="ENk170Te" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1785430657; x=1816966657; h=message-id:subject:from:to:cc:date:in-reply-to: references:content-transfer-encoding:mime-version; bh=yI+MeSwdkd0u5iX3StCqUDKccaiOypz++fsnEVL1VMQ=; b=ENk170TeVO6I+7FwFlLLHEi78wx/pCD1B91PGxosqB/MBe9IoyY8756e JvbBiUqztJd7t4chxRCQigFE0KnUzYX2O0iCS7AHvunYkbBzbMm99BygS hQQNWHseJfxr3olcdYZ/4w0yCfDpIvb9oj1cksBK7U+QUy0zU++WCBH9z KA8KC9aLdjSe32ywtRVZLlAgiMKcAJcV6gvAMEsZUEznVf5yHLwTPlwTo rel93hMMjj9lHKz8wkTYTjosAHARTekSuzGJkmpUXrlXrmbb5J0Q6/fkY MB/obv7MONkG/UBqMPR6b0JcP9bBBkvXxIBRKOs1lEm2JHUngLoeW4lgh A==; X-CSE-ConnectionGUID: 78z3ZtkQR/2m7abwj8BRDg== X-CSE-MsgGUID: EV6ekvF2S2C+xlFrF0qGDg== X-IronPort-AV: E=McAfee;i="6800,10657,11860"; a="88590023" X-IronPort-AV: E=Sophos;i="6.25,194,1779174000"; d="scan'208";a="88590023" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by fmvoesa107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jul 2026 09:57:36 -0700 X-CSE-ConnectionGUID: QINYYJMsQne7ciUgmKvJCg== X-CSE-MsgGUID: IMTgGqeCQfmSJIAk1HdCjw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.25,194,1779174000"; d="scan'208";a="290300530" Received: from unknown (HELO [10.241.243.185]) ([10.241.243.185]) by orviesa002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jul 2026 09:57:36 -0700 Message-ID: <94e4418e7545df9509a0b8bd1233c0b00184455d.camel@linux.intel.com> Subject: Re: : [PATCH v8 1/2] sched/cache: Reduce the overhead of task_cache_work by only scan the visisted cpus From: Tim Chen To: Luo Gengkun , Chen Yu Cc: "Chen, Yu C" , dietmar.eggemann@arm.com, rostedt@goodmis.org, peterz@infradead.org, bsegall@google.com, mgorman@suse.de, vschneid@redhat.com, kprateek.nayak@amd.com, linux-kernel@vger.kernel.org, mingo@redhat.com, juri.lelli@redhat.com, vincent.guittot@linaro.org Date: Thu, 30 Jul 2026 09:57:35 -0700 In-Reply-To: <0ab7ec57-750a-4eb8-8697-aeafdd0f1786@huawei.com> References: <20260723040429.630176-1-luogengkun2@huawei.com> <20260723040429.630176-2-luogengkun2@huawei.com> <1dc03c84-9bc2-4db1-bab4-3f603fba54cd@huawei.com> <8b7cd508-3b89-4fc0-85fd-5a8d35ed06ca@huawei.com> <7d1ae7709bbe052f1d5737b6f0ef6cddf2f5e541.camel@linux.intel.com> <2d191aead79140de43022f480c3b542c101613f1.camel@linux.intel.com> <0ab7ec57-750a-4eb8-8697-aeafdd0f1786@huawei.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.58.1 (3.58.1-1.fc43) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Thu, 2026-07-30 at 21:40 +0800, Luo Gengkun wrote: >=20 > 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: > > > >=20 > > > > On 2026/7/28 20:08, Chen Yu wrote: > > > > > On Tue, Jul 28, 2026 at 04:53:59PM +0800, Luo Gengkun wrote: > > > > > >=20 > > > > > > On 2026/7/27 9:09, Chen, Yu C wrote: > > > > > > > On 7/23/2026 12:04 PM, Luo Gengkun wrote: > > > > > > >=20 > > > > > > > [ ... ] > > > > > > >=20 > > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 guard(raw_spinlock_irqsave= )(&rq->cpu_epoch_lock); > > > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __update_mm_sched(rq, pcpu= _sched); > > > > > > > > +=C2=A0=C2=A0=C2=A0 /* Skip the rq that has not been hit fo= r a long time */ > > > > > > > > +=C2=A0=C2=A0=C2=A0 if ((rq->cpu_epoch - pcpu_sched->epoch_= last_visit) > llc_epoch_affinity_timeout) { > > > > > > >=20 > > > > > > > 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-luogengku= n2@huawei.com/ > > > > > > > do we need to bring that back? > > > > > > >=20 > > > > > > I don't think we need it back. Here is why: > > > > > >=20 > > > > > > 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. Theref= ore, > > > > > > cpumask_test_cpu was necessary to filter out those cases. > > > > > >=20 > > > > > > Now, with for_each_cpu_and(i, sched_domain_span(sd), &mm->sc_st= at.visited_cpus), > > > > > > we can ensure each scanned CPU is set, so the issue no longer e= xists. > > > > > > Furthermore, the only place where the visited_cpus bits are cle= ared is > > > > > > task_cache_work(), which is only called once per scan period, t= here is no > > > > > > risk of the bit being cleared concurrently mid-loop. > > > > > >=20 > > > > >=20 > > > > > Make sense. > > > > > =20 > > > > > > However, is there a possibility that the current task_cache_wor= k() execution > > > > > > hasn't finished yet when the next scan window arrives? For inst= ance, 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 =3D work;` to the very end of task_c= ache_work(), > > > > > > would that resolve this issue? By doing so, the existing `work-= >next =3D=3D work` > > > > > > check in task_tick_cache() should fail and no new task work wil= l be submitted. > > > > > >=20 > > > > > > Please let me know if I'm missing something. > > > > > >=20 > > > > >=20 > > > > > There are two layers of protection: first a cheap timeout gate (t= ime_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 =E2= =80=94 this actually > > > > > guarantees only one scanner per mm at a time, no? > > > >=20 > > > > What I am worried about is the following scenario: > > > >=20 > > > > Thread A (CPU 0) Thread B (CPU 1) > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > task_cache_work() > > > > | > > > > +-> try_cmpxchg() =3D=3D true > > > > | (Sets next_scan =3D now + 10) > > > > | > > > > +-> Enters Scanning Loop (jiffies =3D 100) > > > > | [ Delayed / Preempted ] > > > > | jiffies advances 100 -> 115. > > > > | Thread A STILL in the loop! > > > > | task_cache_work() (j= iffies =3D 115) > > > >=20 > > > > | | > > > > | +-> next_scan =3D= =3D 110 (pass timeout check) > > > >=20 > > > > | | > > > > | +-> try_cmpxchg() = =3D=3D true > > > >=20 > > > > | | (Sets next_sca= n =3D 115 + 10) > > > > | | > > > >=20 > > > > 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 in= to consideration? > > > >=20 > > > > Perhaps we need an explicit state flag (e.g., using test_and_set_bi= t) to ensure > > > > that the previous task_cache_work() execution has fully completed b= efore allowing > > > > a new thread to proceed, regardless of whether the next scan window= has arrived. > > > > What do you think? > > >=20 > > > First, the EPOCH_PERIOD is fairly long (10 msec) so it is unlikely th= at Thread A > > > hasn't completed its work. > > >=20 > > > 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 cp= us > > > in task_cache_work(). > > >=20 > > > So I think we should be okay without an explicit state flag. > > >=20 > > > That said, I think we should use mm->sc_stat.lock instead of cpus_rea= d_lock > > > in task_cache_work()'s cpu loop. That will improve scalability. > >=20 > > It slipped my mind the cpus_read_lock is read lock, so the two > > threads could indeed go in parallel. > >=20 > > 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. > >=20 > > Tim >=20 > When talking about mm->sc_stat.lock, I found an issue that may be > worth paying attention. Below is the relevant code snippet: >=20 > task_tick_cache() > { > ... > /* avoid moving backwards */ > if (time_after_eq(mm->sc_stat.epoch, epoch)) > return; > =09 > guard(raw_spinlock)(&mm->sc_stat.lock); >=20 > if (work->next =3D=3D 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: >=20 > Thread A (CPU 0) Thread B (CPU 1) > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > [ Initial State: mm->sc_stat.epoch =3D 90 ] > =20 > task_tick_cache() task_tick_cache() > =20 > | | > +-> Read rq->cpu_epoch =3D 100 +-> Read rq->cpu_e= poch =3D 101 > =20 > | | > +-> Lockless Check (90, 100) -> PASS +-> Lockless Check (= 90, 101) -> PASS > =20 > | | > | +-> Acquires lock fi= rst > | +-> Writes mm->sc_st= at.epoch =3D 101 > | +-> Releases lock > | [ mm->sc_stat.ep= och is now 101 ] > | > +-> Acquires lock > | > +-> work->next =3D=3D 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 b= ackwards! 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. > | >=20 > The solution is straightforward: we should validate the epoch sequence > under the protection of the lock. >=20 You do not want to acquire the lock before the time check. As account_mm_s= ched() will happen very often and you will to skip those unnecessary lock acquisit= ions when rq epoch is not yet due for an update. Thanks. Tim > 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. >=20 > Below is output from lock stat: >=20 > 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 >=20 >=20 > --- 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; > =20 > epoch =3D rq->cpu_epoch; > - /* avoid moving backwards */ > - if (time_after_eq(mm->sc_stat.epoch, epoch)) > - return; > =20 > - guard(raw_spinlock)(&mm->sc_stat.lock); > + /* > + * Use trylock instead of a blocking lock to avoid spinning in th= e > + * 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 with= in > + * 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; > =20 > - if (work->next =3D=3D work) { > - task_work_add(p, work, TWA_RESUME); > - WRITE_ONCE(mm->sc_stat.epoch, epoch); > + if (work->next =3D=3D work) { > + task_work_add(p, work, TWA_RESUME); > + WRITE_ONCE(mm->sc_stat.epoch, epoch); > + } > } > } >=20 > The above code prevents the epoch from moving backward and > reduces lock contention. >=20 > What do you think? >=20 > thanks, > Gengkun >=20 > >=20