Skip to contentSkip to conversation

sched/deadline: support dl task migration during cpu hotplug

PATCH RESEND Superseded Wanpeng Li 4 participants 17 messages Mar 13, 2015 – Mar 31, 2015 Scheduler

v10 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Wanpeng Li

author
#0 lore raw

PATCH RESEND v10 1/1 sched/deadline: support dl task migration during cpu hotplug

I observe that dl task can't be migrated to other cpus during cpu hotplug, in addition, task may/may not be running again if cpu is added back. The root cause which I found is that dl task will be throtted and removed from dl rq after comsuming all budget, which leads to stop task can't pick it up from dl rq and migrate to other cpus during hotplug.

The method to reproduce:
schedtool -E -t 50000:100000 -e ./test
Actually test is just a simple for loop. Then observe which cpu the test task is on.
echo 0 > /sys/devices/system/cpu/cpuN/online

This patch adds the dl task migration during cpu hotplug by finding a most suitable later deadline rq after dl timer fire if current rq is offline, if fail to find a suitable later deadline rq then fallback to any eligible online cpu in order that the deadline task will come back to us, and the push/pull mechanism should then move it around properly.

Suggested-and-acked-by Juri Lelli <[email protected]>
Signed-off-by Wanpeng Li <[email protected]>
---
 kernel/sched/deadline.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
kernel/sched/deadline.c +54 -0
index 5cb5c9c..db457b9 100644
@@ -492,6 +492,7 @@ static int start_dl_timer(struct sched_dl_entity *dl_se, bool boosted)
 	return hrtimer_active(&dl_se->dl_timer);
 }
 
