[PATCH] sched/fair: Optimize dequeue_task_fair()

[PATCH] sched/fair: Optimize dequeue_task_fair()

[Peng Wang]

While looking at enqueue_task_fair and dequeue_task_fair, it occurred
to me that dequeue_task_fair can also be optimized as Vincent described
in commit 7d148be69e3a ("sched/fair: Optimize enqueue_task_fair()").

dequeue_throttle label can ensure se not to be NULL, and se is
always NULL when reaching root level.

Signed-off-by: Peng Wang <rocking@linux.alibaba.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 cbcb2f7..e6be952 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5614,10 +5614,10 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 
 	}
 
-dequeue_throttle:
-	if (!se)
-		sub_nr_running(rq, 1);
+	/* At this point se is NULL and we are at root level*/
+	sub_nr_running(rq, 1);
 
+dequeue_throttle:
 	/* balance early to pull high priority tasks */
 	if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
 		rq->next_balance = jiffies;
-- 
2.9.5

Re: [PATCH] sched/fair: Optimize dequeue_task_fair()

[Vincent Guittot]

On Mon, 15 Jun 2020 at 16:20, Peng Wang <rocking@linux.alibaba.com> wrote:
>
> While looking at enqueue_task_fair and dequeue_task_fair, it occurred
> to me that dequeue_task_fair can also be optimized as Vincent described
> in commit 7d148be69e3a ("sched/fair: Optimize enqueue_task_fair()").

good point

>
> dequeue_throttle label can ensure se not to be NULL, and se is
> always NULL when reaching root level.
>
> Signed-off-by: Peng Wang <rocking@linux.alibaba.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 cbcb2f7..e6be952 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5614,10 +5614,10 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>
>         }
>
> -dequeue_throttle:
> -       if (!se)
> -               sub_nr_running(rq, 1);
> +       /* At this point se is NULL and we are at root level*/
> +       sub_nr_running(rq, 1);
>
> +dequeue_throttle:
>         /* balance early to pull high priority tasks */
>         if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
>                 rq->next_balance = jiffies;

You can move the label here because sched_idle_rq() uses
rq->nr-running and rq->cfs.idle_h_nr_running so they will not change
if we jump to the label before reaching root level

> --
> 2.9.5
>

[PATCH v2] sched/fair: Optimize dequeue_task_fair()

[Peng Wang]

While looking at enqueue_task_fair and dequeue_task_fair, it occurred
to me that dequeue_task_fair can also be optimized as Vincent described
in commit 7d148be69e3a ("sched/fair: Optimize enqueue_task_fair()").

When encountering throttled cfs_rq, dequeue_throttle label can ensure
se not to be NULL, and rq->nr_running remains unchanged, so we can also
skip the early balance check.

Signed-off-by: Peng Wang <rocking@linux.alibaba.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 cbcb2f7..05242b7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5614,14 +5614,14 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 
 	}
 
-dequeue_throttle:
-	if (!se)
-		sub_nr_running(rq, 1);
+	/* At this point se is NULL and we are at root level*/
+	sub_nr_running(rq, 1);
 
 	/* balance early to pull high priority tasks */
 	if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
 		rq->next_balance = jiffies;
 
+dequeue_throttle:
 	util_est_dequeue(&rq->cfs, p, task_sleep);
 	hrtick_update(rq);
 }
-- 
2.9.5

Re: [PATCH] sched/fair: Optimize dequeue_task_fair()

[Peng Wang]

On 6/15/20 11:09 PM, Vincent Guittot wrote:
> On Mon, 15 Jun 2020 at 16:20, Peng Wang <rocking@linux.alibaba.com> wrote:
>>
>> While looking at enqueue_task_fair and dequeue_task_fair, it occurred
>> to me that dequeue_task_fair can also be optimized as Vincent described
>> in commit 7d148be69e3a ("sched/fair: Optimize enqueue_task_fair()").
> 
> good point
> 
>>
>> dequeue_throttle label can ensure se not to be NULL, and se is
>> always NULL when reaching root level.
>>
>> Signed-off-by: Peng Wang <rocking@linux.alibaba.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 cbcb2f7..e6be952 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5614,10 +5614,10 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>>
>>          }
>>
>> -dequeue_throttle:
>> -       if (!se)
>> -               sub_nr_running(rq, 1);
>> +       /* At this point se is NULL and we are at root level*/
>> +       sub_nr_running(rq, 1);
>>
>> +dequeue_throttle:
>>          /* balance early to pull high priority tasks */
>>          if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
>>                  rq->next_balance = jiffies;
> 
> You can move the label here because sched_idle_rq() uses
> rq->nr-running and rq->cfs.idle_h_nr_running so they will not change
> if we jump to the label before reaching root level

