[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZqEZRBnbwLBjyJCE@chenyu5-mobl2>
Date: Wed, 24 Jul 2024 23:09:56 +0800
From: Chen Yu <yu.c.chen@...el.com>
To: Dietmar Eggemann <dietmar.eggemann@....com>, Vincent Guittot
<vincent.guittot@...aro.org>
CC: Vincent Guittot <vincent.guittot@...aro.org>, kernel test robot
<oliver.sang@...el.com>, <oe-lkp@...ts.linux.dev>, <lkp@...el.com>,
<linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...nel.org>, Lukasz Luba
<lukasz.luba@....com>, Qais Yousef <qyousef@...alina.io>, Yujie Liu
<yujie.liu@...el.com>
Subject: Re: [linus:master] [sched/pelt] 97450eb909:
INFO:task_blocked_for_more_than#seconds
Hi Dietmar,
On 2024-07-24 at 13:34:31 +0200, Dietmar Eggemann wrote:
> On 12/07/2024 18:41, Chen Yu wrote:
> > On 2024-07-09 at 12:03:42 +0200, Vincent Guittot wrote:
> >> On Tue, 9 Jul 2024 at 09:22, kernel test robot <oliver.sang@...el.com> wrote:
> >>>
> >>> Hello,
> >>>
> >>> kernel test robot noticed "INFO:task_blocked_for_more_than#seconds" on:
> >>>
> >>> commit: 97450eb909658573dcacc1063b06d3d08642c0c1 ("sched/pelt: Remove shift of thermal clock")
> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> >>
> >> First, I'm surprised that an Intel platform is impacted by this patch
> >> because Intel doesn't use it AFAIK.
> >> Then, this patch mainly remove a right shift i.e.:
> >> instead of:
> >> return rq_clock_task(rq) >> sched_hw_decay_shift
> >> we are now doing:
> >> return rq_clock_task(rq)
> >>
> >> Could it be a false positive ?
> >
> > Before trying to reproduce it locally, one question is that, should we use
> > rq_clock_task(rq) in __update_blocked_others() rather than 'now', which is
> > actually calculated by rq_clock_pelt(rq)?
> >
> > thanks,
> > Chenyu
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index d34f6d5b11b5..17ec0c51b29d 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9432,7 +9432,7 @@ static bool __update_blocked_others(struct rq *rq, bool *done)
> >
> > decayed = update_rt_rq_load_avg(now, rq, curr_class == &rt_sched_class) |
> > update_dl_rq_load_avg(now, rq, curr_class == &dl_sched_class) |
> > - update_hw_load_avg(now, rq, hw_pressure) |
> > + update_hw_load_avg(rq_clock_task(rq), rq, hw_pressure) |
> > update_irq_load_avg(rq, 0);
> >
> > if (others_have_blocked(rq))
>
> Yes, update_hw_load_avg() should be driven entirely by
> rq_clock_task(rq). But IMHO this PELT signal is only used on some arm64
> platforms. So you won't detect any misbehavior running your tests on Intel.
Yes, on Intel platform the arch_scale_hw_pressure() should return 0. Unfortunately
I could not reproduce the issue locally.
Since the commit replace rq_clock_task() with rq_clock_pelt() in __update_blocked_others(),
while keeps it rq_clock_task() in sched_tick(), I wonder if the following
code would cause inconsistence in ___update_load_sum():
u64 delta = now - sa->last_update_time
'now' could be calculated by rq_clock_pelt(), and last_update_time was
calculated by rq_clock_task(), and usually the former chases after the
latter, it cause a very large 'delta' and bring unexpected behavior.
thanks,
Chenyu
Powered by blists - more mailing lists