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

[Omar Sandoval]

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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>   https://lkml.kernel.org/r/20250110115720.GA17405@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> (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@xxxxxx/