[PATCH 1/2] sched/fair: Fix cfs_rq_clock_pelt() for throttled cfs_rq

[PATCH 1/2] sched/fair: Fix cfs_rq_clock_pelt() for throttled cfs_rq

[Chengming Zhou]

Since commit 23127296889f ("sched/fair: Update scale invariance of PELT")
change to use rq_clock_pelt() instead of rq_clock_task(), we should also
use rq_clock_pelt() for throttled_clock_task_time and throttled_clock_task
accounting.

Fixes: 23127296889f ("sched/fair: Update scale invariance of PELT")
Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 kernel/sched/fair.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d4bd299d67ab..e6fa5d1141b4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4846,7 +4846,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
 
 	cfs_rq->throttle_count--;
 	if (!cfs_rq->throttle_count) {
-		cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
+		cfs_rq->throttled_clock_task_time += rq_clock_pelt(rq) -
 					     cfs_rq->throttled_clock_task;
 
 		/* Add cfs_rq with load or one or more already running entities to the list */
@@ -4864,7 +4864,7 @@ static int tg_throttle_down(struct task_group *tg, void *data)
 
 	/* group is entering throttled state, stop time */
 	if (!cfs_rq->throttle_count) {
-		cfs_rq->throttled_clock_task = rq_clock_task(rq);
+		cfs_rq->throttled_clock_task = rq_clock_pelt(rq);
 		list_del_leaf_cfs_rq(cfs_rq);
 	}
 	cfs_rq->throttle_count++;
@@ -5308,7 +5308,7 @@ static void sync_throttle(struct task_group *tg, int cpu)
 	pcfs_rq = tg->parent->cfs_rq[cpu];
 
 	cfs_rq->throttle_count = pcfs_rq->throttle_count;
-	cfs_rq->throttled_clock_task = rq_clock_task(cpu_rq(cpu));
+	cfs_rq->throttled_clock_task = rq_clock_pelt(cpu_rq(cpu));
 }
 
 /* conditionally throttle active cfs_rq's from put_prev_entity() */
-- 
2.35.1

[PATCH 2/2] sched/fair: Delete useless condition in tg_unthrottle_up()

[Chengming Zhou]

Fully decayed cfs_rq is impossible to have queued entities,
the first condition "!cfs_rq_is_decayed(cfs_rq)" is enough
to cover.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e6fa5d1141b4..17c13c38b1c2 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4850,7 +4850,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
 					     cfs_rq->throttled_clock_task;
 
 		/* Add cfs_rq with load or one or more already running entities to the list */
-		if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
+		if (!cfs_rq_is_decayed(cfs_rq))
 			list_add_leaf_cfs_rq(cfs_rq);
 	}
 
-- 
2.35.1

Re: [PATCH 2/2] sched/fair: Delete useless condition in tg_unthrottle_up()

[Benjamin Segall]

Chengming Zhou <zhouchengming@bytedance.com> writes:

> Fully decayed cfs_rq is impossible to have queued entities,
> the first condition "!cfs_rq_is_decayed(cfs_rq)" is enough
> to cover.

In particular, cfs_rq->load.weight is part of cfs_rq_is_decayed.

Reviewed-by: Ben Segall <bsegall@google.com>

>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
>  kernel/sched/fair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e6fa5d1141b4..17c13c38b1c2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4850,7 +4850,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
>  					     cfs_rq->throttled_clock_task;
>  
>  		/* Add cfs_rq with load or one or more already running entities to the list */
> -		if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
> +		if (!cfs_rq_is_decayed(cfs_rq))
>  			list_add_leaf_cfs_rq(cfs_rq);
>  	}

Re: [PATCH 2/2] sched/fair: Delete useless condition in tg_unthrottle_up()

[Vincent Guittot]

On Thu, 7 Apr 2022 at 23:00, Benjamin Segall <bsegall@google.com> wrote:
>
> Chengming Zhou <zhouchengming@bytedance.com> writes:
>
> > Fully decayed cfs_rq is impossible to have queued entities,
> > the first condition "!cfs_rq_is_decayed(cfs_rq)" is enough
> > to cover.
>
> In particular, cfs_rq->load.weight is part of cfs_rq_is_decayed.