+static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
 /*
  * This is the bandwidth enforcement timer callback. If here, we know
  * a task is not on its dl_rq, since the fact that the timer was running

@@ -537,6 +538,59 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	update_rq_clock(rq);
 
 	/*
+	 * So if we find that the rq the task was on is no longer
+	 * available, we need to select a new rq.
+	 */
+	if (unlikely(!rq->online)) {
+		struct rq *later_rq = NULL;
+		bool fallback = false;
+
+		later_rq = find_lock_later_rq(p, rq);
+
+		if (!later_rq) {
+			int cpu;
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);
+					goto unlock;
+				} else {
+					/*
+					 * If admission control is disabled we
+					 * try a little harder to let the task
+					 * run.
+					 */
+					cpu = cpumask_any(cpu_active_mask);
+				}
+			}
+			later_rq = cpu_rq(cpu);
+			double_lock_balance(rq, later_rq);
+		}
+
+		deactivate_task(rq, p, 0);
+		set_task_cpu(p, later_rq->cpu);
+		activate_task(later_rq, p, ENQUEUE_REPLENISH);
+
+		if (!fallback)
+			resched_curr(later_rq);
+
+		double_unlock_balance(rq, later_rq);
+
+		goto unlock;
+	}
+
+	/*
 	 * If the throttle happened during sched-out; like:
 	 *
 	 *   schedule()
Signature · 1 line
1.9.1
author
#1 lore raw

Ping Ingo,

Quoted 98 lines · Wanpeng Li

I observe that dl task can't be migrated to other cpus during cpu hotplug, in addition, task may/may not be running again if cpu is added back. The root cause which I found is that dl task will be throtted and removed from dl rq after comsuming all budget, which leads to stop task can't pick it up from dl rq and migrate to other cpus during hotplug.

The method to reproduce:
schedtool -E -t 50000:100000 -e ./test
Actually test is just a simple for loop. Then observe which cpu the test task is on.
echo 0 > /sys/devices/system/cpu/cpuN/online

This patch adds the dl task migration during cpu hotplug by finding a most suitable later deadline rq after dl timer fire if current rq is offline, if fail to find a suitable later deadline rq then fallback to any eligible online cpu in order that the deadline task will come back to us, and the push/pull mechanism should then move it around properly.

Suggested-and-acked-by Juri Lelli <[email protected]>
Signed-off-by Wanpeng Li <[email protected]>

---
kernel/sched/deadline.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++

1 file changed, 54 insertions(+)
kernel/sched/deadline.c +54 -0
index 5cb5c9c..db457b9 100644
@@ -492,6 +492,7 @@ static int start_dl_timer(struct sched_dl_entity *dl_se, bool boosted)
 	return hrtimer_active(&dl_se->dl_timer);
 }
 
+static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
 /*
 * This is the bandwidth enforcement timer callback. If here, we know
 * a task is not on its dl_rq, since the fact that the timer was running

@@ -537,6 +538,59 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	update_rq_clock(rq);
 
 	/*
+	 * So if we find that the rq the task was on is no longer
+	 * available, we need to select a new rq.
+	 */
+	if (unlikely(!rq->online)) {
+		struct rq *later_rq = NULL;
+		bool fallback = false;
+
+		later_rq = find_lock_later_rq(p, rq);
+
+		if (!later_rq) {
+			int cpu;
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);
+					goto unlock;
+				} else {
+					/*
+					 * If admission control is disabled we
+					 * try a little harder to let the task
+					 * run.
+					 */
+					cpu = cpumask_any(cpu_active_mask);
+				}
+			}
+			later_rq = cpu_rq(cpu);
+			double_lock_balance(rq, later_rq);
+		}
+
+		deactivate_task(rq, p, 0);
+		set_task_cpu(p, later_rq->cpu);
+		activate_task(later_rq, p, ENQUEUE_REPLENISH);
+
+		if (!fallback)
+			resched_curr(later_rq);
+
+		double_unlock_balance(rq, later_rq);
+
+		goto unlock;
+	}
+
+	/*
 	 * If the throttle happened during sched-out; like:
 	 *
 	 *   schedule()
Signature · 1 line
1.9.1
#2 lore raw
In reply to Wanpeng Li
Quoted 45 lines · Wanpeng Li

I observe that dl task can't be migrated to other cpus during cpu hotplug, in addition, task may/may not be running again if cpu is added back. The root cause which I found is that dl task will be throtted and removed from dl rq after comsuming all budget, which leads to stop task can't pick it up from dl rq and migrate to other cpus during hotplug.

The method to reproduce:
schedtool -E -t 50000:100000 -e ./test
Actually test is just a simple for loop. Then observe which cpu the test task is on.
echo 0 > /sys/devices/system/cpu/cpuN/online

This patch adds the dl task migration during cpu hotplug by finding a most suitable later deadline rq after dl timer fire if current rq is offline, if fail to find a suitable later deadline rq then fallback to any eligible online cpu in order that the deadline task will come back to us, and the push/pull mechanism should then move it around properly.

Suggested-and-acked-by Juri Lelli <[email protected]>
Signed-off-by Wanpeng Li <[email protected]>
---
 kernel/sched/deadline.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
kernel/sched/deadline.c +6 -0
index 5cb5c9c..db457b9 100644
@@ -492,6 +492,7 @@ static int start_dl_timer(struct sched_dl_entity *dl_se, bool boosted)
 	return hrtimer_active(&dl_se->dl_timer);
 }
 
+static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
 /*
  * This is the bandwidth enforcement timer callback. If here, we know
  * a task is not on its dl_rq, since the fact that the timer was running

@@ -537,6 +538,59 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	update_rq_clock(rq);
 
 	/*
+	 * So if we find that the rq the task was on is no longer
+	 * available, we need to select a new rq.
+	 */
+	if (unlikely(!rq->online)) {
+		struct rq *later_rq = NULL;

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 555

Quoted 10 lines
kernel/sched/deadline.c +10 -0
+		bool fallback = false;
+
+		later_rq = find_lock_later_rq(p, rq);
+
+		if (!later_rq) {
+			int cpu;
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 559

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts of this new code should be in a helper function, not inside several layers of branches.

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 566

Quoted 7 lines
kernel/sched/deadline.c +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Thanks,

Ingo

author
#3 lore raw

Hi Ingo,

Quoted 7 lines · Ingo Molnar

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 555

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

Will do.

Quoted 7 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 559

Quoted 5 lines
kernel/sched/deadline.c +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts

Otherwise there is a "WARNING: line over 80 characters".

Quoted 2 lines

of this new code should be in a helper function, not inside several layers of branches.

Do you mean the whole patch should be in a helper function?

Quoted 10 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 566

Quoted 8 lines
kernel/sched/deadline.c +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

What do you prefer? ;-)

Regards,
Wanpeng Li

#4 lore raw
Quoted 13 lines · Wanpeng Li

Hi Ingo,

Quoted 7 lines · Ingo Molnar

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 555

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

Will do.

Quoted 1 line
Quoted 8 lines
Quoted 6 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 559

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts

Otherwise there is a "WARNING: line over 80 characters".

Yes, but did your reaction to that tool's warning improve the code? I don't think so. If do what I suggested and reduce indentation a bit, you'll fix the warning _and_ improve the code. Win-win.

Quoted 4 lines
Quoted 2 lines

of this new code should be in a helper function, not inside several layers of branches.

Do you mean the whole patch should be in a helper function?

Probably.

Quoted 11 lines
Quoted 9 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 566

Quoted 7 lines
kernel/sched/deadline.c +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

What do you prefer? ;-)

