[PATCH v4 0/2] A couple of uclamp fixes

[PATCH v4 0/2] A couple of uclamp fixes

[Quentin Perret]

Hi all,

This is v4 of small series previously posted here:

  https://lore.kernel.org/lkml/20210623123441.592348-1-qperret@google.com/

I dropped the last patch of the series which requires a bit more
discussion, and kept the first two here as they're standalone fixes.

Thanks,
Quentin

Quentin Perret (2):
  sched: Fix UCLAMP_FLAG_IDLE setting
  sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

 kernel/sched/core.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

-- 
2.32.0.402.g57bb445576-goog

[PATCH v4 1/2] sched: Fix UCLAMP_FLAG_IDLE setting

[Quentin Perret]

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 <rickyiu@google.com>
Signed-off-by: Quentin Perret <qperret@google.com>
---
 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

[PATCH v4 2/2] sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

[Quentin Perret]

SCHED_FLAG_KEEP_PARAMS can be passed to sched_setattr to specify that
the call must not touch scheduling parameters (nice or priority). This
is particularly handy for uclamp when used in conjunction with
SCHED_FLAG_KEEP_POLICY as that allows to issue a syscall that only
impacts uclamp values.

However, sched_setattr always checks whether the priorities and nice
values passed in sched_attr are valid first, even if those never get
used down the line. This is useless at best since userspace can
trivially bypass this check to set the uclamp values by specifying low
priorities. However, it is cumbersome to do so as there is no single
expression of this that skips both RT and CFS checks at once. As such,
userspace needs to query the task policy first with e.g. sched_getattr
and then set sched_attr.sched_priority accordingly. This is racy and
slower than a single call.

As the priority and nice checks are useless when SCHED_FLAG_KEEP_PARAMS
is specified, simply inherit them in this case to match the policy
inheritance of SCHED_FLAG_KEEP_POLICY.

Reported-by: Wei Wang <wvw@google.com>
Reviewed-by: Qais Yousef <qais.yousef@arm.com>
Signed-off-by: Quentin Perret <qperret@google.com>
---
 kernel/sched/core.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e801d2c3077b..914076eab242 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7332,6 +7332,16 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 	return -E2BIG;
 }
 
