sched: fix incorrect wait time and wait count statistics

PATCH Superseded Joonwoo Park 3 participants 13 messages Oct 25, 2015 – Nov 23, 2015

Subsystems (2)

Arm Qualcomm mailing list Scheduler

lore

Share & tools

lore

Joonwoo Park

author

2015-10-25 05:23 UTC #0 lore raw

PATCH 1/1 sched: fix incorrect wait time and wait count statistics

At present scheduler resets task's wait start timestamp when the task migrates to another rq. This misleads scheduler itself into reporting less wait time than actual by omitting time spent for waiting prior to migration and also more wait count than actual by counting migration as wait end event which can be seen by trace or /proc/<pid>/sched with CONFIG_SCHEDSTATS=y.

Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc [email protected]

Signed-off-by Joonwoo Park <[email protected]>

---
 kernel/sched/core.c  |  4 ++--
 kernel/sched/fair.c  | 41 ++++++++++++++++++++++++++++-------------
 kernel/sched/sched.h |  2 ++
 3 files changed, 32 insertions(+), 15 deletions(-)

kernel/sched/core.c +2 -2

index bcd214e..4f20895 100644
@@ -1069,7 +1069,7 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
+	dequeue_task(rq, p, DEQUEUE_MIGRATING);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1079,7 +1079,7 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
 	p->on_rq = TASK_ON_RQ_QUEUED;
-	enqueue_task(rq, p, 0);
+	enqueue_task(rq, p, ENQUEUE_MIGRATING);
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

kernel/sched/fair.c +28 ~~-13~~

111 lines not on this page · raw message

kernel/sched/sched.h +2 -0