That was a yes/no question: can this condition trigger on correctly working kernels?

Thanks,

Ingo

author
#5 lore raw

Hi Ingo,

Quoted 23 lines · Ingo Molnar
Quoted 20 lines · Wanpeng Li

Hi Ingo,

Quoted 7 lines · Ingo Molnar

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 555

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

Will do.

Quoted 7 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 559

Quoted 5 lines
kernel/sched/deadline.c +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts

Quoted 5 lines
Quoted 1 line

Otherwise there is a "WARNING: line over 80 characters".

Yes, but did your reaction to that tool's warning improve the code? I don't think so. If do what I suggested and reduce indentation a bit, you'll fix the warning _and_ improve the code. Win-win.

Cool, will do.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

of this new code should be in a helper function, not inside several layers of branches.

Do you mean the whole patch should be in a helper function?

Probably.

Will do.

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 566

Quoted 8 lines
kernel/sched/deadline.c +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 4 lines
Quoted 1 line

What do you prefer? ;-)

That was a yes/no question: can this condition trigger on correctly working kernels?

How about add unlikely() here?

Regards,
Wanpeng Li

Quoted 4 lines

Thanks,

Ingo

#6 lore raw
Quoted 54 lines · Wanpeng Li

Hi Ingo,

Quoted 23 lines · Ingo Molnar
Quoted 20 lines · Wanpeng Li

Hi Ingo,

Quoted 7 lines · Ingo Molnar

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 555

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

Will do.

Quoted 7 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 559

Quoted 5 lines
kernel/sched/deadline.c +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts

Quoted 5 lines
Quoted 1 line

Otherwise there is a "WARNING: line over 80 characters".

Yes, but did your reaction to that tool's warning improve the code? I don't think so. If do what I suggested and reduce indentation a bit, you'll fix the warning _and_ improve the code. Win-win.

Cool, will do.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

of this new code should be in a helper function, not inside several layers of branches.

Do you mean the whole patch should be in a helper function?

Probably.

Will do.

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 566

Quoted 8 lines
kernel/sched/deadline.c +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

What do you prefer? ;-)

That was a yes/no question: can this condition trigger on correctly working kernels?

How about add unlikely() here?

Please answer my question: can this condition trigger on correctly working kernels? I think so, but maybe I'm wrong?

Yes/no.

Thanks,

Ingo

author
#7 lore raw
Quoted 59 lines · Ingo Molnar
Quoted 56 lines · Wanpeng Li

Hi Ingo,

Quoted 23 lines · Ingo Molnar
Quoted 20 lines · Wanpeng Li

Hi Ingo,

Quoted 7 lines · Ingo Molnar

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 555

Quoted 4 lines
kernel/sched/deadline.c +4 -0
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

Will do.

Quoted 7 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 559

Quoted 5 lines
kernel/sched/deadline.c +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts

Quoted 5 lines
Quoted 1 line

