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 update_uclamp_active() as well.
Fixes: e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
Reported-by: Rick Yiu <[email protected]>
Signed-off-by: Quentin Perret <[email protected]>
---
kernel/sched/core.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index cf16f8fda9a6..e801d2c3077b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1619,6 +1619,23 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
uclamp_rq_dec_id(rq, p, clamp_id);
}
+static inline void uclamp_rq_reinc_id(struct rq *rq, struct task_struct *p,
+ enum uclamp_id clamp_id)
+{
+ if (!p->uclamp[clamp_id].active)
+ return;
+
+ 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 == UCLAMP_MAX && (rq->uclamp_flags & UCLAMP_FLAG_IDLE))
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
+}
+
static inline void
uclamp_update_active(struct task_struct *p)
{
@@ -1642,12 +1659,8 @@ uclamp_update_active(struct task_struct *p)
* affecting a valid clamp bucket, the next time it's enqueued,
* it will already see the updated clamp bucket value.
*/
- for_each_clamp_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);
- }
- }
+ for_each_clamp_id(clamp_id)
+ uclamp_rq_reinc_id(rq, p, clamp_id);
task_rq_unlock(rq, p, &rf);
}
--
2.32.0.402.g57bb445576-goog
On 19/07/2021 18:16, 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
s/active/runnable ?
> from suddenly becoming visible.
>
[...]
IMHO, the main argument in v3 to do the clearing outside
uclamp_rq_inc_id() was a possible order change in `for_each_clamp_id()`.
So setting/clearing `rq->uclamp_flags` (UCLAMP_FLAG_IDLE) on UCLAMP_MAX
(currently the highest Uclamp constraint (UCLAMP_CNT-1)) could be
incorrect when UCLAMP_MIN and UCLAMP_MAX change place because the
same `rq->uclamp_flags` value is needed for both Uclamp constraint
values.
What about decoupling rq->uclamp_flags` handling from UCLAMP_MAX and
doing this for 'UCLAMP_CNT - 1', i.e. always on the highest Uclamp
constraint?
#define for_each_clamp_id(clamp_id) \
for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
In this case the code change can be as easy as in your original v3.
Setting UCLAMP_FLAG_IDLE in uclamp_idle_value():
uclamp_rq_dec_id() -> uclamp_rq_max_value() -> *uclamp_idle_value()*
Resetting UCLAMP_FLAG_IDLE in uclamp_idle_reset():
uclamp_rq_inc_id() -> *uclamp_idle_reset()*
This would be more symmetrical then uclamp_idle_value() and
uclamp_rq_inc()/uclamp_rq_reinc_id().
--8<--
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0c22cd026440..600f68f3378c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1300,15 +1300,16 @@ static inline unsigned int
uclamp_idle_value(struct rq *rq, enum uclamp_id clamp_id,
unsigned int clamp_value)
{
+ if (clamp_id == UCLAMP_CNT - 1)
+ rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
+
/*
* Avoid blocked utilization pushing up the frequency when we go
* idle (which drops the max-clamp) by retaining the last known
* max-clamp.
*/
- if (clamp_id == UCLAMP_MAX) {
- rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
+ if (clamp_id == UCLAMP_MAX)
return clamp_value;
- }
return uclamp_none(UCLAMP_MIN);
}
@@ -1320,6 +1321,9 @@ static inline void uclamp_idle_reset(struct rq *rq, enum uclamp_id clamp_id,
if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
return;
+ if ((clamp_id == UCLAMP_CNT - 1) && (rq->uclamp_flags & UCLAMP_FLAG_IDLE))
+ rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
+
WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
}
@@ -1595,10 +1599,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;
}
Hi Dietmar,
On Wednesday 21 Jul 2021 at 12:07:04 (+0200), Dietmar Eggemann wrote:
> On 19/07/2021 18:16, 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
>
> s/active/runnable ?
'active' should still be correct here no? We enter uclamp_rq_max_value()
-> uclamp_idle_value() when the last _active_ uclamp_se is decremented,
and when all the buckets are empty, so I think that works?
> > from suddenly becoming visible.
> >
>
> [...]
>
> IMHO, the main argument in v3 to do the clearing outside
> uclamp_rq_inc_id() was a possible order change in `for_each_clamp_id()`.
> So setting/clearing `rq->uclamp_flags` (UCLAMP_FLAG_IDLE) on UCLAMP_MAX
> (currently the highest Uclamp constraint (UCLAMP_CNT-1)) could be
> incorrect when UCLAMP_MIN and UCLAMP_MAX change place because the
> same `rq->uclamp_flags` value is needed for both Uclamp constraint
> values.
>
> What about decoupling rq->uclamp_flags` handling from UCLAMP_MAX and
> doing this for 'UCLAMP_CNT - 1', i.e. always on the highest Uclamp
> constraint?
>
> #define for_each_clamp_id(clamp_id) \
> for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
>
> In this case the code change can be as easy as in your original v3.
>
> Setting UCLAMP_FLAG_IDLE in uclamp_idle_value():
>
> uclamp_rq_dec_id() -> uclamp_rq_max_value() -> *uclamp_idle_value()*
>
> Resetting UCLAMP_FLAG_IDLE in uclamp_idle_reset():
>
> uclamp_rq_inc_id() -> *uclamp_idle_reset()*
>
> This would be more symmetrical then uclamp_idle_value() and
> uclamp_rq_inc()/uclamp_rq_reinc_id().
Right, thanks for the suggestion but to be fair I feel like this is a
matter of personal preference at this point. I personally like the way
it is in this patch -- I find it easier to reason about, but maybe
that's because I wrote it ...
Do you feel strongly about it? If not I'd prefer to not re-spin this
another time if possible. Let me know what you think.
Cheers,
Quentin
On 21/07/2021 15:09, Quentin Perret wrote:
> Hi Dietmar,
>
> On Wednesday 21 Jul 2021 at 12:07:04 (+0200), Dietmar Eggemann wrote:
>> On 19/07/2021 18:16, 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
>>
>> s/active/runnable ?
>
> 'active' should still be correct here no? We enter uclamp_rq_max_value()
> -> uclamp_idle_value() when the last _active_ uclamp_se is decremented,
> and when all the buckets are empty, so I think that works?
Ah, it this uclamp ative `p->uclamp[clamp_id].active` which is set with
`bucket->tasks` in uclamp_rq_[inc/dec]_id.
Maybe add: last (uclamp) active task, i.e. (bucket.tasks == 0 for all
bucket_id's) ... ?
>>> from suddenly becoming visible.
>>>
>>
>> [...]
>>
>> IMHO, the main argument in v3 to do the clearing outside
>> uclamp_rq_inc_id() was a possible order change in `for_each_clamp_id()`.
>> So setting/clearing `rq->uclamp_flags` (UCLAMP_FLAG_IDLE) on UCLAMP_MAX
>> (currently the highest Uclamp constraint (UCLAMP_CNT-1)) could be
>> incorrect when UCLAMP_MIN and UCLAMP_MAX change place because the
>> same `rq->uclamp_flags` value is needed for both Uclamp constraint
>> values.
>>
>> What about decoupling rq->uclamp_flags` handling from UCLAMP_MAX and
>> doing this for 'UCLAMP_CNT - 1', i.e. always on the highest Uclamp
>> constraint?
>>
>> #define for_each_clamp_id(clamp_id) \
>> for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
>>
>> In this case the code change can be as easy as in your original v3.
>>
>> Setting UCLAMP_FLAG_IDLE in uclamp_idle_value():
>>
>> uclamp_rq_dec_id() -> uclamp_rq_max_value() -> *uclamp_idle_value()*
>>
>> Resetting UCLAMP_FLAG_IDLE in uclamp_idle_reset():
>>
>> uclamp_rq_inc_id() -> *uclamp_idle_reset()*
>>
>> This would be more symmetrical then uclamp_idle_value() and
>> uclamp_rq_inc()/uclamp_rq_reinc_id().
>
> Right, thanks for the suggestion but to be fair I feel like this is a
> matter of personal preference at this point. I personally like the way
> it is in this patch -- I find it easier to reason about, but maybe
> that's because I wrote it ...
>
> Do you feel strongly about it? If not I'd prefer to not re-spin this
> another time if possible. Let me know what you think.
No, not at all ;-) Just like it better since it would mean less code
changes and only one place to reset UCLAMP_FLAG_IDLE.
You can add a:
Tested-by: Dietmar Eggemann <[email protected]>
to your version in case you want to keep it.
On 07/19/21 17:16, 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 update_uclamp_active() as well.
>
> Fixes: e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
> Reported-by: Rick Yiu <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
I've put a note that handling of this flag needs to be improved for the future.
But for now and FWIW, this LGTM:
Reviewed-by: Qais Yousef <[email protected]>
Thanks!
--
Qais Yousef