index 6d2a119..c1ac118 100644
@@ -1165,8 +1165,10 @@ static const u32 prio_to_wmult[40] = {
 #define ENQUEUE_WAKING		0
 #endif
 #define ENQUEUE_REPLENISH	8
+#define ENQUEUE_MIGRATING	16
 
 #define DEQUEUE_SLEEP		1
+#define DEQUEUE_MIGRATING	2
 
 #define RETRY_TASK		((void *)-1UL)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Peter Zijlstra

maintainer

2015-10-25 10:08 UTC #1 lore raw

Quoted 7 lines · Joonwoo Park

At present scheduler resets task's wait start timestamp when the task migrates to another rq. This misleads scheduler itself into reporting less wait time than actual by omitting time spent for waiting prior to migration and also more wait count than actual by counting migration as wait end event which can be seen by trace or /proc/<pid>/sched with CONFIG_SCHEDSTATS=y.

Have you seen:

1de64443d755 ("sched/core: Fix task and run queue sched_info::run_delay inconsistencies")

Peter Zijlstra

maintainer

2015-10-25 10:26 UTC #2 lore raw

In reply to Joonwoo Park

Quoted 11 lines · Joonwoo Park

kernel/sched/core.c +1 -1

@@ -1069,7 +1069,7 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
+	dequeue_task(rq, p, DEQUEUE_MIGRATING);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

kernel/sched/core.c static void detach_task(struct task_struct *p, struct lb_env *env) · line 5677

Quoted 8 lines

kernel/sched/core.c +1 -1

@@ -5656,7 +5671,7 @@ static void detach_task(struct task_struct *p, struct lb_env *env)
 {
 	lockdep_assert_held(&env->src_rq->lock);
 
-	deactivate_task(env->src_rq, p, 0);
+	deactivate_task(env->src_rq, p, DEQUEUE_MIGRATING);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
 	set_task_cpu(p, env->dst_cpu);
 }

Also note that on both sites we also set TASK_ON_RQ_MIGRATING -- albeit late. Can't you simply set that earlier (and back to QUEUED later) and test for task_on_rq_migrating() instead of blowing up the fastpath like you did?

Joonwoo Park

author

2015-10-27 01:44 UTC #3 lore raw

Quoted 15 lines · Peter Zijlstra

Quoted 14 lines · Joonwoo Park

(quoted diff) +1 -1

@@ -1069,7 +1069,7 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
+	dequeue_task(rq, p, DEQUEUE_MIGRATING);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -5656,7 +5671,7 @@ static void detach_task(struct task_struct *p, struct lb_env *env)
 {
 	lockdep_assert_held(&env->src_rq->lock);

Quoted 11 lines

Review static void detach_task(struct task_struct *p, struct lb_env *env) · line 5677
Quoted 5 lines
(quoted diff) +1 -1
-	deactivate_task(env->src_rq, p, 0);
+	deactivate_task(env->src_rq, p, DEQUEUE_MIGRATING);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
 	set_task_cpu(p, env->dst_cpu);
 }
Also note that on both sites we also set TASK_ON_RQ_MIGRATING -- albeit late. Can't you simply set that earlier (and back to QUEUED later) and test for task_on_rq_migrating() instead of blowing up the fastpath like you did?

Yes it's doable. I also find it's much simpler.
Please find patch v2. I verified v2 does same job as v1 by comparing sched_stat_wait time with sched_switch - sched_wakeup timestamp.

Thanks,
Joonwoo

Quoted 1 line

From 98d615d46211a90482a0f9b7204265c54bba8520 Mon Sep 17 00:00:00 2001

From: Joonwoo Park <[email protected]>
Date: Mon, 26 Oct 2015 16:37:47 -0700
Subject: [PATCH v2] sched: fix incorrect wait time and wait count statistics

Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

In order to determine whether task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.

To: Ingo Molnar <[email protected]>
To: Peter Zijlstra <[email protected]>

Cc [email protected]

Signed-off-by Joonwoo Park <[email protected]>

---
Changes in v2:
* Set p->on_rq = TASK_ON_RQ_MIGRATING while doing migration dequeue/enqueue and check whether task's migrating with task_on_rq_migrating().

 kernel/sched/core.c |  4 ++--
 kernel/sched/fair.c | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/sched/core.c +2 -2

index bcd214e..d9e4ad5 100644
@@ -1069,8 +1069,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1078,8 +1078,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

kernel/sched/fair.c +14 -3

index 9a5e60f..7609576 100644
@@ -740,7 +740,11 @@ static void update_curr_fair(struct rq *rq)
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	schedstat_set(se->statistics.wait_start, rq_clock(rq_of(cfs_rq)));
+	schedstat_set(se->statistics.wait_start,
+		task_on_rq_migrating(task_of(se)) &&
+		likely(rq_clock(rq_of(cfs_rq)) > se->statistics.wait_start) ?
+		rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start :
+		rq_clock(rq_of(cfs_rq)));
 }
 
 /*

@@ -759,6 +763,13 @@ static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	if (task_on_rq_migrating(task_of(se))) {
+		schedstat_set(se->statistics.wait_start,
+			      rq_clock(rq_of(cfs_rq)) -
+			      se->statistics.wait_start);
+		return;
+	}
+
 	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
 	schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);

@@ -5656,8 +5667,8 @@ static void detach_task(struct task_struct *p, struct lb_env *env)
 {
 	lockdep_assert_held(&env->src_rq->lock);
 
-	deactivate_task(env->src_rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	deactivate_task(env->src_rq, p, 0);
 	set_task_cpu(p, env->dst_cpu);
 }

@@ -5790,8 +5801,8 @@ static void attach_task(struct rq *rq, struct task_struct *p)
 	lockdep_assert_held(&rq->lock);
 
 	BUG_ON(task_rq(p) != rq);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	activate_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 }

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Peter Zijlstra

maintainer

2015-10-28 00:10 UTC #4 lore raw

(Excessive quoting for Olav)

Quoted 25 lines · Joonwoo Park

Quoted 5 lines · Peter Zijlstra
Also note that on both sites we also set TASK_ON_RQ_MIGRATING -- albeit late. Can't you simply set that earlier (and back to QUEUED later) and test for task_on_rq_migrating() instead of blowing up the fastpath like you did?
Yes it's doable. I also find it's much simpler.
From 98d615d46211a90482a0f9b7204265c54bba8520 Mon Sep 17 00:00:00 2001
From: Joonwoo Park <[email protected]>
Date: Mon, 26 Oct 2015 16:37:47 -0700
Subject: [PATCH v2] sched: fix incorrect wait time and wait count statistics
At present scheduler resets task's wait start timestamp when the task migrates to another rq. This misleads scheduler itself into reporting less wait time than actual by omitting time spent for waiting prior to migration and also more wait count than actual by counting migration as wait end event which can be seen by trace or /proc/<pid>/sched with CONFIG_SCHEDSTATS=y.
Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

Quoted 8 lines

In order to determine whether task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.
To: Ingo Molnar <[email protected]>
To: Peter Zijlstra <[email protected]>
Cc [email protected]
Signed-off-by Joonwoo Park <[email protected]>
---

So now that you rely on TASK_ON_RQ_MIGRATING; I think you missed one place that can migrate sched_fair tasks and doesn't set it.

Olav recently did a patch adding TASK_ON_RQ_MIGRATING to _every_ migration path, but that is (still) somewhat overkill. With your changes we need it for sched_fair though.

So I think you need to change __migrate_swap_task(), which is used by the NUMA scheduling to swap two running tasks.

Also, it might be prudent to extend the CONFIG_SCHED_DEBUG ifdef in set_task_cpu() to test for this new requirement:

	WARN_ON_ONCE(p->state == TASK_RUNNING &&
	             p->class == &fair_sched_class &&
		     p->on_rq != TASK_ON_RQ_MIGRATING);

Quoted 33 lines

 kernel/sched/core.c |  4 ++--
 kernel/sched/fair.c | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/sched/core.c +2 -2

index bcd214e..d9e4ad5 100644
@@ -1069,8 +1069,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1078,8 +1078,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

kernel/sched/fair.c +0 -0

index 9a5e60f..7609576 100644

kernel/sched/fair.c static void update_curr_fair(struct rq *rq) · line 747

Quoted 9 lines

kernel/sched/fair.c +5 -1

@@ -740,7 +740,11 @@ static void update_curr_fair(struct rq *rq)
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	schedstat_set(se->statistics.wait_start, rq_clock(rq_of(cfs_rq)));
+	schedstat_set(se->statistics.wait_start,
+		task_on_rq_migrating(task_of(se)) &&
+		likely(rq_clock(rq_of(cfs_rq)) > se->statistics.wait_start) ?
+		rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start :
+		rq_clock(rq_of(cfs_rq)));

So I get that you want to avoid emitting code for !SCHEDSTATS; but that is rather unreadable. Either use the GCC stmt-expr or wrap the lot in #ifdef.

kernel/sched/fair.c static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se) · line 766

Quoted 8 lines

kernel/sched/fair.c +1 -0

 }
 
 /*

@@ -759,6 +763,13 @@ static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	if (task_on_rq_migrating(task_of(se))) {

Maybe add a comment that we adjust the wait_start time-base and will rebase on the new rq_clock in update_stats_wait_start().

Quoted 28 lines

kernel/sched/fair.c +8 -2

+		schedstat_set(se->statistics.wait_start,
+			      rq_clock(rq_of(cfs_rq)) -
+			      se->statistics.wait_start);
+		return;
+	}
+
 	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
 	schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);

@@ -5656,8 +5667,8 @@ static void detach_task(struct task_struct *p, struct lb_env *env)
 {
 	lockdep_assert_held(&env->src_rq->lock);
 
-	deactivate_task(env->src_rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	deactivate_task(env->src_rq, p, 0);
 	set_task_cpu(p, env->dst_cpu);
 }

@@ -5790,8 +5801,8 @@ static void attach_task(struct rq *rq, struct task_struct *p)
 	lockdep_assert_held(&rq->lock);
 
 	BUG_ON(task_rq(p) != rq);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	activate_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 }

Joonwoo Park

author

2015-10-28 02:41 UTC #5 lore raw

Quoted 41 lines · Peter Zijlstra

(Excessive quoting for Olav)
Quoted 25 lines · Joonwoo Park
Quoted 5 lines · Peter Zijlstra
Also note that on both sites we also set TASK_ON_RQ_MIGRATING -- albeit late. Can't you simply set that earlier (and back to QUEUED later) and test for task_on_rq_migrating() instead of blowing up the fastpath like you did?
Yes it's doable. I also find it's much simpler.
From 98d615d46211a90482a0f9b7204265c54bba8520 Mon Sep 17 00:00:00 2001
From: Joonwoo Park <[email protected]>
Date: Mon, 26 Oct 2015 16:37:47 -0700
Subject: [PATCH v2] sched: fix incorrect wait time and wait count statistics
At present scheduler resets task's wait start timestamp when the task migrates to another rq. This misleads scheduler itself into reporting less wait time than actual by omitting time spent for waiting prior to migration and also more wait count than actual by counting migration as wait end event which can be seen by trace or /proc/<pid>/sched with CONFIG_SCHEDSTATS=y.
Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.
Quoted 8 lines
In order to determine whether task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.
To: Ingo Molnar <[email protected]>
To: Peter Zijlstra <[email protected]>
Cc [email protected]
Signed-off-by Joonwoo Park <[email protected]>
---
So now that you rely on TASK_ON_RQ_MIGRATING; I think you missed one place that can migrate sched_fair tasks and doesn't set it.

Quoted 6 lines

Olav recently did a patch adding TASK_ON_RQ_MIGRATING to _every_ migration path, but that is (still) somewhat overkill. With your changes we need it for sched_fair though.
So I think you need to change __migrate_swap_task(), which is used by the NUMA scheduling to swap two running tasks.

Oh yes... __migrate_swap_task() can migrate fair class task so I should mark as TASK_ON_RQ_MIGRATING. I will fix this in subsequent patch.

Quoted 8 lines

Also, it might be prudent to extend the CONFIG_SCHED_DEBUG ifdef in set_task_cpu() to test for this new requirement:
	WARN_ON_ONCE(p->state == TASK_RUNNING &&
	             p->class == &fair_sched_class &&
		     p->on_rq != TASK_ON_RQ_MIGRATING);

I conceived to argue that if we had Olav's change (with revised marking order) dummy like myself don't need to worry about stale on_rq state and probably didn't make mistake like I did with __migrate_swap_task().
But I don't think I can argue for that reason anymore as my patch will set TASK_ON_RQ_MIGRATING for all migration paths for the fair class tasks at least and moreover above macro you suggested would fortify the new requirement.

I will add that macro.

I recall Olav had some other reason for his patch though.

Quoted 38 lines

 kernel/sched/core.c |  4 ++--
 kernel/sched/fair.c | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

kernel/sched/core.c +2 -2

index bcd214e..d9e4ad5 100644
@@ -1069,8 +1069,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1078,8 +1078,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

kernel/sched/fair.c +1 -1

index 9a5e60f..7609576 100644
@@ -740,7 +740,11 @@ static void update_curr_fair(struct rq *rq)
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	schedstat_set(se->statistics.wait_start, rq_clock(rq_of(cfs_rq)));
+	schedstat_set(se->statistics.wait_start,

Quoted 9 lines

kernel/sched/fair.c static void update_curr_fair(struct rq *rq) · line 747
Quoted 4 lines
kernel/sched/fair.c +4 -0
+		task_on_rq_migrating(task_of(se)) &&
+		likely(rq_clock(rq_of(cfs_rq)) > se->statistics.wait_start) ?
+		rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start :
+		rq_clock(rq_of(cfs_rq)));
So I get that you want to avoid emitting code for !SCHEDSTATS; but that is rather unreadable. Either use the GCC stmt-expr or wrap the lot in #ifdef.

Will do.

Quoted 12 lines

kernel/sched/fair.c static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se) · line 766
Quoted 8 lines
kernel/sched/fair.c +1 -0
 }
 
 /*
@@ -759,6 +763,13 @@ static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	if (task_on_rq_migrating(task_of(se))) {
Maybe add a comment that we adjust the wait_start time-base and will rebase on the new rq_clock in update_stats_wait_start().

Will do.

Thanks,
Joonwoo

Quoted 28 lines

kernel/sched/fair.c +8 -2

+		schedstat_set(se->statistics.wait_start,
+			      rq_clock(rq_of(cfs_rq)) -
+			      se->statistics.wait_start);
+		return;
+	}
+
 	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
 	schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);

@@ -5656,8 +5667,8 @@ static void detach_task(struct task_struct *p, struct lb_env *env)
 {
 	lockdep_assert_held(&env->src_rq->lock);
 
-	deactivate_task(env->src_rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	deactivate_task(env->src_rq, p, 0);
 	set_task_cpu(p, env->dst_cpu);
 }

@@ -5790,8 +5801,8 @@ static void attach_task(struct rq *rq, struct task_struct *p)
 	lockdep_assert_held(&rq->lock);
 
 	BUG_ON(task_rq(p) != rq);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	activate_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 }

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Joonwoo Park

author

2015-10-28 04:46 UTC #6 lore raw

PATCH v4 1/1 sched: fix incorrect wait time and wait count statistics

Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

In order to determine whether fair task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.

To: Ingo Molnar <[email protected]>
To: Peter Zijlstra <[email protected]>

Cc [email protected]

Signed-off-by Joonwoo Park <[email protected]>

---
Changes in v2:
* Set p->on_rq = TASK_ON_RQ_MIGRATING while doing migration dequeue/enqueue
and check whether task's migrating with task_on_rq_migrating().
Changes in v3:
* Fixed "WARNING: CPU: 0 PID: 3 at kernel/sched/fair.c:260 update_stats_wait_end+0x23/0x30()" caught by Intel kernel test robot.
Changes in v4:
* Made __migrate_swap_task() to set p->on_rq = TASK_ON_RQ_MIGRATING.
* Added WARN_ON_ONCE() inside CONFIG_SCHED_DEBUG.
* Added comments.
* Cleanup with ifdefy.

 kernel/sched/core.c | 15 +++++++++++--
 kernel/sched/fair.c | 62 ++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 58 insertions(+), 19 deletions(-)

kernel/sched/core.c +13 -2

index bcd214e..1ddbabc 100644
@@ -1069,8 +1069,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1078,8 +1078,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

@@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
 			!p->on_rq);
 
+	/*
+	 * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
+	 * because schedstat_wait_{start,end} rebase migrating task's wait_start
+	 * time relying on p->on_rq.
+	 */
+	WARN_ON_ONCE(p->state == TASK_RUNNING &&
+		     p->sched_class == &fair_sched_class &&
+		     (p->on_rq && !task_on_rq_migrating(p)));
+
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * The caller should hold either p->pi_lock or rq->lock, when changing

