Re: [PATCH] sched/fair: Add null pointer check to pick_next_entity()

Peter Zijlstra
2025-04-22T15:14:32Z

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.