Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Vincent Guittot]

On Wed, 2 Sept 2026 at 10:13, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Wed, Sep 02, 2026 at 09:55:49AM +0200, Vincent Guittot wrote:
> > On Wed, 2 Sept 2026 at 07:39, Chen Yu <yu.c.chen@xxxxxxxxx> wrote:
> > >
> > > On Fri, Aug 28, 2026 at 09:41:03AM +0200, Peter Zijlstra wrote:
> > >
> > > [ ... ]
> > >
> > > > @@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
> > > >                       weight = __calc_prop_weight(cfs_rq, se, weight);
> > > >       }
> > > >
> > > > +     for_each_sched_entity_bl(se, cfs_rq)
> > > > +             update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> > > > +
> > >
> > > It panics during bootup on a 192 Cores system,
> > > [    9.445362][ T1687] Oops: general protection fault, kernel NULL pointer dereference 0x69: 0000 [#1] SMP NOPTI
> > > [   18.987940][ T1687] CPU: 58 UID: 0 PID: 1687 Comm: systemd-udevd Not tainted 7.3.0-rc1-flat-hload+ #9 PREEMPTLAZY
> > > [   19.022374][ T1687] RIP: 0010:pick_task_fair+0x43/0xd0
> > > [   19.175316][ T1687]  <TASK>
> > > [   19.181239][ T1687]  __pick_next_task+0x49/0x1b0
> > > [   19.189229][ T1687]  __schedule+0x14b/0x6b0
> > > [   19.196711][ T1687]  preempt_schedule+0x3a/0x60
> > > [   19.204543][ T1687]  preempt_schedule_thunk+0x16/0x40
> > > [   19.212931][ T1687]  _raw_spin_unlock_irqrestore+0x2b/0x30
> > > [   19.221833][ T1687]  autogroup_move_group+0xc5/0x160
> > > [   19.230124][ T1687]  sched_autogroup_create_attach+0xa7/0x180
> > > [   19.239280][ T1687]  ksys_setsid+0x12c/0x170
> > > [   19.246753][ T1687]  __do_sys_setsid+0xe/0x20
> > > [   19.254292][ T1687]  do_syscall_64+0xbc/0x470
> > > [   19.307858][ T1687]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > [   19.316405][ T1687] RIP: 0033:0x7f347bfbcb9b
> > >
> > > It seems that the crash is a read of se->sched_delayed on a NULL se inside
> > > pick_next_entity(), so pick_eevdf() return NULL
> >
> > I faced the same crash while testing
>
> Weirdly that crash didn't show up for me :-(, I had a few others that I
> cured.

Some of my platforms didn't crash until I added +cpu in cgroup.sub_controller

>
> > >
> > > After the following top->down backlink traverse,
> > > for_each_sched_entity_bl(se, cfs_rq)
> > >     update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> > >
> > > cfs_rq is not the root->cfs_rq anymore, but a middle cfs_rq(autogroup
> > > in above example). Meanwhile rq->cfs.curr remains NULL because the
> > > rq->cfs.curr has been dequeued if the task is runnable and queued:
> > > if (on_rq)
> > >         __dequeue_entity(cfs_rq, se)
> > >
> > > se = &p->se;
> > > cfs_rq->curr = se; /*wrong cfs_rq*/
> > >
> > > Then later _raw_spin_unlock_irqrestore triggers the scheduling
> > > it picks from rq->cfs_rq.curr with a NULL tree.
> > >
> > > Maybe we need to restore the rq->cfs_rq after the
> > > for_each_sched_entity_bl()?
> > > se = &p->se;
> > > cfs_rq = &rq->cfs; <--
> >
> > Yes, this fixes it for me too
>
> So I had this issue in task_tick_fair(), where
> for_each_sched_entity_bl() clobbered cfs_rq, and fixed that by moving
> things after reweight_eevdf() (which is what uses cfs_rq).

Yeah, all eevdf related things aim to use root cfs

>
> I at point I did actually look to see if anybody else would suffer that
> same problem, but clearly I missed one.
>
> I'm thinking the problem here is set_next_task_fair() ? I think I
> misread the:
>
>         se = &p->se;
>         cfs_rq->curr = se;
>
> to reset both se and cfs_rq, but clearly it doesn't. The below should
> fix I suppose. Let me go and try and reproduce.

it doesn't boot witht he below

>
>
> ---
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -15287,9 +15287,6 @@ static void set_next_task_fair(struct rq
>                         weight = __calc_prop_weight(cfs_rq, se, weight);
>         }
>
> -       for_each_sched_entity_bl(se, cfs_rq)
> -               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> -
>         if (throttled)
>                 task_throttle_setup_work(p);
>
> @@ -15302,6 +15299,9 @@ static void set_next_task_fair(struct rq
>                         set_protect_slice(cfs_rq, se);
>         }
>
> +       for_each_sched_entity_bl(se, cfs_rq)

Doesn't for_each_sched_entity_bl assume to start from root cfs as well ?


> +               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +

This modifies se, which is then used below in
list_move(se->group_node, &rq->cfs_tasks);
and
WARN_ON_ONCE(se->sched_delayed); below

Might be good to save pse = p->se


>         if (task_on_rq_queued(p)) {
>                 /*
>                  * Move the next running task to the front of the list, so our