@@ -1308,9 +1317,11 @@ static void __migrate_swap_task(struct task_struct *p, int cpu)
 		src_rq = task_rq(p);
 		dst_rq = cpu_rq(cpu);
 
+		p->on_rq = TASK_ON_RQ_MIGRATING;
 		deactivate_task(src_rq, p, 0);
 		set_task_cpu(p, cpu);
 		activate_task(dst_rq, p, 0);
+		p->on_rq = TASK_ON_RQ_QUEUED;
 		check_preempt_curr(dst_rq, p, 0);
 	} else {
 		/*

kernel/sched/fair.c +45 ~~-17~~

102 lines not on this page · raw message

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Peter Zijlstra

maintainer

2015-11-06 13:57 UTC #7 lore raw

kernel/sched/core.c void set_task_cpu(struct task_struct *p, unsigned int new_cpu) · line 1283

Quoted 13 lines · Joonwoo Park

kernel/sched/core.c +9 -0

@@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
 			!p->on_rq);
 
+	/*
+	 * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
+	 * because schedstat_wait_{start,end} rebase migrating task's wait_start
+	 * time relying on p->on_rq.
+	 */
+	WARN_ON_ONCE(p->state == TASK_RUNNING &&
+		     p->sched_class == &fair_sched_class &&
+		     (p->on_rq && !task_on_rq_migrating(p)));
+

Why do we have to test p->on_rq? Would not ->state == RUNNING imply that?

Quoted 11 lines

kernel/sched/fair.c +3 -0

+++ b/kernel/sched/fair.c

@@ -737,41 +737,69 @@ static void update_curr_fair(struct rq *rq)
 	update_curr(cfs_rq_of(&rq->curr->se));
 }
 
