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 8C1143C09ED for ; Thu, 13 Aug 2026 10:32:03 +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=1786617127; cv=none; b=kK2BaFXXb9EKJailpSQaFv0up9LXwEKkiBZAhCVTmE/Rlj87e9gkHxc6CGJS3NOuQFrs5DVFvDZkwI1KyrZbi6q4ugRe0CJ7z3Gx3E5OEJScC+PzZCWx8bwBSMjalNUWrTOqQjtNQuZ9bIv4Hn5fwSqxefMUwwEq+iW+ANbqMvw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786617127; c=relaxed/simple; bh=6SM/eZsZzoY1iJLT5dJBGmttOtc3iGFVz5UpdJIpXls=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=nGWj/e1f+vUS5xrh4wpoyk75fsfU3PT5BrGz8dB2+jeXIlUwrZC9yU4XfOZWWZ7sLxAP+ncF7MkorspyYGS2t8XlXwEIQKTSCsGHL22fJvisWh6dUXep8DTGNFjeUs+9YCNx6Ukkmll2u6ROvjFlj3BD0nPi9DGujEtySrCEtlw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org; spf=pass smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=lTz7kJUu; 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=pass 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="lTz7kJUu" 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=2FxwvqGTGp8FPP9zr57IxWxsc+scsqCe991crdRUejk=; b=lTz7kJUuUGVYotno8ntQMKBLtb ybmixVenSN3eSgP1zpv4M7BPZDiwMItjUqhOkfwmUqJE2Rflv7QRUhBcv41HJGkn+TuvZw94oWUht /AuC6ewl2+W4eBDZCdPNpobZoSjOov7MMZ2TJ9BhJG5CZXSGUEWAqCCaF5UxCsRAUZvKpO05c9mlr 0JfofondNedmPNGYEtLkx87ssWHe5O5DqQ8c6h0tNjX7J4Mz0miA8IG6atZbI1zDcBrZ9gqiqjJnD 95K+dZZJZo0egb/0ptkysxE2ANXzQtg/hVXehmA5go4yocfSW00w8hk6R3u/yGjBxKgpy8ekWbQWy G6I0yXGQ==; Received: from 77-249-17-252.cable.dynamic.v4.ziggo.nl ([77.249.17.252] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.99.1 #2 (Red Hat Linux)) id 1wuSj6-00000003RKQ-2ceg; Thu, 13 Aug 2026 10:31:56 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id 9BBD7301127; Thu, 13 Aug 2026 12:31:55 +0200 (CEST) Date: Thu, 13 Aug 2026 12:31:55 +0200 From: Peter Zijlstra To: Vincent Guittot Cc: mingo@redhat.com, juri.lelli@redhat.com, dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de, vschneid@redhat.com, kprateek.nayak@amd.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] sched/fair: Fix flat hierarchy Message-ID: <20260813103155.GC1246887@noisy.programming.kicks-ass.net> References: <20260812125039.1717249-1-vincent.guittot@linaro.org> <20260812140331.GN776954@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, Aug 12, 2026 at 04:21:14PM +0200, Vincent Guittot wrote: > On Wed, 12 Aug 2026 at 16:03, Peter Zijlstra wrote: > > > > On Wed, Aug 12, 2026 at 02:50:39PM +0200, Vincent Guittot wrote: > > > When a fair task is enqueued, we must update curr and more precisely > > > its vruntime before placing the enqueued task so avg vruntime will take > > > into account the last exec phase. > > > > > > Example: > > > TA is an always running task in cgroup G0. > > > TB is a short running task (cyclictest) in cgroup G1. > > > The lag of TB always increases up the clamp limit because TB is placed > > > before TA(curr) is updated (since the last tick). When curr(TA) is > > > finally updated, its last exec phase provide positive lag to TB > > > > > > Because TA and TB don't belong to the same group, enqueue_hierarchy() > > > will not update TA's entity when updating curr but only G0's entity at > > > root level. > > > > > > The same applies when dequeuing. > > > > This doesn't quite make sense to me; on the one hand you talk about > > vruntime (which is only relevant for rq->cfs) on the other hand you talk > > about non overlapping cgroup hierarchies. > > > > Hmm, update_curr() looks at ->h_curr, which is the intermediate crud. So > > even though it updates all the cgroup nonsense, it will not in fact > > update the root group, because it never actually sees rq->cfs.curr. > > Exactly > > > > > Bah. > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index dcf860c59a14..649b4f7505a1 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -7983,6 +7983,9 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags) > > > if (!p->se.sched_delayed || (flags & ENQUEUE_DELAYED)) > > > util_est_enqueue(cfs_rq, p); > > > > > > + if (cfs_rq->curr) > > > + update_curr(cfs_rq_of(cfs_rq->curr)); > > > + > > > > Still, I think this wants to be in a different spot. It needs to be > > below the whole initial if(curr) place_entity() thing. Perhaps stick > > these into {en,de}queue_hierarchy() ? > > But are we sure that cfs_rq->curr has been updated ? Otherwise it > means that we place cfs_rq->curr before having updated its vruntime so > avg_vruntime will not account the last running phase. > > The same applies when we requeue a delayed entity. Well, it is the same place the update was previously, no? Also, that XXX comment thing is about enqueueing self, with the thinking that you should not shift time since dequeue, allowing 'atomic' dequeue+enqueue. However, looking at things now, I can't actually see how this could happen in the new code. Putting a trace_printk() in also doesn't seem to help much. So perhaps there is some dead code there to clean up as well. But for consistencies sake I think placing it near {en,de}queue_hierarchy() where the other update_curr() lived/lives makes most sense. Hmm?