[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aAtNf6QMG7Dj6snR@telecaster>
Date: Fri, 25 Apr 2025 01:53:19 -0700
From: Omar Sandoval <osandov@...ndov.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Rik van Riel <riel@...riel.com>, Chris Mason <clm@...a.com>,
Pat Cody <pat@...cody.io>, mingo@...hat.com, juri.lelli@...hat.com,
vincent.guittot@...aro.org, dietmar.eggemann@....com,
rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
vschneid@...hat.com, linux-kernel@...r.kernel.org, patcody@...a.com,
kernel-team@...a.com, Breno Leitao <leitao@...ian.org>
Subject: Re: [PATCH] sched/fair: Add null pointer check to pick_next_entity()
On Tue, Apr 22, 2025 at 05:14:21PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 22, 2025 at 04:13:52PM +0200, Peter Zijlstra wrote:
> > On Mon, Apr 21, 2025 at 05:06:45PM -0700, Omar Sandoval wrote:
> >
> > > Hey, Peter,
> > >
> > > We haven't been able to test your latest patch, but I dug through some
> > > core dumps from crashes with your initial zero_vruntime patch. It looks
> > > like on just about all of them, the entity vruntimes are way too spread
> > > out, so we would get overflows regardless of what we picked as
> > > zero_vruntime.
> > >
> > > As a representative example, we have a cfs_rq with 3 entities with the
> > > follow vruntimes and (scaled down) weights:
> > >
> > > vruntime weight
> > > 39052385155836636 2 (curr)
> > > 43658311782076206 2
> > > 42824722322062111 4886
> > >
> > > The difference between the minimum and maximum is 4605926626239570,
> >
> > Right, that is quite beyond usable. The key question at this point
> > is how did we get here...
> >
> > > which is 53 bits. The total load is 4890. Even if you picked
> > > zero_vruntime to be equidistant from the minimum and maximum, the
> > > (vruntime - zero_vruntime) * load calculation in entity_eligible() is
> > > doomed to overflow.
> > >
> > > That range in vruntime seems too absurd to be due to only to running too
> > > long without preemption. We're only seeing these crashes on internal
> > > node cgroups (i.e., cgroups whose children are cgroups, not tasks). This
> > > all leads me to suspect reweight_entity().
> > >
> > > Specifically, this line in reweight_entity():
> > >
> > > se->vlag = div_s64(se->vlag * se->load.weight, weight);
> > >
> > > seems like it could create a very large vlag, which could cause
> > > place_entity() to adjust vruntime by a large value.
> >
> > Right, I fixed that not too long ago. At the time I convinced myself
> > clipping there wasn't needed (in fact, it would lead to some other
> > artifacts iirc). Let me go review that decision :-)
>
> In particular, the two most recent commits in this area are:
>
> https://lore.kernel.org/r/20250109105959.GA2981@noisy.programming.kicks-ass.net
> https://lkml.kernel.org/r/20250110115720.GA17405@noisy.programming.kicks-ass.net
>
> (from the same thread).
>
> Note that it does call update_entity_lag() which does clip. So after
> that it's just scaling for the new weight.
>
> Notably, virtual time = time / weight, and the clip limit is adjusted
> for weight.
>
> So if it is inside limits pre-scaling, it should still be in limits
> after scaling.
>
> l = max / w;
>
> w->w' --> l' = l*w/w' = (max / w) * (w/w') = max / w'
>
> I've stuck some trace_printk()s on again, and the numbers I get here
> seem sane.
For anyone following along, I found the source of the bad vruntimes and
sent a patch:
https://lore.kernel.org/all/f0c2d1072be229e1bdddc73c0703919a8b00c652.1745570998.git.osandov@fb.com/
Powered by blists - more mailing lists