+#ifdef CONFIG_SCHEDSTATS
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	u64 wait_start = rq_clock(rq_of(cfs_rq));

kernel/sched/fair.c static void update_curr_fair(struct rq *rq) · line 755

Quoted 10 lines

kernel/sched/fair.c +5 -0

+	if (entity_is_task(se) && task_on_rq_migrating(task_of(se)) &&
+	    likely(wait_start > se->statistics.wait_start))
+		wait_start -= se->statistics.wait_start;
+
+	schedstat_set(se->statistics.wait_start, wait_start);
 }
 
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {

Since this is now all under CONFIG_SCHEDSTAT, would it not make sense to do something like:

	u64 now = rq_clock(rq_of(cfs_rq));

to avoid the endless calling of that function?

Also, for that very same reason; would it not make sense to drop the schedstat_set() usage below, that would greatly enhance readability.

Quoted 11 lines

kernel/sched/fair.c +11 -0

+	if (entity_is_task(se) && task_on_rq_migrating(task_of(se))) {
+		/*
+		 * Preserve migrating task's wait time so wait_start time stamp
+		 * can be adjusted to accumulate wait time prior to migration.
+		 */
+		schedstat_set(se->statistics.wait_start,
+			      rq_clock(rq_of(cfs_rq)) -
+			      se->statistics.wait_start);
+		return;
+	}
+

kernel/sched/fair.c static void update_curr_fair(struct rq *rq) · line 776

Quoted 10 lines

kernel/sched/fair.c +1 -0

 	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
 	schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);
 	schedstat_set(se->statistics.wait_sum, se->statistics.wait_sum +
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
+
 	if (entity_is_task(se)) {
 		trace_sched_stat_wait(task_of(se),
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
 	}

Is there no means of collapsing the two 'entity_is_task()' branches?

Quoted 13 lines

kernel/sched/fair.c +11 -0

 	schedstat_set(se->statistics.wait_start, 0);
 }
+#else
+static inline void
+update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+}
+
+static inline void
+update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+}
+#endif

