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 <[email protected]>
---
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
On Mon, 15 Jun 2020 at 16:20, Peng Wang <[email protected]> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
---
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
On 6/15/20 11:09 PM, Vincent Guittot wrote:
> On Mon, 15 Jun 2020 at 16:20, Peng Wang <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
On Tue, 16 Jun 2020 at 08:05, Peng Wang <[email protected]> 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 <[email protected]>
Reviewed-by: Vincent Guittot <[email protected]>
> ---
> 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
>
On Tue, Jun 16, 2020 at 02:31:15PM +0200, Vincent Guittot wrote:
> On Tue, 16 Jun 2020 at 08:05, Peng Wang <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Vincent Guittot <[email protected]>
Thanks!
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 <[email protected]>
> ---
> 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)
Hi,
> On Aug 12, 2020, at 12:55 AM, Dietmar Eggemann <[email protected]> wrote:
>
> 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 <[email protected]>
>> ---
>> 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)
Indeed, my local repo has been outdated, sorry for the interruption. :)
Thx.
Regards,
Jiang