Testing cfs_rq->load.weight is the key point because nothing prevent
to add a task with null load

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

>
> Reviewed-by: Ben Segall <bsegall@google.com>
>
> >
> > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> > ---
> >  kernel/sched/fair.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index e6fa5d1141b4..17c13c38b1c2 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4850,7 +4850,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
> >                                            cfs_rq->throttled_clock_task;
> >
> >               /* Add cfs_rq with load or one or more already running entities to the list */
> > -             if (!cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running)
> > +             if (!cfs_rq_is_decayed(cfs_rq))
> >                       list_add_leaf_cfs_rq(cfs_rq);
> >       }

Re: [PATCH 1/2] sched/fair: Fix cfs_rq_clock_pelt() for throttled cfs_rq

[Benjamin Segall]

Chengming Zhou <zhouchengming@bytedance.com> writes:

> Since commit 23127296889f ("sched/fair: Update scale invariance of PELT")
> change to use rq_clock_pelt() instead of rq_clock_task(), we should also
> use rq_clock_pelt() for throttled_clock_task_time and throttled_clock_task
> accounting.

I think this patch is indeed what we want, despite the confusing
interactions between pelt slowdown+skipping and throttle_clock_task
trying to freeze time.

I think it would be slightly better to rename
throttled_clock_task(_time) to be clock_pelt rather than clock_task, but
that's minor.

Reviewed-by: Ben Segall <bsegall@google.com>

>
> Fixes: 23127296889f ("sched/fair: Update scale invariance of PELT")
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
>  kernel/sched/fair.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d4bd299d67ab..e6fa5d1141b4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4846,7 +4846,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
>  
>  	cfs_rq->throttle_count--;
>  	if (!cfs_rq->throttle_count) {
> -		cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
> +		cfs_rq->throttled_clock_task_time += rq_clock_pelt(rq) -
>  					     cfs_rq->throttled_clock_task;
>  
>  		/* Add cfs_rq with load or one or more already running entities to the list */
> @@ -4864,7 +4864,7 @@ static int tg_throttle_down(struct task_group *tg, void *data)
>  
>  	/* group is entering throttled state, stop time */
>  	if (!cfs_rq->throttle_count) {
> -		cfs_rq->throttled_clock_task = rq_clock_task(rq);
> +		cfs_rq->throttled_clock_task = rq_clock_pelt(rq);
>  		list_del_leaf_cfs_rq(cfs_rq);
>  	}
>  	cfs_rq->throttle_count++;
> @@ -5308,7 +5308,7 @@ static void sync_throttle(struct task_group *tg, int cpu)
>  	pcfs_rq = tg->parent->cfs_rq[cpu];
>  
>  	cfs_rq->throttle_count = pcfs_rq->throttle_count;
> -	cfs_rq->throttled_clock_task = rq_clock_task(cpu_rq(cpu));
> +	cfs_rq->throttled_clock_task = rq_clock_pelt(cpu_rq(cpu));
>  }
>  
>  /* conditionally throttle active cfs_rq's from put_prev_entity() */

Re: [External] Re: [PATCH 1/2] sched/fair: Fix cfs_rq_clock_pelt() for throttled cfs_rq

[Chengming Zhou]

On 2022/4/8 04:57, Benjamin Segall wrote:
> Chengming Zhou <zhouchengming@bytedance.com> writes:
> 
>> Since commit 23127296889f ("sched/fair: Update scale invariance of PELT")
>> change to use rq_clock_pelt() instead of rq_clock_task(), we should also
>> use rq_clock_pelt() for throttled_clock_task_time and throttled_clock_task
>> accounting.
> 
> I think this patch is indeed what we want, despite the confusing
> interactions between pelt slowdown+skipping and throttle_clock_task
> trying to freeze time.
> 
> I think it would be slightly better to rename
> throttled_clock_task(_time) to be clock_pelt rather than clock_task, but
> that's minor.

It's a good suggestion, will do and send v2.

Thanks.