Joonwoo Park

author

2015-11-07 02:41 UTC #8 lore raw

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines · Joonwoo Park

kernel/sched/core.c +0 -0

@@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
 			!p->on_rq);

Quoted 13 lines

kernel/sched/core.c void set_task_cpu(struct task_struct *p, unsigned int new_cpu) · line 1283
Quoted 9 lines
kernel/sched/core.c +9 -0
+	/*
+	 * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
+	 * because schedstat_wait_{start,end} rebase migrating task's wait_start
+	 * time relying on p->on_rq.
+	 */
+	WARN_ON_ONCE(p->state == TASK_RUNNING &&
+		     p->sched_class == &fair_sched_class &&
+		     (p->on_rq && !task_on_rq_migrating(p)));
+
Why do we have to test p->on_rq? Would not ->state == RUNNING imply that?

sched_fork() sets p->state = RUNNING before changing task cpu. Please let me know if you got better idea.

Quoted 25 lines

Quoted 11 lines

kernel/sched/fair.c +3 -0

+++ b/kernel/sched/fair.c

@@ -737,41 +737,69 @@ static void update_curr_fair(struct rq *rq)
 	update_curr(cfs_rq_of(&rq->curr->se));
 }
 
+#ifdef CONFIG_SCHEDSTATS
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	u64 wait_start = rq_clock(rq_of(cfs_rq));

