[PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Quentin Perret]

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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Joel Fernandes]

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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Quentin Perret]

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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Joel Fernandes]

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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Patrick Bellasi]

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

Fixes: 1a00d999971c ("sched/uclamp: Set default clamps for RT tasks")
Reviewed-by: Patrick Bellasi <patrick.bellasi@matbug.net>

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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Quentin Perret]

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 <patrick.bellasi@matbug.net>

Thanks!
Quentin

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Doug Anderson]

Hi,

On Tue, Apr 14, 2020 at 9:13 AM Quentin Perret <qperret@google.com> 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 <ctheegal@codeaurora.org>
> Signed-off-by: Quentin Perret <qperret@google.com>
> ---
>  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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Quentin Perret]

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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Doug Anderson]

Hi,

On Wed, Apr 15, 2020 at 1:20 AM Quentin Perret <qperret@google.com> 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 <qperret@google.com> 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 <ctheegal@codeaurora.org>
> > > Signed-off-by: Quentin Perret <qperret@google.com>
> > > ---
> > >  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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Patrick Bellasi]

On 15-Apr 09:31, Doug Anderson wrote:
> Hi,
> 
> On Wed, Apr 15, 2020 at 1:20 AM Quentin Perret <qperret@google.com> 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 <qperret@google.com> 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

Re: [PATCH] sched/core: Fix reset-on-fork from RT with uclamp

[Quentin Perret]

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