Otherwise there is a "WARNING: line over 80 characters".

Yes, but did your reaction to that tool's warning improve the code? I don't think so. If do what I suggested and reduce indentation a bit, you'll fix the warning _and_ improve the code. Win-win.

Cool, will do.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

of this new code should be in a helper function, not inside several layers of branches.

Do you mean the whole patch should be in a helper function?

Probably.

Will do.

Quoted 13 lines
Quoted 12 lines
Quoted 10 lines

kernel/sched/deadline.c static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) · line 566

Quoted 8 lines
kernel/sched/deadline.c +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

What do you prefer? ;-)

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

That was a yes/no question: can this condition trigger on correctly working kernels?

How about add unlikely() here?

Please answer my question: can this condition trigger on correctly working kernels? I think so, but maybe I'm wrong?

I didn't see it happen, I add this by Juri's suggestion, maybe he can explain more.

Ping Juri, ;-)

Regards,
Wanpeng Li

Quoted 6 lines

Yes/no.

Thanks,

Ingo

#8 lore raw
Quoted 65 lines · Wanpeng Li
Quoted 64 lines · Ingo Molnar
Quoted 60 lines · Wanpeng Li

Hi Ingo,

Quoted 23 lines · Ingo Molnar
Quoted 20 lines · Wanpeng Li

Hi Ingo,

Quoted 7 lines · Ingo Molnar

Review

Quoted 4 lines
(quoted diff) +4 -0
+
+			/*
+			 * If cannot preempt any rq, fallback to pick any
+			 * online cpu.

s/If cannot/If we cannot
s/fallback/fall back

Will do.

Quoted 7 lines

Review

Quoted 5 lines
(quoted diff) +4 -0
+			 */
+			fallback = true;
+			cpu = cpumask_any_and(cpu_active_mask,
+						tsk_cpus_allowed(p));

shouldn't be on separate lines - but this is also a sign that the guts

Quoted 5 lines
Quoted 1 line

Otherwise there is a "WARNING: line over 80 characters".

Yes, but did your reaction to that tool's warning improve the code? I don't think so. If do what I suggested and reduce indentation a bit, you'll fix the warning _and_ improve the code. Win-win.

Cool, will do.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

of this new code should be in a helper function, not inside several layers of branches.

Do you mean the whole patch should be in a helper function?

Probably.

Will do.

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

Review

Quoted 8 lines
(quoted diff) +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 4 lines
Quoted 1 line

What do you prefer? ;-)

That was a yes/no question: can this condition trigger on correctly working kernels?

How about add unlikely() here?

Quoted 7 lines
Quoted 2 lines

Please answer my question: can this condition trigger on correctly working kernels? I think so, but maybe I'm wrong?

I didn't see it happen, I add this by Juri's suggestion, maybe he can explain more.

Ping Juri, ;-)

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

Thanks,

Ingo

maintainer
#9 lore raw
Quoted 10 lines · Ingo Molnar
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 2 lines

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

So I had a look on Friday, it _should_ not happen, but it does due to a second bug Juri is currently chasing down.

#10 lore raw

Hi,

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Ingo Molnar
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 6 lines
Quoted 2 lines

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

So I had a look on Friday, it _should_ not happen, but it does due to a second bug Juri is currently chasing down.

Right, it should not happen. It happens because hotplug operations are destructive w.r.t. cpusets. Peter, how about we move the check you put in sched_cpu_inactive() to cpuset_cpu_inactive()? This way, if we fail, we don't need to destroy/rebuild the domains.

Thanks,

- Juri

Quoted 1 line

From 65e8033e05f8b70116747062d00d5a5c266699fb Mon Sep 17 00:00:00 2001

From: Juri Lelli <[email protected]>
Date: Tue, 24 Mar 2015 07:47:03 +0000
Subject: [PATCH] sched/core: check for available -dl bandwidth in
cpuset_cpu_inactive

Signed-off-by Juri Lelli <[email protected]>
---
 kernel/sched/core.c | 56 ++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)