kernel/sched/fair.c static void update_curr_fair(struct rq *rq) · line 755

Quoted 10 lines

kernel/sched/fair.c +5 -0

+	if (entity_is_task(se) && task_on_rq_migrating(task_of(se)) &&
+	    likely(wait_start > se->statistics.wait_start))
+		wait_start -= se->statistics.wait_start;
+
+	schedstat_set(se->statistics.wait_start, wait_start);
 }
 
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {

Since this is now all under CONFIG_SCHEDSTAT, would it not make sense to do something like:

Quoted 7 lines

	u64 now = rq_clock(rq_of(cfs_rq));
to avoid the endless calling of that function?
Also, for that very same reason; would it not make sense to drop the schedstat_set() usage below, that would greatly enhance readability.

Agreed.

Quoted 15 lines

kernel/sched/fair.c +11 -0

+	if (entity_is_task(se) && task_on_rq_migrating(task_of(se))) {
+		/*
+		 * Preserve migrating task's wait time so wait_start time stamp
+		 * can be adjusted to accumulate wait time prior to migration.
+		 */
+		schedstat_set(se->statistics.wait_start,
+			      rq_clock(rq_of(cfs_rq)) -
+			      se->statistics.wait_start);
+		return;
+	}
+
 	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
 	schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);
 	schedstat_set(se->statistics.wait_sum, se->statistics.wait_sum +

Quoted 9 lines

kernel/sched/fair.c static void update_curr_fair(struct rq *rq) · line 776
Quoted 6 lines
kernel/sched/fair.c +1 -0
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
+
 	if (entity_is_task(se)) {
 		trace_sched_stat_wait(task_of(se),
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
 	}
Is there no means of collapsing the two 'entity_is_task()' branches?

Agreed. Will spin v5 with these clean up.

Thanks,
Joonwoo

Quoted 13 lines

kernel/sched/fair.c +11 -0

 	schedstat_set(se->statistics.wait_start, 0);
 }
+#else
+static inline void
+update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+}
+
+static inline void
+update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+}
+#endif

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Peter Zijlstra

maintainer

2015-11-09 10:32 UTC #9 lore raw

Quoted 20 lines · Joonwoo Park

