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 609F42EAB71 for ; Wed, 28 Jan 2026 20:17:54 +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=1769631476; cv=none; b=DBNjVCMvGzA7vjwH/jJ9hsDeWeJWt06Ya4XD2bwSmnvueZLL4XLDufWLl9/SCdnYwRRC196Fn7xWfL4Tma6SvyM+T68Xu1C2+Dq3fQxn2wQOQkWg1uLiMlCgDVj+E17J9btmRstK+DUWoMAwcAq2TWdjvC8AD4+U0xP2bWZ6w1I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769631476; c=relaxed/simple; bh=1v733BMUGBE1Mjlw5U94HCuMwlAyZrJXyEt0IG5Ubbs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=L8r8BMkm2b5BW1M9fq4O7hM+rC0Z7BL/aRz/5zk3JVtWUjHaobdrfWwlG94XJr5IJurjRig3T1750yP8V4mXWhHaHjs11nlvv1IBcsWTdH/EWXHgOinaXv1D86Tq4BUeSQikhUMhIp8zgjL+nAY80bBLmokgVYKi6NnSrgvUYW0= 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=d7SrYRaB; 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="d7SrYRaB" 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=v0f/oFZAmB3HfJxQuNXeq0MCQcKcsUQ1ShAAqF/dIPc=; b=d7SrYRaBMA6LwZ76mXEQPemrdO PN1mFZzxoF+3lRvfJXEMjgneHO62eTG9K2+cSmGJcgeUWSUVrOJ8E2AqUuURzVeJEycBbXC+tAljl HeZHDuggDKXYiQCEh9KJ0YzMH+oXUhaAi7LbcnWR0NhVCg9oW6qaFXb+//kDzASBDOEKIjCAZuyMw a5c5eRn41VRmIRqZaSTiZYo2/cyeL5ogj/SZyStHqD6gyzl8UCPppeSJyiSFkFoIjOcX+4+BmUIpx UIqaToEz1bJguMy8AohwzvZyW0GXJ1Lt4dnsoXCWdOaeuEBn/RM5bDLgo8cRb/2vFD2vKPdwMTGj1 EFpPQuHA==; 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 1vlBz4-00000009YUH-2AvZ; Wed, 28 Jan 2026 20:17:50 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id 6C629303018; Wed, 28 Jan 2026 21:17:43 +0100 (CET) Date: Wed, 28 Jan 2026 21:17:43 +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: <20260128201743.GW166857@noisy.programming.kicks-ass.net> References: <20260114154317.1815429-1-hannes@cmpxchg.org> <20260128103514.GX171111@noisy.programming.kicks-ass.net> 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: On Wed, Jan 28, 2026 at 01:56:20PM -0500, Johannes Weiner wrote: > That mutual understanding of "now" is expensive, so I'm trying to > remove it. This necessarily opens a race: the reader could sample a > live state to a time beyond what the scheduler will use to conclude > that state. The result is that one reader run can oversample slightly, > and the next run will see a negative delta. Right, exactly the problem from: 3840cbe24cf0 ("sched: psi: fix bogus pressure spikes from aggregation race") > Here is how I'm proposing to handle it: > > > > + /* > > > + * 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. > > In the last round, the reader oversampled. That means times_prev[] > slipped ahead of the scheduler reality (times[]). > > The actual accounting error caused by this should be acceptable, owed > to the slightly different clock reads of the two racing threads. > > Certainly, there is no meaningful additional sample time in *this* > round, so we fix the delta to 0. OK, so you don't care about the fact that consistent races can inflate the number beyond actuality. You just want to get rid of backward motion? The argument being something like: Since its a decaying average, and the per-time accumulated error is relatively minor, it doesn't affect the over-all outcome much. Because if we just consider it as free-running counters, the accumulation error is unbound. But since it is time averaged, the error becomes insignificant. > > > groupc->times_prev[aggregator][s] = times[s]; > > > > It updates times_prev irrespectively. Storing a potentially larger > > value. > > Right, this is on purpose. Once the delta is extracted and processed, > we need to update the reader to where the scheduler is, as per > usual. But there is now a second case: > > 1) Existing case. The scheduler accumulated state time the reader > hasn't seen yet, so times_prev[] is behind times[]. After delta > extraction, the assignment catches the reader up to the scheduler. > > 2) New case. The previous reader run used a `now' too far in the > future and oversampled. times_prev[] is *ahead* of times[]. The > assignment *rewinds* the reader back to the scheduler's reality. Well, not quite, that too large delta has already 'escaped' and been accumulated. This way you ensure a second race will again result in a too large delta being accumulated, rather than the next state being accounted slightly short -- which would compensate for the previous one being accounted slightly larger. That is afaict 2) ensures you are consistently oversampling but never undersampling. > > > times[s] = delta; > > > > And stores the delta, which can be larger than it should be? > > We filtered out bogus deltas, so it should be valid or 0. Semantically a too large value is equally bogus to a negative value. Its just that negative numbers are not expected and wreck things down the chain. > > 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. > > I think there are two considerations. > > We use that clock value to both start and stop the clock on a > state. So while we use a timestamp from slightly earlier in the > scheduling event, we do it on both ends. This should not affect > long-term accuracy. If we only consider these timestamps, sure. But due to the whole accumulation mess in between you get unbounded drift (on the accumulator -- pre-averaging). > Then there is reader coherency. Before, state_start would match the > exact time at which the reader could see the state go live inside the > state_mask. After the patch, the reader could see a state whose > state_start is slightly in the past. But that's the common case for > the reader anyway? Since it rarely runs in the same nanosecond in > which the state change occurs. > > Am I missing something? So currently, with things being inside the locks, if we sample early we miss a bit. If we sample late, we see the exact time. With the update time being early, we go back to 3840cbe24cf0, and can see a too long period in both cases. But because you're then also using a late clock on read, the error is larger still. If you are measuring time from a start to 'now', and the actual period is (10 characters) like so: .x.|--------|.y. Then, if you move the start to x (earlier), your period becomes longer (12 characters). If you then also move now to y (later) you get an ever larger error (14 characters). I mean, it all probably doesn't matter, but its there. > > 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. > > Fair enough, it's tricky. > > I'll do my best to capture all the above into the changelog and code > comments. But let's try to get on the same page first. That should > also help identify which parts exactly need the most help. Mostly I wasn't sure on which errors you care about and which you don't. As I understand it now, you *only* care about not having negative values in the accumulation chain because that's otherwise unsigned and negatives show up as large numbers and things go 'funny'. You do not care about long term drift in the pure accumulator -- since its all time averaged?