> 
> Reviewed-by: Ben Segall <bsegall@google.com>
> 
>>
>> Fixes: 23127296889f ("sched/fair: Update scale invariance of PELT")
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>>  kernel/sched/fair.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index d4bd299d67ab..e6fa5d1141b4 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4846,7 +4846,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
>>  
>>  	cfs_rq->throttle_count--;
>>  	if (!cfs_rq->throttle_count) {
>> -		cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
>> +		cfs_rq->throttled_clock_task_time += rq_clock_pelt(rq) -
>>  					     cfs_rq->throttled_clock_task;
>>  
>>  		/* Add cfs_rq with load or one or more already running entities to the list */
>> @@ -4864,7 +4864,7 @@ static int tg_throttle_down(struct task_group *tg, void *data)
>>  
>>  	/* group is entering throttled state, stop time */
>>  	if (!cfs_rq->throttle_count) {
>> -		cfs_rq->throttled_clock_task = rq_clock_task(rq);
>> +		cfs_rq->throttled_clock_task = rq_clock_pelt(rq);
>>  		list_del_leaf_cfs_rq(cfs_rq);
>>  	}
>>  	cfs_rq->throttle_count++;
>> @@ -5308,7 +5308,7 @@ static void sync_throttle(struct task_group *tg, int cpu)
>>  	pcfs_rq = tg->parent->cfs_rq[cpu];
>>  
>>  	cfs_rq->throttle_count = pcfs_rq->throttle_count;
>> -	cfs_rq->throttled_clock_task = rq_clock_task(cpu_rq(cpu));
>> +	cfs_rq->throttled_clock_task = rq_clock_pelt(cpu_rq(cpu));
>>  }
>>  
>>  /* conditionally throttle active cfs_rq's from put_prev_entity() */

Re: [PATCH 1/2] sched/fair: Fix cfs_rq_clock_pelt() for throttled cfs_rq

[Vincent Guittot]

On Thu, 7 Apr 2022 at 04:17, Chengming Zhou <zhouchengming@bytedance.com> wrote:
>
> Since commit 23127296889f ("sched/fair: Update scale invariance of PELT")
> change to use rq_clock_pelt() instead of rq_clock_task(), we should also
> use rq_clock_pelt() for throttled_clock_task_time and throttled_clock_task
> accounting.
>
> Fixes: 23127296889f ("sched/fair: Update scale invariance of PELT")
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

> ---
>  kernel/sched/fair.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d4bd299d67ab..e6fa5d1141b4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4846,7 +4846,7 @@ static int tg_unthrottle_up(struct task_group *tg, void *data)
>
>         cfs_rq->throttle_count--;
>         if (!cfs_rq->throttle_count) {
> -               cfs_rq->throttled_clock_task_time += rq_clock_task(rq) -
> +               cfs_rq->throttled_clock_task_time += rq_clock_pelt(rq) -
>                                              cfs_rq->throttled_clock_task;
>
>                 /* Add cfs_rq with load or one or more already running entities to the list */
> @@ -4864,7 +4864,7 @@ static int tg_throttle_down(struct task_group *tg, void *data)
>
>         /* group is entering throttled state, stop time */
>         if (!cfs_rq->throttle_count) {
> -               cfs_rq->throttled_clock_task = rq_clock_task(rq);
> +               cfs_rq->throttled_clock_task = rq_clock_pelt(rq);
>                 list_del_leaf_cfs_rq(cfs_rq);
>         }
>         cfs_rq->throttle_count++;
> @@ -5308,7 +5308,7 @@ static void sync_throttle(struct task_group *tg, int cpu)
>         pcfs_rq = tg->parent->cfs_rq[cpu];
>
>         cfs_rq->throttle_count = pcfs_rq->throttle_count;
> -       cfs_rq->throttled_clock_task = rq_clock_task(cpu_rq(cpu));
> +       cfs_rq->throttled_clock_task = rq_clock_pelt(cpu_rq(cpu));
>  }
>
>  /* conditionally throttle active cfs_rq's from put_prev_entity() */
> --
> 2.35.1
>