From: Qais Yousef <qais.yousef@arm.com>
To: Valentin Schneider <valentin.schneider@arm.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>,
Ingo Molnar <mingo@kernel.org>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sched/uclamp: Fix rq->uclamp_max not set on first enqueue
Date: Tue, 30 Nov 2021 11:23:56 +0000 [thread overview]
Message-ID: <20211130112356.25bm5s66sywtdgw4@e107158-lin.cambridge.arm.com> (raw)
In-Reply-To: <87wnkvb35n.mognet@arm.com>
Hi Valentin
On 11/26/21 10:51, Valentin Schneider wrote:
> On 25/11/21 16:52, Qais Yousef wrote:
> > Commit d81ae8aac85c ("sched/uclamp: Fix initialization of struct
> > uclamp_rq") introduced a bug where uclamp_max of the rq is not reset to
> > match the woken up task's uclamp_max when the rq is idle. This only
> > impacts the first wake up after enabling the static key. And it only
>
> Wouldn't that rather be all wakeups after enabling the static key, until
> the rq goes idle and gains UCLAMP_FLAG_IDLE? e.g. if you enqueue N
> uclamp_max=512 tasks, the first enqueue flips the static key and the rq
> max-aggregate will stay at 1024 after the remaining enqueues.
Yep. Bad phrasing from my side. How about:
"This is visible from first wake up(s) until the first dequeue to idle after
enabling the static key"?
>
> > matters if the uclamp_max of this task is < 1024 since only then its
> > uclamp_max will be effectively ignored.
> >
> > Fix it by properly initializing rq->uclamp_flags = UCLAMP_FLAG_IDLE to
> > ensure we reset rq uclamp_max when waking up from idle.
> >
> > Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
>
> Looking at this again, I'm starting to think this actually stems from the
> introduction of the flag:
>
> e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
>
> Before the commit you point at, we would still initialize ->uclamp_flags to
> 0. This was probably hidden by all the activity at boot-time (e.g. just
> unparking smpboot threads) which yielded an nr_running>0 -> nr_running==0
> transition, IOW we'd most likely get UCLAMP_FLAG_IDLE set on a rq before
> any userspace task could get on there.
>
> The static key would have only made this problem more visible.
Hmm. I can't see the sequence of events. I guess you could argue in theory that
this commit should have initialized the ->uclamp_flags to UCLAMP_FLAG_IDLE but
I think it used to work because uc_rq->value = 0 by default
static inline void uclamp_rq_inc_id(struct rq *rq, struct task_struct *p,
enum uclamp_id clamp_id)
{
...
if (uc_se->value > READ_ONCE(uc_rq->value))
WRITE_ONCE(uc_rq->value, uc_se->value);
}
The commit I point to changed makes uc_rq->value = 1024 by default, hence we
miss the first update.
I don't mind updating the FIXES tag here, though AFAICT there's no visible side
effect from it.
>
> > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
>
> Changelog nitpicking aside:
> Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com>
Thanks!
--
Qais Yousef
next prev parent reply other threads:[~2021-11-30 11:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-25 16:52 [PATCH] sched/uclamp: Fix rq->uclamp_max not set on first enqueue Qais Yousef
2021-11-26 10:51 ` Valentin Schneider
2021-11-30 11:23 ` Qais Yousef [this message]
2021-11-30 12:29 ` Valentin Schneider
2021-11-30 15:41 ` Qais Yousef
2021-12-01 9:20 ` Dietmar Eggemann
2021-12-02 10:36 ` Qais Yousef
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211130112356.25bm5s66sywtdgw4@e107158-lin.cambridge.arm.com \
--to=qais.yousef@arm.com \
--cc=dietmar.eggemann@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=valentin.schneider@arm.com \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).