The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last
active task to maintain the last uclamp.max and prevent blocked util
from suddenly becoming visible.
However, there is an asymmetry in how the flag is set and cleared which
can lead to having the flag set whilst there are active tasks on the rq.
Specifically, the flag is cleared in the uclamp_rq_inc() path, which is
called at enqueue time, but set in uclamp_rq_dec_id() which is called
both when dequeueing a task _and_ in the update_uclamp_active() path. As
a result, when both uclamp_rq_{dec,ind}_id() are called from
update_uclamp_active(), the flag ends up being set but not cleared,
hence leaving the runqueue in a broken state.
Fix this by clearing the flag in uclamp_idle_reset() which is also
called in both paths to ensure things remain symmetrical.
Fixes: e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
Reported-by: Rick Yiu <[email protected]>
Reviewed-by: Qais Yousef <[email protected]>
Signed-off-by: Quentin Perret <[email protected]>
---
kernel/sched/core.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4ca80df205ce..e514a093a0ba 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -980,6 +980,7 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
return;
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
}
@@ -1252,10 +1253,6 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
for_each_clamp_id(clamp_id)
uclamp_rq_inc_id(rq, p, clamp_id);
-
- /* Reset clamp idle holding when there is one RUNNABLE task */
- if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
- rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
}
static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
--
2.32.0.288.g62a8d224e6-goog
Hi Quentin
On 06/23/21 12:34, Quentin Perret wrote:
> The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last
> active task to maintain the last uclamp.max and prevent blocked util
> from suddenly becoming visible.
>
> However, there is an asymmetry in how the flag is set and cleared which
> can lead to having the flag set whilst there are active tasks on the rq.
> Specifically, the flag is cleared in the uclamp_rq_inc() path, which is
> called at enqueue time, but set in uclamp_rq_dec_id() which is called
> both when dequeueing a task _and_ in the update_uclamp_active() path. As
> a result, when both uclamp_rq_{dec,ind}_id() are called from
> update_uclamp_active(), the flag ends up being set but not cleared,
> hence leaving the runqueue in a broken state.
>
> Fix this by clearing the flag in uclamp_idle_reset() which is also
> called in both paths to ensure things remain symmetrical.
>
> Fixes: e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
> Reported-by: Rick Yiu <[email protected]>
> Reviewed-by: Qais Yousef <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> kernel/sched/core.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 4ca80df205ce..e514a093a0ba 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -980,6 +980,7 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> return;
>
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
I just realized this needs
if (clamp_id == UCLAMP_MAX)
rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
The code is only set for UCLAMP_MAX, so should be cleared for UCLAMP_MAX too.
Though there's ugly overload here:
if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
return;
This check would fail prematurely if UCLAMP_MAX was reset before UCLAMP_MIN.
The code before your change would reset both then do the clear. But now when we
do it from here, we need to be more careful about that.
Not sure what we can do beside adding a comment. The options I'm thinking about
are too intrusive FWIW.
Cheers
--
Qais Yousef
> WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> }
>
> @@ -1252,10 +1253,6 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>
> for_each_clamp_id(clamp_id)
> uclamp_rq_inc_id(rq, p, clamp_id);
> -
> - /* Reset clamp idle holding when there is one RUNNABLE task */
> - if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> }
>
> static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> --
> 2.32.0.288.g62a8d224e6-goog
>
Hi Qais,
On Wednesday 30 Jun 2021 at 15:58:48 (+0100), Qais Yousef wrote:
> I just realized this needs
>
> if (clamp_id == UCLAMP_MAX)
> rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
>
> The code is only set for UCLAMP_MAX, so should be cleared for UCLAMP_MAX too.
>
> Though there's ugly overload here:
>
> if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> return;
>
> This check would fail prematurely if UCLAMP_MAX was reset before UCLAMP_MIN.
> The code before your change would reset both then do the clear. But now when we
> do it from here, we need to be more careful about that.
Right, although this should all work fine as-is, I agree that relying on
the calling order is a bit dodgy and might cause issues in the long run.
What do you think of this instead?
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b094da4c5fea..c0b999a8062a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
return;
- rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
}
@@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
for_each_clamp_id(clamp_id)
uclamp_rq_inc_id(rq, p, clamp_id);
+
+ /* Reset clamp idle holding when there is one RUNNABLE task */
+ if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
}
static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
@@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
if (p->uclamp[clamp_id].active) {
uclamp_rq_dec_id(rq, p, clamp_id);
uclamp_rq_inc_id(rq, p, clamp_id);
+
+ /*
+ * Make sure to clear the idle flag if we've transiently reached
+ * 0 uclamp active tasks on the rq.
+ */
+ if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
}
task_rq_unlock(rq, p, &rf);
On Wednesday 30 Jun 2021 at 15:45:14 (+0000), Quentin Perret wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b094da4c5fea..c0b999a8062a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> return;
>
> - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> }
>
> @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>
> for_each_clamp_id(clamp_id)
> uclamp_rq_inc_id(rq, p, clamp_id);
> +
> + /* Reset clamp idle holding when there is one RUNNABLE task */
> + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> }
>
> static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> if (p->uclamp[clamp_id].active) {
> uclamp_rq_dec_id(rq, p, clamp_id);
> uclamp_rq_inc_id(rq, p, clamp_id);
> +
> + /*
> + * Make sure to clear the idle flag if we've transiently reached
> + * 0 uclamp active tasks on the rq.
> + */
> + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
Bah, now that I had coffee I realize this has the exact same problem.
Let me look at this again ...
On 06/30/21 15:45, Quentin Perret wrote:
> Hi Qais,
>
> On Wednesday 30 Jun 2021 at 15:58:48 (+0100), Qais Yousef wrote:
> > I just realized this needs
> >
> > if (clamp_id == UCLAMP_MAX)
> > rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> >
> > The code is only set for UCLAMP_MAX, so should be cleared for UCLAMP_MAX too.
> >
> > Though there's ugly overload here:
> >
> > if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> > return;
> >
> > This check would fail prematurely if UCLAMP_MAX was reset before UCLAMP_MIN.
> > The code before your change would reset both then do the clear. But now when we
> > do it from here, we need to be more careful about that.
>
> Right, although this should all work fine as-is, I agree that relying on
> the calling order is a bit dodgy and might cause issues in the long run.
>
> What do you think of this instead?
I can't objectively say one way is better than the other, this has the drawback
of having to remember to clear the flag after each call to uclamp_rq_inc_id().
So it's pick your pain type of situation :-)
We can move the flag to struct uclamp_se. But this looks unnecessary churn to
me..
Cheers
--
Qais Yousef
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b094da4c5fea..c0b999a8062a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> return;
>
> - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> }
>
> @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>
> for_each_clamp_id(clamp_id)
> uclamp_rq_inc_id(rq, p, clamp_id);
> +
> + /* Reset clamp idle holding when there is one RUNNABLE task */
> + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> }
>
> static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> if (p->uclamp[clamp_id].active) {
> uclamp_rq_dec_id(rq, p, clamp_id);
> uclamp_rq_inc_id(rq, p, clamp_id);
> +
> + /*
> + * Make sure to clear the idle flag if we've transiently reached
> + * 0 uclamp active tasks on the rq.
> + */
> + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> }
>
> task_rq_unlock(rq, p, &rf);
On 07/01/21 10:07, Quentin Perret wrote:
> On Wednesday 30 Jun 2021 at 15:45:14 (+0000), Quentin Perret wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index b094da4c5fea..c0b999a8062a 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> > if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> > return;
> >
> > - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> > }
> >
> > @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
> >
> > for_each_clamp_id(clamp_id)
> > uclamp_rq_inc_id(rq, p, clamp_id);
> > +
> > + /* Reset clamp idle holding when there is one RUNNABLE task */
> > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > }
> >
> > static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> > @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> > if (p->uclamp[clamp_id].active) {
> > uclamp_rq_dec_id(rq, p, clamp_id);
> > uclamp_rq_inc_id(rq, p, clamp_id);
> > +
> > + /*
> > + * Make sure to clear the idle flag if we've transiently reached
> > + * 0 uclamp active tasks on the rq.
> > + */
> > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
>
> Bah, now that I had coffee I realize this has the exact same problem.
> Let me look at this again ...
Hehe uclamp has this effect. It's all obvious, until it's not :-)
Yes this needs to be out of the loop.
Thanks for looking at this!
Cheers
--
Qais Yousef
On Thursday 01 Jul 2021 at 12:08:03 (+0100), Qais Yousef wrote:
> On 07/01/21 10:07, Quentin Perret wrote:
> > On Wednesday 30 Jun 2021 at 15:45:14 (+0000), Quentin Perret wrote:
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index b094da4c5fea..c0b999a8062a 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> > > if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> > > return;
> > >
> > > - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> > > }
> > >
> > > @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
> > >
> > > for_each_clamp_id(clamp_id)
> > > uclamp_rq_inc_id(rq, p, clamp_id);
> > > +
> > > + /* Reset clamp idle holding when there is one RUNNABLE task */
> > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > }
> > >
> > > static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> > > @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> > > if (p->uclamp[clamp_id].active) {
> > > uclamp_rq_dec_id(rq, p, clamp_id);
> > > uclamp_rq_inc_id(rq, p, clamp_id);
> > > +
> > > + /*
> > > + * Make sure to clear the idle flag if we've transiently reached
> > > + * 0 uclamp active tasks on the rq.
> > > + */
> > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> >
> > Bah, now that I had coffee I realize this has the exact same problem.
> > Let me look at this again ...
>
> Hehe uclamp has this effect. It's all obvious, until it's not :-)
Indeed ... :)
> Yes this needs to be out of the loop.
Right or maybe we can just check that uclamp_id == UCLAMP_MAX here and
we should be good to go? That is, what about the below?
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b094da4c5fea..8e9b8106a0df 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
return;
- rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
}
@@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
for_each_clamp_id(clamp_id)
uclamp_rq_inc_id(rq, p, clamp_id);
+
+ /* Reset clamp idle holding when there is one RUNNABLE task */
+ if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
}
static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
@@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
if (p->uclamp[clamp_id].active) {
uclamp_rq_dec_id(rq, p, clamp_id);
uclamp_rq_inc_id(rq, p, clamp_id);
+
+ /*
+ * Make sure to clear the idle flag if we've transiently reached
+ * 0 active tasks on rq.
+ */
+ if (clamp_id == MAX && rq->uclamp_flags & UCLAMP_FLAG_IDLE)
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
}
task_rq_unlock(rq, p, &rf);
On 07/01/21 12:43, Quentin Perret wrote:
> On Thursday 01 Jul 2021 at 12:08:03 (+0100), Qais Yousef wrote:
> > On 07/01/21 10:07, Quentin Perret wrote:
> > > On Wednesday 30 Jun 2021 at 15:45:14 (+0000), Quentin Perret wrote:
> > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > index b094da4c5fea..c0b999a8062a 100644
> > > > --- a/kernel/sched/core.c
> > > > +++ b/kernel/sched/core.c
> > > > @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> > > > if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> > > > return;
> > > >
> > > > - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > > WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> > > > }
> > > >
> > > > @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
> > > >
> > > > for_each_clamp_id(clamp_id)
> > > > uclamp_rq_inc_id(rq, p, clamp_id);
> > > > +
> > > > + /* Reset clamp idle holding when there is one RUNNABLE task */
> > > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > > > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > > }
> > > >
> > > > static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> > > > @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> > > > if (p->uclamp[clamp_id].active) {
> > > > uclamp_rq_dec_id(rq, p, clamp_id);
> > > > uclamp_rq_inc_id(rq, p, clamp_id);
> > > > +
> > > > + /*
> > > > + * Make sure to clear the idle flag if we've transiently reached
> > > > + * 0 uclamp active tasks on the rq.
> > > > + */
> > > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > > > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > >
> > > Bah, now that I had coffee I realize this has the exact same problem.
> > > Let me look at this again ...
> >
> > Hehe uclamp has this effect. It's all obvious, until it's not :-)
>
> Indeed ... :)
>
> > Yes this needs to be out of the loop.
>
> Right or maybe we can just check that uclamp_id == UCLAMP_MAX here and
> we should be good to go? That is, what about the below?
Wouldn't it be better to do this from uclamp_idle_reset() then?
Thanks
--
Qais Yousef
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b094da4c5fea..8e9b8106a0df 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> return;
>
> - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> }
>
> @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>
> for_each_clamp_id(clamp_id)
> uclamp_rq_inc_id(rq, p, clamp_id);
> +
> + /* Reset clamp idle holding when there is one RUNNABLE task */
> + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> }
>
> static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> if (p->uclamp[clamp_id].active) {
> uclamp_rq_dec_id(rq, p, clamp_id);
> uclamp_rq_inc_id(rq, p, clamp_id);
> +
> + /*
> + * Make sure to clear the idle flag if we've transiently reached
> + * 0 active tasks on rq.
> + */
> + if (clamp_id == MAX && rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> }
>
> task_rq_unlock(rq, p, &rf);
On Thursday 01 Jul 2021 at 15:57:50 (+0100), Qais Yousef wrote:
> On 07/01/21 12:43, Quentin Perret wrote:
> > On Thursday 01 Jul 2021 at 12:08:03 (+0100), Qais Yousef wrote:
> > > On 07/01/21 10:07, Quentin Perret wrote:
> > > > On Wednesday 30 Jun 2021 at 15:45:14 (+0000), Quentin Perret wrote:
> > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > > index b094da4c5fea..c0b999a8062a 100644
> > > > > --- a/kernel/sched/core.c
> > > > > +++ b/kernel/sched/core.c
> > > > > @@ -980,7 +980,6 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
> > > > > if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> > > > > return;
> > > > >
> > > > > - rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > > > WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> > > > > }
> > > > >
> > > > > @@ -1253,6 +1252,10 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
> > > > >
> > > > > for_each_clamp_id(clamp_id)
> > > > > uclamp_rq_inc_id(rq, p, clamp_id);
> > > > > +
> > > > > + /* Reset clamp idle holding when there is one RUNNABLE task */
> > > > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > > > > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > > > }
> > > > >
> > > > > static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> > > > > @@ -1300,6 +1303,13 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id)
> > > > > if (p->uclamp[clamp_id].active) {
> > > > > uclamp_rq_dec_id(rq, p, clamp_id);
> > > > > uclamp_rq_inc_id(rq, p, clamp_id);
> > > > > +
> > > > > + /*
> > > > > + * Make sure to clear the idle flag if we've transiently reached
> > > > > + * 0 uclamp active tasks on the rq.
> > > > > + */
> > > > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > > > > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > > >
> > > > Bah, now that I had coffee I realize this has the exact same problem.
> > > > Let me look at this again ...
> > >
> > > Hehe uclamp has this effect. It's all obvious, until it's not :-)
> >
> > Indeed ... :)
> >
> > > Yes this needs to be out of the loop.
> >
> > Right or maybe we can just check that uclamp_id == UCLAMP_MAX here and
> > we should be good to go? That is, what about the below?
>
> Wouldn't it be better to do this from uclamp_idle_reset() then?
That should work too, but clearing the flag outside of
uclamp_rq_inc_id() feels a little bit more robust to ordering
issues.
Specifically, uclamp_rq_inc() has the following pattern:
for_each_clamp_id(clamp_id)
uclamp_rq_inc_id(rq, p , clamp_id);
if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
So, if we change this to clear the flag from
uclamp_rq_inc_id()->uclamp_idle_reset() then we'll have issues if
(for example) for_each_clamp_id()'s order changes in the future.
IOW, it feels cleaner to not create side effects in uclamp_rq_inc_id()
that impact the idle flag given that its very own behaviour depends on
the flag.
WDYT?
Cheers,
Quentin
On 07/01/21 15:20, Quentin Perret wrote:
> > > Right or maybe we can just check that uclamp_id == UCLAMP_MAX here and
> > > we should be good to go? That is, what about the below?
> >
> > Wouldn't it be better to do this from uclamp_idle_reset() then?
>
> That should work too, but clearing the flag outside of
> uclamp_rq_inc_id() feels a little bit more robust to ordering
> issues.
>
> Specifically, uclamp_rq_inc() has the following pattern:
>
> for_each_clamp_id(clamp_id)
> uclamp_rq_inc_id(rq, p , clamp_id);
>
> if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
>
> So, if we change this to clear the flag from
> uclamp_rq_inc_id()->uclamp_idle_reset() then we'll have issues if
> (for example) for_each_clamp_id()'s order changes in the future.
> IOW, it feels cleaner to not create side effects in uclamp_rq_inc_id()
> that impact the idle flag given that its very own behaviour depends on
> the flag.
>
> WDYT?
Do the clearing from outside the loop then to keep the pattern consistent?
Anyway, I think there's no clear objective advantage. So I'll trust your
judgement and promise not to complain with your final choice ;-)
Cheers
--
Qais Yousef
On Thursday 01 Jul 2021 at 18:59:32 (+0100), Qais Yousef wrote:
> On 07/01/21 15:20, Quentin Perret wrote:
> > > > Right or maybe we can just check that uclamp_id == UCLAMP_MAX here and
> > > > we should be good to go? That is, what about the below?
> > >
> > > Wouldn't it be better to do this from uclamp_idle_reset() then?
> >
> > That should work too, but clearing the flag outside of
> > uclamp_rq_inc_id() feels a little bit more robust to ordering
> > issues.
> >
> > Specifically, uclamp_rq_inc() has the following pattern:
> >
> > for_each_clamp_id(clamp_id)
> > uclamp_rq_inc_id(rq, p , clamp_id);
> >
> > if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
> > rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> >
> > So, if we change this to clear the flag from
> > uclamp_rq_inc_id()->uclamp_idle_reset() then we'll have issues if
> > (for example) for_each_clamp_id()'s order changes in the future.
> > IOW, it feels cleaner to not create side effects in uclamp_rq_inc_id()
> > that impact the idle flag given that its very own behaviour depends on
> > the flag.
> >
> > WDYT?
>
> Do the clearing from outside the loop then to keep the pattern consistent?
Right, but I actually preferred doing it from here as we're under
task_rq_lock(), which means well behaved readers won't observe the flag
being transiently set. I could also refactor the locking, but oh well ...
> Anyway, I think there's no clear objective advantage. So I'll trust your
> judgement and promise not to complain with your final choice ;-)
:) Alrighty, I'll cook something.
Thanks!
Quentin