kernel/sched/core.c +28 -28
index 50927eb..3723ad0 100644
@@ -5318,36 +5318,13 @@ static int sched_cpu_active(struct notifier_block *nfb,
 static int sched_cpu_inactive(struct notifier_block *nfb,
 					unsigned long action, void *hcpu)
 {
-	unsigned long flags;
-	long cpu = (long)hcpu;
-	struct dl_bw *dl_b;
-
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
-		set_cpu_active(cpu, false);
-
-		/* explicitly allow suspend */
-		if (!(action & CPU_TASKS_FROZEN)) {
-			bool overflow;
-			int cpus;
-
-			rcu_read_lock_sched();
-			dl_b = dl_bw_of(cpu);
-
-			raw_spin_lock_irqsave(&dl_b->lock, flags);
-			cpus = dl_bw_cpus(cpu);
-			overflow = __dl_overflow(dl_b, cpus, 0, 0);
-			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
-
-			rcu_read_unlock_sched();
-
-			if (overflow)
-				return notifier_from_errno(-EBUSY);
-		}
+		set_cpu_active((long)hcpu, false);
 		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
 	}
-
-	return NOTIFY_DONE;
 }
 
 static int __init migration_init(void)

@@ -7001,7 +6978,6 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 		 */
 
 	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
 		cpuset_update_active_cpus(true);
 		break;
 	default:

