From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (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 CDE5D29B78F for ; Wed, 28 Jan 2026 10:35:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769596523; cv=none; b=aAXAYGjR1LbQFcl6h7wgKZJOl88So6547y4qrDwhsyLp+cFVZrSkwoR3/TJzZ6+5a6ig3vb9RtPTE4XVmy9x5WFvP7sSmVXGgLxOVr/dVsM9s4qLrDi/CHq32Le1YhMSxjGdo2tL6c8+tLfGQoCpx44528rBHsdZZtbnk+7WaIg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769596523; c=relaxed/simple; bh=4UPwqrpebVioGe+ReYUi4IGcIQ0oxVAkMteb6l8mW30=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=K+0pxmbzhScjFHzWHvM2F8hlIBbSQrI+6UMOrcd0UAZd+8RkMjuGTRnlQfJhiK+c99X77pxeg9mn91cbsf/FFlp83zdG9VWxcFfKULXaqbKR/cNcdOf0RywsfAsz+iAbp2bSsi/tF9dNo1rCC3ojhBBGPN58gBffZ3GK09Liros= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org; spf=none smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=ZzSxAm0y; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="ZzSxAm0y" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=EOhYbr+KeRPc9zw+orgTolkh7Mg7AlPTxhM5pgg8N7E=; b=ZzSxAm0ymgtEI2TiGuPPX2AuLD LpRLpjy7SB3x3+n5Q8xK0cwUAgN+c+SWfjpP7P2H6KPTg08AF+V/k1QjKPPsA3VgylxAxdzOM7/cu b8VEKZh0IRhmj5Uc/od/llbEeJsrA5nX+Y/+IVHzeUTx2uxsNBrDods9FhYDQykiMmFYxT5Rp1/P+ SNFrSTxQu/aslDSMfR4RhA6UGibyxKBcdZnFTv34yZRY6F8yBoCt9OZ0dmAz6j+t/M3sUzlcTpRPZ ZK1PB5cPaY72wyX23sFoUJ2HOI7XKQzMKHA5qKqIfAAcgH8yB/hIPYdQxPuwkckJrDAnNE9bOK2Gp esFpH4KA==; Received: from 2001-1c00-8d85-5700-266e-96ff-fe07-7dcc.cable.dynamic.v6.ziggo.nl ([2001:1c00:8d85:5700:266e:96ff:fe07:7dcc] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.98.2 #2 (Red Hat Linux)) id 1vl2tI-00000008tCF-0mOQ; Wed, 28 Jan 2026 10:35:16 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id DE583300754; Wed, 28 Jan 2026 11:35:14 +0100 (CET) Date: Wed, 28 Jan 2026 11:35:14 +0100 From: Peter Zijlstra To: Johannes Weiner Cc: Suren Baghdasaryan , Ingo Molnar , Chengming Zhou , Dietmar Eggemann , John Stultz , linux-kernel@vger.kernel.org Subject: Re: [PATCH resend 1/2] sched: psi: loosen clock sync between scheduler and aggregator Message-ID: <20260128103514.GX171111@noisy.programming.kicks-ass.net> References: <20260114154317.1815429-1-hannes@cmpxchg.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260114154317.1815429-1-hannes@cmpxchg.org> On Wed, Jan 14, 2026 at 10:43:16AM -0500, Johannes Weiner wrote: > In the aggregator, catch races between state snooping and task state > conclusions explicitly by checking for sample underflows; then move > the clock reads out of the reader's seqcount protection. I'm utterly failing to make sense of this -- what?! > This shrinks the critical section and allows switching the scheduler > side to looser (cheaper) clock sourcing in the next patch. > > Suggested-by: Chengming Zhou > Reviewed-by: Chengming Zhou > Signed-off-by: Johannes Weiner > --- > kernel/sched/psi.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index 59fdb7ebbf22..4b7bf8eb46c2 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -285,7 +285,6 @@ static void get_recent_times(struct psi_group *group, int cpu, > /* Snapshot a coherent view of the CPU state */ > do { > seq = psi_read_begin(cpu); > - now = cpu_clock(cpu); > memcpy(times, groupc->times, sizeof(groupc->times)); > state_mask = groupc->state_mask; > state_start = groupc->state_start; > @@ -293,6 +292,9 @@ static void get_recent_times(struct psi_group *group, int cpu, > memcpy(tasks, groupc->tasks, sizeof(groupc->tasks)); > } while (psi_read_retry(cpu, seq)); > > + if (state_mask) > + now = cpu_clock(cpu); > + So this can be later... which results in a larger value > /* Calculate state time deltas against the previous snapshot */ > for (s = 0; s < NR_PSI_STATES; s++) { > u32 delta; > @@ -308,7 +310,22 @@ static void get_recent_times(struct psi_group *group, int cpu, > if (state_mask & (1 << s)) > times[s] += now - state_start; > which makes times[s] larger than it should be > + /* > + * This snooping ahead can obviously race with the > + * state concluding on the cpu. If we previously > + * snooped to a time past where the state concludes, > + * times[s] can now be behind times_prev[s]. > + * > + * time_after32() would be the obvious choice, but > + * S32_MAX is right around two seconds, which is the > + * aggregation interval; if the aggregator gets > + * delayed, there would be a risk of dismissing > + * genuinely large samples. Use a larger margin. > + */ > delta = times[s] - groupc->times_prev[aggregator][s]; > + if (delta > psi_period + (psi_period >> 1)) > + delta = 0; > + This seems to check if times_prev is larger than times; confused again. > groupc->times_prev[aggregator][s] = times[s]; It updates times_prev irrespectively. Storing a potentially larger value. > > times[s] = delta; And stores the delta, which can be larger than it should be? > @@ -908,16 +925,18 @@ static void psi_flags_change(struct task_struct *task, int clear, int set) > > void psi_task_change(struct task_struct *task, int clear, int set) > { > - int cpu = task_cpu(task); > + int cpu; > u64 now; > > if (!task->pid) > return; > > + cpu = task_cpu(task); > + now = cpu_clock(cpu); > + So this clock is earlier, or smaller. > psi_flags_change(task, clear, set); > > psi_write_begin(cpu); > - now = cpu_clock(cpu); > for_each_group(group, task_psi_group(task)) > psi_group_change(group, cpu, clear, set, now, true); > psi_write_end(cpu); > @@ -928,10 +947,9 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next, > { > struct psi_group *common = NULL; > int cpu = task_cpu(prev); > - u64 now; > + u64 now = cpu_clock(cpu); Same. > psi_write_begin(cpu); > - now = cpu_clock(cpu); > > if (next->pid) { > psi_flags_change(next, 0, TSK_ONCPU); > @@ -999,6 +1017,7 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next, > psi_group_change(group, cpu, clear, set, now, wake_clock); > } > } > + > psi_write_end(cpu); > } > > @@ -1027,9 +1046,9 @@ void psi_account_irqtime(struct rq *rq, struct task_struct *curr, struct task_st > return; > rq->psi_irq_time = irq; > > - psi_write_begin(cpu); > now = cpu_clock(cpu); Same. > + psi_write_begin(cpu); > for_each_group(group, task_psi_group(curr)) { > if (!group->enabled) > continue; > @@ -1234,8 +1253,9 @@ void psi_cgroup_restart(struct psi_group *group) > > guard(rq_lock_irq)(cpu_rq(cpu)); > > - psi_write_begin(cpu); > now = cpu_clock(cpu); And same again. > + > + psi_write_begin(cpu); > psi_group_change(group, cpu, 0, 0, now, true); > psi_write_end(cpu); > } For all these we call psi_group_change(), which calls record_times() which then sets ->state_start to a smaller value. Resulting in times above to be larger still. So this inflates delta and leaves me utterly confused. Not only does the Changelog here not explain anything, this also very much needs comments in the code, because the next time someone is going to be reading this, they'll break their WTF'o'meter and probably the next one they get too. /me stomps off searching for where the heck he left his pile of spare WTF'o'meters..