+static void get_params(struct task_struct *p, struct sched_attr *attr)
+{
+	if (task_has_dl_policy(p))
+		__getparam_dl(p, attr);
+	else if (task_has_rt_policy(p))
+		attr->sched_priority = p->rt_priority;
+	else
+		attr->sched_nice = task_nice(p);
+}
+
 /**
  * sys_sched_setscheduler - set/change the scheduler policy and RT priority
  * @pid: the pid in question.
@@ -7393,6 +7403,8 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	rcu_read_unlock();
 
 	if (likely(p)) {
+		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS)
+			get_params(p, &attr);
 		retval = sched_setattr(p, &attr);
 		put_task_struct(p);
 	}
@@ -7541,12 +7553,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
 	kattr.sched_policy = p->policy;
 	if (p->sched_reset_on_fork)
 		kattr.sched_flags |= SCHED_FLAG_RESET_ON_FORK;
-	if (task_has_dl_policy(p))
-		__getparam_dl(p, &kattr);
-	else if (task_has_rt_policy(p))
-		kattr.sched_priority = p->rt_priority;
-	else
-		kattr.sched_nice = task_nice(p);
+	get_params(p, &kattr);
 
 #ifdef CONFIG_UCLAMP_TASK
 	/*
-- 
2.32.0.402.g57bb445576-goog

Re: [PATCH v4 1/2] sched: Fix UCLAMP_FLAG_IDLE setting

[Dietmar Eggemann]

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;
 }

Re: [PATCH v4 1/2] sched: Fix UCLAMP_FLAG_IDLE setting

[Quentin Perret]

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

Re: [PATCH v4 1/2] sched: Fix UCLAMP_FLAG_IDLE setting

[Dietmar Eggemann]

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 <dietmar.eggemann@arm.com>

to your version in case you want to keep it.

Re: [PATCH v4 2/2] sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

[Dietmar Eggemann]

On 19/07/2021 18:16, Quentin Perret wrote:
> SCHED_FLAG_KEEP_PARAMS can be passed to sched_setattr to specify that
> the call must not touch scheduling parameters (nice or priority). This

What about DL params (runtime, deadline, period)?

Uclamp is not for DL but we could (*) still set uclamp values on a DL
task. Obviously they would only be used when the task switches policy.

On tip/sched/core:

root@juno:~# chrt -d -P 1000000000 -T 100000000 -p 0 1671

root@juno:~# uclampset -m200 -M400 -p 1671

root@juno:~# cat /proc/1671/sched | grep uclamp
uclamp.min                                   :                  200
uclamp.max                                   :                  400
effective uclamp.min                         :                  200
effective uclamp.max                         :                  400

root@juno:~# chrt -o -p 0
pid 1702's current scheduling policy: SCHED_OTHER
pid 1702's current scheduling priority: 0

root@juno:~# cat /proc/1671/sched | grep uclamp
uclamp.min                                   :                  200
uclamp.max                                   :                  400
effective uclamp.min                         :                  200
effective uclamp.max                         :                  400


> is particularly handy for uclamp when used in conjunction with
> SCHED_FLAG_KEEP_POLICY as that allows to issue a syscall that only
> impacts uclamp values.
> 
> However, sched_setattr always checks whether the priorities and nice
> values passed in sched_attr are valid first, even if those never get

+ DL params (__checkparam_dl())

> used down the line. This is useless at best since userspace can
> trivially bypass this check to set the uclamp values by specifying low
> priorities. However, it is cumbersome to do so as there is no single
> expression of this that skips both RT and CFS checks at once. As such,
> userspace needs to query the task policy first with e.g. sched_getattr
> and then set sched_attr.sched_priority accordingly. This is racy and
> slower than a single call.
> 
> As the priority and nice checks are useless when SCHED_FLAG_KEEP_PARAMS
> is specified, simply inherit them in this case to match the policy
> inheritance of SCHED_FLAG_KEEP_POLICY.
> 
> Reported-by: Wei Wang <wvw@google.com>
> Reviewed-by: Qais Yousef <qais.yousef@arm.com>
> Signed-off-by: Quentin Perret <qperret@google.com>
> ---
>  kernel/sched/core.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e801d2c3077b..914076eab242 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7332,6 +7332,16 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
>  	return -E2BIG;
>  }
>  
> +static void get_params(struct task_struct *p, struct sched_attr *attr)
> +{
> +	if (task_has_dl_policy(p))
> +		__getparam_dl(p, attr);

(*) This changes the behaviour when setting uclamp values on a DL task.

Before uclamp values could be set but now, because of

  void __getparam_dl(struct task_struct *p, struct sched_attr *attr)
    ..
    attr->sched_flags = dl_se->flags

SCHED_FLAG_UTIL_CLAMP gets overwritten and  __sched_setscheduler() bails in:

    if (unlikely(policy == p->policy)) {
      ...
      retval = 0;
      goto unlock;
    }
  change:

I.e. the:

      if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
        goto change;

can't trigger anymore.


> +	else if (task_has_rt_policy(p))
> +		attr->sched_priority = p->rt_priority;
> +	else
> +		attr->sched_nice = task_nice(p);
> +}
> +
>  /**
>   * sys_sched_setscheduler - set/change the scheduler policy and RT priority
>   * @pid: the pid in question.
> @@ -7393,6 +7403,8 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
>  	rcu_read_unlock();
>  
>  	if (likely(p)) {
> +		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS)
> +			get_params(p, &attr);

SCHED_FLAG_KEEP_PARAMS is handled here but SCHED_FLAG_KEEP_POLICY
outside (before) the `if (likely(p))`?

>  		retval = sched_setattr(p, &attr);
>  		put_task_struct(p);
>  	}

[...]

Re: [PATCH v4 2/2] sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

[Quentin Perret]

On Thursday 22 Jul 2021 at 10:47:33 (+0200), Dietmar Eggemann wrote:
> (*) This changes the behaviour when setting uclamp values on a DL task.
> 
> Before uclamp values could be set but now, because of
> 
>   void __getparam_dl(struct task_struct *p, struct sched_attr *attr)
>     ..
>     attr->sched_flags = dl_se->flags
> 
> SCHED_FLAG_UTIL_CLAMP gets overwritten and  __sched_setscheduler() bails in:
> 
>     if (unlikely(policy == p->policy)) {
>       ...
>       retval = 0;
>       goto unlock;
>     }
>   change:
> 
> I.e. the:
> 
>       if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
>         goto change;
> 
> can't trigger anymore.

Bah, as you said it doesn't seem to be a big deal, but clearly that was
unintentional. Let me try and fix this.

> > +	else if (task_has_rt_policy(p))
> > +		attr->sched_priority = p->rt_priority;
> > +	else
> > +		attr->sched_nice = task_nice(p);
> > +}
> > +
> >  /**
> >   * sys_sched_setscheduler - set/change the scheduler policy and RT priority
> >   * @pid: the pid in question.
> > @@ -7393,6 +7403,8 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
> >  	rcu_read_unlock();
> >  
> >  	if (likely(p)) {
> > +		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS)
> > +			get_params(p, &attr);
> 
> SCHED_FLAG_KEEP_PARAMS is handled here but SCHED_FLAG_KEEP_POLICY
> outside (before) the `if (likely(p))`?

Because I need to dereference p while SCHED_FLAG_KEEP_POLICY doesn't :)

> >  		retval = sched_setattr(p, &attr);
> >  		put_task_struct(p);
> >  	}
> 
> [...]

Re: [PATCH v4 2/2] sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

[Quentin Perret]

On Monday 26 Jul 2021 at 14:56:10 (+0100), Quentin Perret wrote:
> On Thursday 22 Jul 2021 at 10:47:33 (+0200), Dietmar Eggemann wrote:
> > (*) This changes the behaviour when setting uclamp values on a DL task.
> > 
> > Before uclamp values could be set but now, because of
> > 
> >   void __getparam_dl(struct task_struct *p, struct sched_attr *attr)
> >     ..
> >     attr->sched_flags = dl_se->flags
> > 
> > SCHED_FLAG_UTIL_CLAMP gets overwritten and  __sched_setscheduler() bails in:
> > 
> >     if (unlikely(policy == p->policy)) {
> >       ...
> >       retval = 0;
> >       goto unlock;
> >     }
> >   change:
> > 
> > I.e. the:
> > 
> >       if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
> >         goto change;
> > 
> > can't trigger anymore.
> 
> Bah, as you said it doesn't seem to be a big deal, but clearly that was
> unintentional. Let me try and fix this.

While looking at this I found existing bugs in the area. Fixes are here:

https://lore.kernel.org/lkml/20210727101103.2729607-1-qperret@google.com/

And with the above series applied this patch should behave correctly
now.

Thanks,
Quentin

Re: [PATCH v4 1/2] sched: Fix UCLAMP_FLAG_IDLE setting

[Qais Yousef]

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 <rickyiu@google.com>
> Signed-off-by: Quentin Perret <qperret@google.com>
> ---

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 <qais.yousef@arm.com>

Thanks!

--
Qais Yousef

Re: [PATCH v4 2/2] sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

[Dietmar Eggemann]

On 26/07/2021 15:56, Quentin Perret wrote:
> On Thursday 22 Jul 2021 at 10:47:33 (+0200), Dietmar Eggemann wrote:

[...]

>>> @@ -7393,6 +7403,8 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
>>>  	rcu_read_unlock();
>>>  
>>>  	if (likely(p)) {
>>> +		if (attr.sched_flags & SCHED_FLAG_KEEP_PARAMS)
>>> +			get_params(p, &attr);
>>
>> SCHED_FLAG_KEEP_PARAMS is handled here but SCHED_FLAG_KEEP_POLICY
>> outside (before) the `if (likely(p))`?
> 
> Because I need to dereference p while SCHED_FLAG_KEEP_POLICY doesn't :)

Ah, true. Looked weird though.
But then the SCHED_FLAG_KEEP_POLICY condition can be placed closer to
the SCHED_FLAG_KEEP_PARAMS condition. We don't have to set
SETPARAM_POLICY if p == NULL.

>>>  		retval = sched_setattr(p, &attr);
>>>  		put_task_struct(p);
>>>  	}

[...]

Re: [PATCH v4 2/2] sched: Skip priority checks with SCHED_FLAG_KEEP_PARAMS

[Dietmar Eggemann]

On 27/07/2021 12:16, Quentin Perret wrote:
> On Monday 26 Jul 2021 at 14:56:10 (+0100), Quentin Perret wrote:
>> On Thursday 22 Jul 2021 at 10:47:33 (+0200), Dietmar Eggemann wrote:
>>> (*) This changes the behaviour when setting uclamp values on a DL task.
>>>
>>> Before uclamp values could be set but now, because of
>>>
>>>   void __getparam_dl(struct task_struct *p, struct sched_attr *attr)
>>>     ..
>>>     attr->sched_flags = dl_se->flags
>>>
>>> SCHED_FLAG_UTIL_CLAMP gets overwritten and  __sched_setscheduler() bails in:
>>>
>>>     if (unlikely(policy == p->policy)) {
>>>       ...
>>>       retval = 0;
>>>       goto unlock;
>>>     }
>>>   change:
>>>
>>> I.e. the:
>>>
>>>       if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
>>>         goto change;
>>>
>>> can't trigger anymore.
>>
>> Bah, as you said it doesn't seem to be a big deal, but clearly that was
>> unintentional. Let me try and fix this.
> 
> While looking at this I found existing bugs in the area. Fixes are here:
> 
> https://lore.kernel.org/lkml/20210727101103.2729607-1-qperret@google.com/
> 
> And with the above series applied this patch should behave correctly
> now.

It does. Like depicted in

https://lkml.kernel.org/r/e6d103f1-f8ee-cad9-c7c0-c9ea5d0f099a@arm.com

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>