@@ -7013,8 +6989,32 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 			       void *hcpu)
 {
-	switch (action) {
+	unsigned long flags;
+	long cpu = (long)hcpu;
+	struct dl_bw *dl_b;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
+		/* explicitly allow suspend */
+		if (!(action & CPU_TASKS_FROZEN)) {
+			bool overflow;
+			int cpus;
+
+			rcu_read_lock_sched();
+			dl_b = dl_bw_of(cpu);
+
+			raw_spin_lock_irqsave(&dl_b->lock, flags);
+			cpus = dl_bw_cpus(cpu);
+			overflow = __dl_overflow(dl_b, cpus, 0, 0);
+			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
+
+			rcu_read_unlock_sched();
+
+			if (overflow) {
+				trace_printk("hotplug failed for cpu %lu", cpu);
+				return notifier_from_errno(-EBUSY);
+			}
+		}
 		cpuset_update_active_cpus(false);
 		break;
 	case CPU_DOWN_PREPARE_FROZEN:
Signature · 1 line
2.3.0
author
#11 lore raw

Hi Juri,

Quoted 17 lines · Juri Lelli

Hi,

Quoted 14 lines · Peter Zijlstra
Quoted 12 lines · Ingo Molnar
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

Quoted 8 lines
Quoted 3 lines

So I had a look on Friday, it _should_ not happen, but it does due to a second bug Juri is currently chasing down.

Right, it should not happen. It happens because hotplug operations are destructive w.r.t. cpusets. Peter, how about we move the check you put in sched_cpu_inactive() to cpuset_cpu_inactive()? This way, if we fail, we don't need to destroy/rebuild the domains.

I remember you mentioned that there is a bug through IRC last week, if this patch solve it?

Regards,
Wanpeng Li

Quoted 104 lines

Thanks,

- Juri

Quoted 1 line

From 65e8033e05f8b70116747062d00d5a5c266699fb Mon Sep 17 00:00:00 2001

From: Juri Lelli <[email protected]>
Date: Tue, 24 Mar 2015 07:47:03 +0000
Subject: [PATCH] sched/core: check for available -dl bandwidth in cpuset_cpu_inactive

Signed-off-by Juri Lelli <[email protected]>

---
kernel/sched/core.c | 56 ++++++++++++++++++++++++++---------------------------

1 file changed, 28 insertions(+), 28 deletions(-)
kernel/sched/core.c +28 -28
index 50927eb..3723ad0 100644
@@ -5318,36 +5318,13 @@ static int sched_cpu_active(struct notifier_block *nfb,
 static int sched_cpu_inactive(struct notifier_block *nfb,
 					unsigned long action, void *hcpu)
 {
-	unsigned long flags;
-	long cpu = (long)hcpu;
-	struct dl_bw *dl_b;
-
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
-		set_cpu_active(cpu, false);
-
-		/* explicitly allow suspend */
-		if (!(action & CPU_TASKS_FROZEN)) {
-			bool overflow;
-			int cpus;
-
-			rcu_read_lock_sched();
-			dl_b = dl_bw_of(cpu);
-
-			raw_spin_lock_irqsave(&dl_b->lock, flags);
-			cpus = dl_bw_cpus(cpu);
-			overflow = __dl_overflow(dl_b, cpus, 0, 0);
-			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
-
-			rcu_read_unlock_sched();
-
-			if (overflow)
-				return notifier_from_errno(-EBUSY);
-		}
+		set_cpu_active((long)hcpu, false);
 		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
 	}
-
-	return NOTIFY_DONE;
 }
 
 static int __init migration_init(void)

@@ -7001,7 +6978,6 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 		 */
 
 	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
 		cpuset_update_active_cpus(true);
 		break;
 	default:

@@ -7013,8 +6989,32 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 			       void *hcpu)
 {
-	switch (action) {
+	unsigned long flags;
+	long cpu = (long)hcpu;
+	struct dl_bw *dl_b;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
+		/* explicitly allow suspend */
+		if (!(action & CPU_TASKS_FROZEN)) {
+			bool overflow;
+			int cpus;
+
+			rcu_read_lock_sched();
+			dl_b = dl_bw_of(cpu);
+
+			raw_spin_lock_irqsave(&dl_b->lock, flags);
+			cpus = dl_bw_cpus(cpu);
+			overflow = __dl_overflow(dl_b, cpus, 0, 0);
+			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
+
+			rcu_read_unlock_sched();
+
+			if (overflow) {
+				trace_printk("hotplug failed for cpu %lu", cpu);
+				return notifier_from_errno(-EBUSY);
+			}
+		}
 		cpuset_update_active_cpus(false);
 		break;
 	case CPU_DOWN_PREPARE_FROZEN:
Signature · 1 line
2.3.0
#12 lore raw
Quoted 23 lines · Wanpeng Li

Hi Juri,

Quoted 21 lines · Juri Lelli

Hi,

Quoted 17 lines · Peter Zijlstra
Quoted 10 lines · Ingo Molnar
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 2 lines

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

So I had a look on Friday, it _should_ not happen, but it does due to a second bug Juri is currently chasing down.

Quoted 8 lines
Quoted 4 lines

Right, it should not happen. It happens because hotplug operations are destructive w.r.t. cpusets. Peter, how about we move the check you put in sched_cpu_inactive() to cpuset_cpu_inactive()? This way, if we fail, we don't need to destroy/rebuild the domains.

I remember you mentioned that there is a bug through IRC last week, if this patch solve it?

It seems to fix it. With the previous check we correctly fail to turn off a cpu with -dl task running only the first time. After that the bandwidth information associated with it was gone and subsequent hotplug operations on the same cpu would turn it off.

Thanks,

- Juri

Quoted 108 lines

Regards,
Wanpeng Li

Quoted 104 lines

Thanks,

- Juri

Quoted 1 line

From 65e8033e05f8b70116747062d00d5a5c266699fb Mon Sep 17 00:00:00 2001

From: Juri Lelli <[email protected]>
Date: Tue, 24 Mar 2015 07:47:03 +0000
Subject: [PATCH] sched/core: check for available -dl bandwidth in cpuset_cpu_inactive

Signed-off-by Juri Lelli <[email protected]>

---
kernel/sched/core.c | 56 ++++++++++++++++++++++++++---------------------------

1 file changed, 28 insertions(+), 28 deletions(-)
kernel/sched/core.c +28 -28
index 50927eb..3723ad0 100644
@@ -5318,36 +5318,13 @@ static int sched_cpu_active(struct notifier_block *nfb,
 static int sched_cpu_inactive(struct notifier_block *nfb,
 					unsigned long action, void *hcpu)
 {
-	unsigned long flags;
-	long cpu = (long)hcpu;
-	struct dl_bw *dl_b;
-
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
-		set_cpu_active(cpu, false);
-
-		/* explicitly allow suspend */
-		if (!(action & CPU_TASKS_FROZEN)) {
-			bool overflow;
-			int cpus;
-
-			rcu_read_lock_sched();
-			dl_b = dl_bw_of(cpu);
-
-			raw_spin_lock_irqsave(&dl_b->lock, flags);
-			cpus = dl_bw_cpus(cpu);
-			overflow = __dl_overflow(dl_b, cpus, 0, 0);
-			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
-
-			rcu_read_unlock_sched();
-
-			if (overflow)
-				return notifier_from_errno(-EBUSY);
-		}
+		set_cpu_active((long)hcpu, false);
 		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
 	}
-
-	return NOTIFY_DONE;
 }
 
 static int __init migration_init(void)

@@ -7001,7 +6978,6 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 		 */
 
 	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
 		cpuset_update_active_cpus(true);
 		break;
 	default:

@@ -7013,8 +6989,32 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 			       void *hcpu)
 {
-	switch (action) {
+	unsigned long flags;
+	long cpu = (long)hcpu;
+	struct dl_bw *dl_b;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
+		/* explicitly allow suspend */
+		if (!(action & CPU_TASKS_FROZEN)) {
+			bool overflow;
+			int cpus;
+
+			rcu_read_lock_sched();
+			dl_b = dl_bw_of(cpu);
+
+			raw_spin_lock_irqsave(&dl_b->lock, flags);
+			cpus = dl_bw_cpus(cpu);
+			overflow = __dl_overflow(dl_b, cpus, 0, 0);
+			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
+
+			rcu_read_unlock_sched();
+
+			if (overflow) {
+				trace_printk("hotplug failed for cpu %lu", cpu);
+				return notifier_from_errno(-EBUSY);
+			}
+		}
 		cpuset_update_active_cpus(false);
 		break;
 	case CPU_DOWN_PREPARE_FROZEN:
Signature · 1 line
2.3.0
author
#13 lore raw
Quoted 29 lines · Juri Lelli
Quoted 28 lines · Wanpeng Li

Hi Juri,

Quoted 25 lines · Juri Lelli

Hi,

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Ingo Molnar
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+			if (cpu >= nr_cpu_ids) {
+				if (dl_bandwidth_enabled()) {
+					/*
+					 * Fail to find any suitable cpu.
+					 * The task will never come back!
+					 */
+					WARN_ON(1);

Can this condition happen to users with a non-buggy kernel?

Quoted 6 lines
Quoted 2 lines

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

So I had a look on Friday, it _should_ not happen, but it does due to a second bug Juri is currently chasing down.

Right, it should not happen. It happens because hotplug operations are destructive w.r.t. cpusets. Peter, how about we move the check you put in sched_cpu_inactive() to cpuset_cpu_inactive()? This way, if we fail, we don't need to destroy/rebuild the domains.

Quoted 8 lines
Quoted 3 lines

I remember you mentioned that there is a bug through IRC last week, if this patch solve it?

It seems to fix it. With the previous check we correctly fail to turn off a cpu with -dl task running only the first time. After that the bandwidth information associated with it was gone and subsequent hotplug operations on the same cpu would turn it off.

Cool, thanks for your patch and efforts. ;-)

Regards,
Wanpeng Li

Quoted 113 lines

Thanks,

- Juri

Quoted 108 lines

Regards,
Wanpeng Li

Quoted 104 lines

Thanks,

- Juri

Quoted 1 line

From 65e8033e05f8b70116747062d00d5a5c266699fb Mon Sep 17 00:00:00 2001

From: Juri Lelli <[email protected]>
Date: Tue, 24 Mar 2015 07:47:03 +0000
Subject: [PATCH] sched/core: check for available -dl bandwidth in cpuset_cpu_inactive

Signed-off-by Juri Lelli <[email protected]>

---
kernel/sched/core.c | 56 ++++++++++++++++++++++++++---------------------------

1 file changed, 28 insertions(+), 28 deletions(-)
kernel/sched/core.c +28 -28
index 50927eb..3723ad0 100644
@@ -5318,36 +5318,13 @@ static int sched_cpu_active(struct notifier_block *nfb,
 static int sched_cpu_inactive(struct notifier_block *nfb,
 					unsigned long action, void *hcpu)
 {
-	unsigned long flags;
-	long cpu = (long)hcpu;
-	struct dl_bw *dl_b;
-
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
-		set_cpu_active(cpu, false);
-
-		/* explicitly allow suspend */
-		if (!(action & CPU_TASKS_FROZEN)) {
-			bool overflow;
-			int cpus;
-
-			rcu_read_lock_sched();
-			dl_b = dl_bw_of(cpu);
-
-			raw_spin_lock_irqsave(&dl_b->lock, flags);
-			cpus = dl_bw_cpus(cpu);
-			overflow = __dl_overflow(dl_b, cpus, 0, 0);
-			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
-
-			rcu_read_unlock_sched();
-
-			if (overflow)
-				return notifier_from_errno(-EBUSY);
-		}
+		set_cpu_active((long)hcpu, false);
 		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
 	}
-
-	return NOTIFY_DONE;
 }
 
 static int __init migration_init(void)

@@ -7001,7 +6978,6 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 		 */
 
 	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
 		cpuset_update_active_cpus(true);
 		break;
 	default:

@@ -7013,8 +6989,32 @@ static int cpuset_cpu_active(struct notifier_block *nfb, unsigned long action,
 static int cpuset_cpu_inactive(struct notifier_block *nfb, unsigned long action,
 			       void *hcpu)
 {
-	switch (action) {
+	unsigned long flags;
+	long cpu = (long)hcpu;
+	struct dl_bw *dl_b;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_DOWN_PREPARE:
+		/* explicitly allow suspend */
+		if (!(action & CPU_TASKS_FROZEN)) {
+			bool overflow;
+			int cpus;
+
+			rcu_read_lock_sched();
+			dl_b = dl_bw_of(cpu);
+
+			raw_spin_lock_irqsave(&dl_b->lock, flags);
+			cpus = dl_bw_cpus(cpu);
+			overflow = __dl_overflow(dl_b, cpus, 0, 0);
+			raw_spin_unlock_irqrestore(&dl_b->lock, flags);
+
+			rcu_read_unlock_sched();
+
+			if (overflow) {
+				trace_printk("hotplug failed for cpu %lu", cpu);
+				return notifier_from_errno(-EBUSY);
+			}
+		}
 		cpuset_update_active_cpus(false);
 		break;
 	case CPU_DOWN_PREPARE_FROZEN:
Signature · 1 line
2.3.0
maintainer
#14 lore raw
In reply to Juri Lelli
Quoted 4 lines · Juri Lelli

Right, it should not happen. It happens because hotplug operations are destructive w.r.t. cpusets. Peter, how about we move the check you put in sched_cpu_inactive() to cpuset_cpu_inactive()? This way, if we fail, we don't need to destroy/rebuild the domains.

Seems ok to my Monday brain, could you shoot a coherent Changelog this way so that I can copy/paste it into the patch?

#15 lore raw

Hi Peter,

Quoted 9 lines · Peter Zijlstra
Quoted 4 lines · Juri Lelli

Right, it should not happen. It happens because hotplug operations are destructive w.r.t. cpusets. Peter, how about we move the check you put in sched_cpu_inactive() to cpuset_cpu_inactive()? This way, if we fail, we don't need to destroy/rebuild the domains.

Seems ok to my Monday brain, could you shoot a coherent Changelog this way so that I can copy/paste it into the patch?

Sent it out as an updated patch.

Thanks,

- Juri

author
#16 lore raw
In reply to Ingo Molnar
Quoted 2 lines · Ingo Molnar

I still haven't seen a satisfactory answer to this question. Please don't resend patches without clearing questions raised during review.

Got it, btw, I think the v12 is the ready version. ;-)

Regards,
Wanpeng Li

Quoted 4 lines

Thanks,

Ingo