uclamp_fork() resets the uclamp values to their default when the
reset-on-fork flag is set. It also checks whether the task has a RT
policy, and sets its uclamp.min to 1024 accordingly. However, during
reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
hence leading to an erroneous uclamp.min setting for the new task if it
was forked from RT.
Fix this by removing the unnecessary check on rt_policy() in
uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
set.
Reported-by: Chitti Babu Theegala <[email protected]>
Signed-off-by: Quentin Perret <[email protected]>
---
kernel/sched/core.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3a61a3b8eaa9..9ea3e484eea2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
for_each_clamp_id(clamp_id) {
unsigned int clamp_value = uclamp_none(clamp_id);
- /* By default, RT tasks always get 100% boost */
- if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
- clamp_value = uclamp_none(UCLAMP_MAX);
-
uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
}
}
--
2.26.0.110.g2183baf09c-goog
On Tuesday 14 Apr 2020 at 12:21:28 (-0400), Joel Fernandes wrote:
> Shouldn't this be conditional on p->sched_reset_on_fork instead of deleting
> the code?
Right, it's not obvious from the diff, but this code _is_ conditional on
p->sched_reset_on_fork already. This is what the whole function looks
like with my patch applied:
---8<---
static void uclamp_fork(struct task_struct *p)
{
enum uclamp_id clamp_id;
for_each_clamp_id(clamp_id)
p->uclamp[clamp_id].active = false;
if (likely(!p->sched_reset_on_fork))
return;
for_each_clamp_id(clamp_id) {
unsigned int clamp_value = uclamp_none(clamp_id);
uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
}
}
--->8---
Thanks,
Quentin
On Tue, Apr 14, 2020 at 05:27:13PM +0100, Quentin Perret wrote:
> On Tuesday 14 Apr 2020 at 12:21:28 (-0400), Joel Fernandes wrote:
> > Shouldn't this be conditional on p->sched_reset_on_fork instead of deleting
> > the code?
>
> Right, it's not obvious from the diff, but this code _is_ conditional on
> p->sched_reset_on_fork already. This is what the whole function looks
> like with my patch applied:
>
> ---8<---
> static void uclamp_fork(struct task_struct *p)
> {
> enum uclamp_id clamp_id;
>
> for_each_clamp_id(clamp_id)
> p->uclamp[clamp_id].active = false;
>
> if (likely(!p->sched_reset_on_fork))
> return;
>
> for_each_clamp_id(clamp_id) {
> unsigned int clamp_value = uclamp_none(clamp_id);
>
> uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
> }
Oh ok, thanks for clarification.
- Joel
> }
> --->8---
>
> Thanks,
> Quentin
On Tue, Apr 14, 2020 at 05:13:20PM +0100, Quentin Perret wrote:
> uclamp_fork() resets the uclamp values to their default when the
> reset-on-fork flag is set. It also checks whether the task has a RT
> policy, and sets its uclamp.min to 1024 accordingly. However, during
> reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
> hence leading to an erroneous uclamp.min setting for the new task if it
> was forked from RT.
>
> Fix this by removing the unnecessary check on rt_policy() in
> uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
> set.
>
> Reported-by: Chitti Babu Theegala <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> kernel/sched/core.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3a61a3b8eaa9..9ea3e484eea2 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> for_each_clamp_id(clamp_id) {
> unsigned int clamp_value = uclamp_none(clamp_id);
>
> - /* By default, RT tasks always get 100% boost */
> - if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> - clamp_value = uclamp_none(UCLAMP_MAX);
> -
Shouldn't this be conditional on p->sched_reset_on_fork instead of deleting
the code?
thanks,
- Joel
> uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
> }
> }
> --
> 2.26.0.110.g2183baf09c-goog
>
On 14-Apr 17:13, Quentin Perret wrote:
> uclamp_fork() resets the uclamp values to their default when the
> reset-on-fork flag is set. It also checks whether the task has a RT
> policy, and sets its uclamp.min to 1024 accordingly. However, during
> reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
> hence leading to an erroneous uclamp.min setting for the new task if it
> was forked from RT.
Right, good catch. Thanks.
> Fix this by removing the unnecessary check on rt_policy() in
> uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
> set.
>
> Reported-by: Chitti Babu Theegala <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
Fixes: 1a00d999971c ("sched/uclamp: Set default clamps for RT tasks")
Reviewed-by: Patrick Bellasi <[email protected]>
> ---
> kernel/sched/core.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3a61a3b8eaa9..9ea3e484eea2 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> for_each_clamp_id(clamp_id) {
> unsigned int clamp_value = uclamp_none(clamp_id);
>
> - /* By default, RT tasks always get 100% boost */
> - if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> - clamp_value = uclamp_none(UCLAMP_MAX);
> -
> uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
> }
> }
> --
> 2.26.0.110.g2183baf09c-goog
>
--
#include <best/regards.h>
Patrick Bellasi
On Tuesday 14 Apr 2020 at 19:21:20 (+0200), Patrick Bellasi wrote:
> Fixes: 1a00d999971c ("sched/uclamp: Set default clamps for RT tasks")
Argh, you're right, we need this, I always forget ...
> Reviewed-by: Patrick Bellasi <[email protected]>
Thanks!
Quentin
Hi,
On Tue, Apr 14, 2020 at 9:13 AM Quentin Perret <[email protected]> wrote:
>
> uclamp_fork() resets the uclamp values to their default when the
> reset-on-fork flag is set. It also checks whether the task has a RT
> policy, and sets its uclamp.min to 1024 accordingly. However, during
> reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
> hence leading to an erroneous uclamp.min setting for the new task if it
> was forked from RT.
>
> Fix this by removing the unnecessary check on rt_policy() in
> uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
> set.
>
> Reported-by: Chitti Babu Theegala <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> kernel/sched/core.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3a61a3b8eaa9..9ea3e484eea2 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> for_each_clamp_id(clamp_id) {
> unsigned int clamp_value = uclamp_none(clamp_id);
>
> - /* By default, RT tasks always get 100% boost */
> - if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> - clamp_value = uclamp_none(UCLAMP_MAX);
> -
> uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
The local variable "clamp_value" doesn't have a lot of value anymore,
does it? (Pun intended). Remove it?
-Doug
Hi Doug,
On Tuesday 14 Apr 2020 at 13:45:03 (-0700), Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 14, 2020 at 9:13 AM Quentin Perret <[email protected]> wrote:
> >
> > uclamp_fork() resets the uclamp values to their default when the
> > reset-on-fork flag is set. It also checks whether the task has a RT
> > policy, and sets its uclamp.min to 1024 accordingly. However, during
> > reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
> > hence leading to an erroneous uclamp.min setting for the new task if it
> > was forked from RT.
> >
> > Fix this by removing the unnecessary check on rt_policy() in
> > uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
> > set.
> >
> > Reported-by: Chitti Babu Theegala <[email protected]>
> > Signed-off-by: Quentin Perret <[email protected]>
> > ---
> > kernel/sched/core.c | 4 ----
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 3a61a3b8eaa9..9ea3e484eea2 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> > for_each_clamp_id(clamp_id) {
> > unsigned int clamp_value = uclamp_none(clamp_id);
> >
> > - /* By default, RT tasks always get 100% boost */
> > - if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> > - clamp_value = uclamp_none(UCLAMP_MAX);
> > -
> > uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
>
> The local variable "clamp_value" doesn't have a lot of value anymore,
> does it? (Pun intended).
:)
> Remove it?
Right, but I figured the generated code should be similar, and
'uclamp_se_set(&p->uclamp_req[clamp_id], uclamp_none(clamp_id), false);'
doesn't fit in 80 cols at this identation level, so I kept the local
var. No strong opinion, though.
Thanks,
Quentin
Hi,
On Wed, Apr 15, 2020 at 1:20 AM Quentin Perret <[email protected]> wrote:
>
> Hi Doug,
>
> On Tuesday 14 Apr 2020 at 13:45:03 (-0700), Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 14, 2020 at 9:13 AM Quentin Perret <[email protected]> wrote:
> > >
> > > uclamp_fork() resets the uclamp values to their default when the
> > > reset-on-fork flag is set. It also checks whether the task has a RT
> > > policy, and sets its uclamp.min to 1024 accordingly. However, during
> > > reset-on-fork, the task's policy is lowered to SCHED_NORMAL right after,
> > > hence leading to an erroneous uclamp.min setting for the new task if it
> > > was forked from RT.
> > >
> > > Fix this by removing the unnecessary check on rt_policy() in
> > > uclamp_fork() as this doesn't make sense if the reset-on-fork flag is
> > > set.
> > >
> > > Reported-by: Chitti Babu Theegala <[email protected]>
> > > Signed-off-by: Quentin Perret <[email protected]>
> > > ---
> > > kernel/sched/core.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 3a61a3b8eaa9..9ea3e484eea2 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> > > for_each_clamp_id(clamp_id) {
> > > unsigned int clamp_value = uclamp_none(clamp_id);
> > >
> > > - /* By default, RT tasks always get 100% boost */
> > > - if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> > > - clamp_value = uclamp_none(UCLAMP_MAX);
> > > -
> > > uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
> >
> > The local variable "clamp_value" doesn't have a lot of value anymore,
> > does it? (Pun intended).
>
> :)
>
> > Remove it?
>
> Right, but I figured the generated code should be similar, and
> 'uclamp_se_set(&p->uclamp_req[clamp_id], uclamp_none(clamp_id), false);'
> doesn't fit in 80 cols at this identation level, so I kept the local
> var. No strong opinion, though.
OK. It's definitely a bikeshed color issue and since you'll spend
more time in this bikeshed than I will I'll leave it to you to pick
the color.
I'm not at all an expert on this code but it sure looks sane to me.
If you think my review tag is worth anything in this context feel free
to add it, but since you already have Patrick's mine probably adds
very little value.
-Doug
On 15-Apr 09:31, Doug Anderson wrote:
> Hi,
>
> On Wed, Apr 15, 2020 at 1:20 AM Quentin Perret <[email protected]> wrote:
> >
> > Hi Doug,
> > On Tuesday 14 Apr 2020 at 13:45:03 (-0700), Doug Anderson wrote:
> > > Hi,
> > > On Tue, Apr 14, 2020 at 9:13 AM Quentin Perret <[email protected]> wrote:
[...]
> > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > index 3a61a3b8eaa9..9ea3e484eea2 100644
> > > > --- a/kernel/sched/core.c
> > > > +++ b/kernel/sched/core.c
> > > > @@ -1234,10 +1234,6 @@ static void uclamp_fork(struct task_struct *p)
> > > > for_each_clamp_id(clamp_id) {
> > > > unsigned int clamp_value = uclamp_none(clamp_id);
> > > >
> > > > - /* By default, RT tasks always get 100% boost */
> > > > - if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> > > > - clamp_value = uclamp_none(UCLAMP_MAX);
> > > > -
> > > > uclamp_se_set(&p->uclamp_req[clamp_id], clamp_value, false);
> > >
> > > The local variable "clamp_value" doesn't have a lot of value anymore,
> > > does it? (Pun intended).
> >
> > :)
> >
> > > Remove it?
> >
> > Right, but I figured the generated code should be similar, and
> > 'uclamp_se_set(&p->uclamp_req[clamp_id], uclamp_none(clamp_id), false);'
> > doesn't fit in 80 cols at this identation level, so I kept the local
> > var. No strong opinion, though.
>
> OK. It's definitely a bikeshed color issue and since you'll spend
> more time in this bikeshed than I will I'll leave it to you to pick
> the color.
>
> I'm not at all an expert on this code but it sure looks sane to me.
> If you think my review tag is worth anything in this context feel free
> to add it, but since you already have Patrick's mine probably adds
> very little value.
Honestly I was almost there to ask for the same cleanup. :)
If you get rid of the useless variable we can go back to the exact
same code modified by the commit we are fixing, i.e. 1a00d999971c.
So, not strong opinions here too, but if it's not a big problem I
would post a cleaned up v2.
Best,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
On Wednesday 15 Apr 2020 at 19:47:18 (+0200), Patrick Bellasi wrote:
> Honestly I was almost there to ask for the same cleanup. :)
>
> If you get rid of the useless variable we can go back to the exact
> same code modified by the commit we are fixing, i.e. 1a00d999971c.
>
> So, not strong opinions here too, but if it's not a big problem I
> would post a cleaned up v2.
Alright, that works for me. I'll send a v2 shortly.
Thanks!
Quentin