Quoted 5 lines · Peter Zijlstra
Quoted 4 lines · Joonwoo Park
kernel/sched/core.c +0 -0
@@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
 			!p->on_rq);
 
Quoted 13 lines
kernel/sched/core.c void set_task_cpu(struct task_struct *p, unsigned int new_cpu) · line 1283
Quoted 9 lines
kernel/sched/core.c +9 -0
+	/*
+	 * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
+	 * because schedstat_wait_{start,end} rebase migrating task's wait_start
+	 * time relying on p->on_rq.
+	 */
+	WARN_ON_ONCE(p->state == TASK_RUNNING &&
+		     p->sched_class == &fair_sched_class &&
+		     (p->on_rq && !task_on_rq_migrating(p)));
+
Why do we have to test p->on_rq? Would not ->state == RUNNING imply that?

Quoted 2 lines

sched_fork() sets p->state = RUNNING before changing task cpu. Please let me know if you got better idea.

Ah, indeed. OK.

Joonwoo Park

author

2015-11-13 03:39 UTC #10 lore raw

Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

In order to determine whether task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.

To: Ingo Molnar <[email protected]>
To: Peter Zijlstra <[email protected]>

Cc [email protected]

Signed-off-by Joonwoo Park <[email protected]>

 kernel/sched/core.c | 15 ++++++++++--
 kernel/sched/fair.c | 67 +++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 60 insertions(+), 22 deletions(-)

kernel/sched/core.c +13 -2

index bcd214e..1ddbabc 100644
@@ -1069,8 +1069,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1078,8 +1078,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

@@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
 			!p->on_rq);
 
+	/*
+	 * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
+	 * because schedstat_wait_{start,end} rebase migrating task's wait_start
+	 * time relying on p->on_rq.
+	 */
+	WARN_ON_ONCE(p->state == TASK_RUNNING &&
+		     p->sched_class == &fair_sched_class &&
+		     (p->on_rq && !task_on_rq_migrating(p)));
+
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * The caller should hold either p->pi_lock or rq->lock, when changing

@@ -1308,9 +1317,11 @@ static void __migrate_swap_task(struct task_struct *p, int cpu)
 		src_rq = task_rq(p);
 		dst_rq = cpu_rq(cpu);
 
+		p->on_rq = TASK_ON_RQ_MIGRATING;
 		deactivate_task(src_rq, p, 0);
 		set_task_cpu(p, cpu);
 		activate_task(dst_rq, p, 0);