Yes, then we can also skip the early balance check.

> 
>> --
>> 2.9.5
>>

Re: [PATCH v2] sched/fair: Optimize dequeue_task_fair()

[Vincent Guittot]

On Tue, 16 Jun 2020 at 08:05, Peng Wang <rocking@linux.alibaba.com> wrote:
>
> While looking at enqueue_task_fair and dequeue_task_fair, it occurred
> to me that dequeue_task_fair can also be optimized as Vincent described
> in commit 7d148be69e3a ("sched/fair: Optimize enqueue_task_fair()").
>
> When encountering throttled cfs_rq, dequeue_throttle label can ensure
> se not to be NULL, and rq->nr_running remains unchanged, so we can also
> skip the early balance check.
>
> Signed-off-by: Peng Wang <rocking@linux.alibaba.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 cbcb2f7..05242b7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5614,14 +5614,14 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>
>         }
>
> -dequeue_throttle:
> -       if (!se)
> -               sub_nr_running(rq, 1);
> +       /* At this point se is NULL and we are at root level*/
> +       sub_nr_running(rq, 1);
>
>         /* balance early to pull high priority tasks */
>         if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
>                 rq->next_balance = jiffies;
>
> +dequeue_throttle:
>         util_est_dequeue(&rq->cfs, p, task_sleep);
>         hrtick_update(rq);
>  }
> --
> 2.9.5
>

Re: [PATCH v2] sched/fair: Optimize dequeue_task_fair()

[Peter Zijlstra]

On Tue, Jun 16, 2020 at 02:31:15PM +0200, Vincent Guittot wrote:
> On Tue, 16 Jun 2020 at 08:05, Peng Wang <rocking@linux.alibaba.com> wrote:
> >
> > While looking at enqueue_task_fair and dequeue_task_fair, it occurred
> > to me that dequeue_task_fair can also be optimized as Vincent described
> > in commit 7d148be69e3a ("sched/fair: Optimize enqueue_task_fair()").
> >
> > When encountering throttled cfs_rq, dequeue_throttle label can ensure
> > se not to be NULL, and rq->nr_running remains unchanged, so we can also
> > skip the early balance check.
> >
> > Signed-off-by: Peng Wang <rocking@linux.alibaba.com>
> 
> Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

Thanks!

Re: [PATCH] sched/fair: Optimize dequeue_task_fair()

[Dietmar Eggemann]

On 11/08/2020 10:43, Jiang Biao wrote:
> Similar optimization as what has been done in commit,
> 7d148be69e3a(sched/fair: Optimize enqueue_task_fair())
> 
> dequeue_task_fair jumps to dequeue_throttle label when cfs_rq_of(se) is
> throttled which means that se can't be NULL. We can move the label after
> the if (!se) statement and remove the if(!se) statment as se is always
> NULL when reaching this point.
> 
> Besides, trying to keep the same pattern with enqueue_task_fair can make
> it more readable.
> 
> Signed-off-by: Jiang Biao <benbjiang@tencent.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 04fa8dbcfa4d..cbbeafdfa8b7 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5618,10 +5618,10 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  
>  	}
>  
> -dequeue_throttle:
> -	if (!se)
> -		sub_nr_running(rq, 1);
> +	/* At this point se is NULL and we are at root level*/
> +	sub_nr_running(rq, 1);
>  
> +dequeue_throttle:
>  	/* balance early to pull high priority tasks */
>  	if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
>  		rq->next_balance = jiffies;

There is already a similar patch in master.

423d02e1463b - sched/fair: Optimize dequeue_task_fair() (2020-06-25 Peng
Wang)

[PATCH] sched/fair: Optimize dequeue_task_fair()

[Jiang Biao]

Similar optimization as what has been done in commit,
7d148be69e3a(sched/fair: Optimize enqueue_task_fair())

dequeue_task_fair jumps to dequeue_throttle label when cfs_rq_of(se) is
throttled which means that se can't be NULL. We can move the label after
the if (!se) statement and remove the if(!se) statment as se is always
NULL when reaching this point.

Besides, trying to keep the same pattern with enqueue_task_fair can make
it more readable.

Signed-off-by: Jiang Biao <benbjiang@tencent.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 04fa8dbcfa4d..cbbeafdfa8b7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5618,10 +5618,10 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 
 	}
 
-dequeue_throttle:
-	if (!se)
-		sub_nr_running(rq, 1);
+	/* At this point se is NULL and we are at root level*/
+	sub_nr_running(rq, 1);
 
+dequeue_throttle:
 	/* balance early to pull high priority tasks */
 	if (unlikely(!was_sched_idle && sched_idle_rq(rq)))
 		rq->next_balance = jiffies;
-- 
2.21.0