+		p->on_rq = TASK_ON_RQ_QUEUED;
 		check_preempt_curr(dst_rq, p, 0);
 	} else {
 		/*

kernel/sched/fair.c +47 ~~-20~~

98 lines not on this page · raw message

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

tip-bot for Joonwoo Park

merge bot

2015-11-23 16:21 UTC #11 lore raw

[tip:sched/core] sched/core: Fix incorrect wait time and wait count statistics

sched/core: Fix incorrect wait time and wait count statistics

Commit-ID:  3ea94de15ce9f3a217f6d0a7e9e0f48388902bb7
Gitweb:     http://git.kernel.org/tip/3ea94de15ce9f3a217f6d0a7e9e0f48388902bb7
Author:     Joonwoo Park <[email protected]>
AuthorDate: Thu, 12 Nov 2015 19:38:54 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:17 +0100

sched/core: Fix incorrect wait time and wait count statistics

Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

In order to determine whether task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.

Signed-off-by Joonwoo Park <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/core.c | 15 ++++++++++--
 kernel/sched/fair.c | 67 +++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 60 insertions(+), 22 deletions(-)

kernel/sched/core.c +13 -2

index 4d568ac..1b7cb5e 100644
@@ -1071,8 +1071,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1080,8 +1080,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

@@ -1274,6 +1274,15 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 	WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
 			!p->on_rq);
 
+	/*
+	 * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
+	 * because schedstat_wait_{start,end} rebase migrating task's wait_start
+	 * time relying on p->on_rq.
+	 */
+	WARN_ON_ONCE(p->state == TASK_RUNNING &&
+		     p->sched_class == &fair_sched_class &&
+		     (p->on_rq && !task_on_rq_migrating(p)));
+
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * The caller should hold either p->pi_lock or rq->lock, when changing

@@ -1310,9 +1319,11 @@ static void __migrate_swap_task(struct task_struct *p, int cpu)
 		src_rq = task_rq(p);
 		dst_rq = cpu_rq(cpu);
 
+		p->on_rq = TASK_ON_RQ_MIGRATING;
 		deactivate_task(src_rq, p, 0);
 		set_task_cpu(p, cpu);
 		activate_task(dst_rq, p, 0);
+		p->on_rq = TASK_ON_RQ_QUEUED;
 		check_preempt_curr(dst_rq, p, 0);
 	} else {
 		/*

kernel/sched/fair.c +47 ~~-20~~

98 lines not on this page · raw message

Joonwoo Park

author

2015-10-27 19:17 UTC #12 lore raw

PATCH v3 1/1 sched: fix incorrect wait time and wait count statistics

In reply to Joonwoo Park

Carry forward migrating task's wait time prior to migration and don't count migration as a wait end event to fix such statistics error.

In order to determine whether task is migrating mark task->on_rq with TASK_ON_RQ_MIGRATING while dequeuing and enqueuing due to migration.

To: Ingo Molnar <[email protected]>
To: Peter Zijlstra <[email protected]>

Cc [email protected]

Signed-off-by Joonwoo Park <[email protected]>

 kernel/sched/core.c |  4 ++--
 kernel/sched/fair.c | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 7 deletions(-)

kernel/sched/core.c +2 -2

index bcd214e..d9e4ad5 100644
@@ -1069,8 +1069,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 {
 	lockdep_assert_held(&rq->lock);
 
-	dequeue_task(rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	dequeue_task(rq, p, 0);
 	set_task_cpu(p, new_cpu);
 	raw_spin_unlock(&rq->lock);

@@ -1078,8 +1078,8 @@ static struct rq *move_queued_task(struct rq *rq, struct task_struct *p, int new
 
 	raw_spin_lock(&rq->lock);
 	BUG_ON(task_cpu(p) != new_cpu);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	enqueue_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 
 	return rq;

kernel/sched/fair.c +22 -5

index 9a5e60f..4c174a1 100644
@@ -740,7 +740,11 @@ static void update_curr_fair(struct rq *rq)
 static inline void
 update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	schedstat_set(se->statistics.wait_start, rq_clock(rq_of(cfs_rq)));
+	schedstat_set(se->statistics.wait_start,
+		entity_is_task(se) && task_on_rq_migrating(task_of(se)) &&
+		likely(rq_clock(rq_of(cfs_rq)) > se->statistics.wait_start) ?
+		rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start :
+		rq_clock(rq_of(cfs_rq)));
 }
 
 /*

@@ -756,22 +760,35 @@ static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
 		update_stats_wait_start(cfs_rq, se);
 }
 
+#ifdef CONFIG_SCHEDSTATS
 static void
 update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
+	if (entity_is_task(se) && task_on_rq_migrating(task_of(se))) {
+		schedstat_set(se->statistics.wait_start,
+			      rq_clock(rq_of(cfs_rq)) -
+			      se->statistics.wait_start);
+		return;
+	}
+
 	schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
 	schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);
 	schedstat_set(se->statistics.wait_sum, se->statistics.wait_sum +
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
-#ifdef CONFIG_SCHEDSTATS
+
 	if (entity_is_task(se)) {
 		trace_sched_stat_wait(task_of(se),
 			rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
 	}
-#endif
 	schedstat_set(se->statistics.wait_start, 0);
 }
+#else
+static inline void
+update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+}
+#endif
 
 static inline void
 update_stats_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se)

@@ -5656,8 +5673,8 @@ static void detach_task(struct task_struct *p, struct lb_env *env)
 {
 	lockdep_assert_held(&env->src_rq->lock);
 
-	deactivate_task(env->src_rq, p, 0);
 	p->on_rq = TASK_ON_RQ_MIGRATING;
+	deactivate_task(env->src_rq, p, 0);
 	set_task_cpu(p, env->dst_cpu);
 }

@@ -5790,8 +5807,8 @@ static void attach_task(struct rq *rq, struct task_struct *p)
 	lockdep_assert_held(&rq->lock);
 
 	BUG_ON(task_rq(p) != rq);
-	p->on_rq = TASK_ON_RQ_QUEUED;
 	activate_task(rq, p, 0);
+	p->on_rq = TASK_ON_RQ_QUEUED;
 	check_preempt_curr(rq, p, 0);
 }

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation