[PATCH] sched: fix cpupri hotplug support

[PATCH] sched: fix cpupri hotplug support

[Gregory Haskins]

Hi Ingo, Steven, Thomas,
  This patch is critical to fix a snafu in the root-domain/cpupri code.  It
  applies to 25.4-rt4, but as mentioned below it affects most recent -rt
  kernels as well as sched-devel.  If you accept this patch, and you have
  non-trivial merge issues with other flavors just let me know and I will port
  it to those kernels as well.

Regards
-Greg

--------------------------------
sched: fix cpupri hotplug support

The RT folks over at RedHat found an issue w.r.t. hotplug support which
was traced to problems with the cpupri infrastructure in the scheduler:

https://bugzilla.redhat.com/show_bug.cgi?id=449676

This bug affects 23-rt12+, 24-rtX, 25-rtX, and sched-devel.  This patch
applies to 25.4-rt4, though it should trivially apply to most cpupri enabled
kernels mentioned above.

It turned out that the issue was that offline cpus could get inadvertently
registered with cpupri so that they were erroneously selected during
migration decisions.  The end result would be an OOPS as the offline cpu
had tasks routed to it.

This patch adds a new per-sched-class pair of interfaces:
online/offline. They allow for the sched-class to register for
notification when a run-queue is taken online or offline.  Additionally,
the existing join/leave-domain code has been unified behind the new
online/offline handlers so that they do the right thing w.r.t. the online
status of a particular CPU.

I was able to easily reproduce the issue prior to this patch, and am no
longer able to reproduce it after this patch.  I can offline cpus
indefinately and everything seems to be in working order.

Thanks to Arnaldo (acme) and Thomas (tglx) for doing the legwork to point
me in the right direction. 

Signed-off-by: Gregory Haskins <ghaskins@novell.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Thomas Gleixner <tglx@linutonix.de>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
---

 include/linux/sched.h |    2 ++
 kernel/sched.c        |   19 +++++++++++++++++++
 kernel/sched_rt.c     |   16 ++++++++++++----
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index bb71371..9c5b8c9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -970,6 +970,8 @@ struct sched_class {
 
 	void (*join_domain)(struct rq *rq);
 	void (*leave_domain)(struct rq *rq);
+	void (*online)(struct rq *rq);
+	void (*offline)(struct rq *rq);
 
 	void (*switched_from) (struct rq *this_rq, struct task_struct *task,
 			       int running);
diff --git a/kernel/sched.c b/kernel/sched.c
index 0399411..fe96984 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6278,6 +6278,9 @@ static void unregister_sched_domain_sysctl(void)
 }
 #endif
 
+#define for_each_class(class) \
+   for (class = sched_class_highest; class; class = class->next)
+
 /*
  * migration_call - callback that gets triggered when a CPU is added.
  * Here we can start up the necessary migration thread for the new CPU.
@@ -6314,8 +6317,15 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
 		rq = cpu_rq(cpu);
 		spin_lock_irqsave(&rq->lock, flags);
 		if (rq->rd) {
+			const struct sched_class *class;
+
 			BUG_ON(!cpu_isset(cpu, rq->rd->span));
 			cpu_set(cpu, rq->rd->online);
+
+			for_each_class(class) {
+				if (class->online)
+					class->online(rq);
+			}
 		}
 		spin_unlock_irqrestore(&rq->lock, flags);
 		break;
@@ -6375,8 +6385,17 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
 		rq = cpu_rq(cpu);
 		spin_lock_irqsave(&rq->lock, flags);
 		if (rq->rd) {
+			const struct sched_class *class;
+
 			BUG_ON(!cpu_isset(cpu, rq->rd->span));
+
+			for_each_class(class) {
+				if (class->offline)
+					class->offline(rq);
+			}
+
 			cpu_clear(cpu, rq->rd->online);
+
 		}
 		spin_unlock_irqrestore(&rq->lock, flags);
 		break;
diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 425cf01..8baf2e8 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1101,8 +1101,11 @@ static void set_cpus_allowed_rt(struct task_struct *p, cpumask_t *new_mask)
 }
 
 /* Assumes rq->lock is held */
-static void join_domain_rt(struct rq *rq)
+static void rq_online_rt(struct rq *rq)
 {
+	if (!cpu_isset(rq->cpu, rq->rd->online))
+		return;
+
 	if (rq->rt.overloaded)
 		rt_set_overload(rq);
 
@@ -1110,8 +1113,11 @@ static void join_domain_rt(struct rq *rq)
 }
 
 /* Assumes rq->lock is held */
-static void leave_domain_rt(struct rq *rq)
+static void rq_offline_rt(struct rq *rq)
 {
+	if (!cpu_isset(rq->cpu, rq->rd->online))
+		return;
+
 	if (rq->rt.overloaded)
 		rt_clear_overload(rq);
 
@@ -1278,8 +1284,10 @@ const struct sched_class rt_sched_class = {
 	.load_balance		= load_balance_rt,
 	.move_one_task		= move_one_task_rt,
 	.set_cpus_allowed       = set_cpus_allowed_rt,
-	.join_domain            = join_domain_rt,
-	.leave_domain           = leave_domain_rt,
+	.join_domain            = rq_online_rt,
+	.leave_domain           = rq_offline_rt,
+	.online                 = rq_online_rt,
+	.offline                = rq_offline_rt,
 	.pre_schedule		= pre_schedule_rt,
 	.post_schedule		= post_schedule_rt,
 	.task_wake_up		= task_wake_up_rt,

Re: [PATCH] sched: fix cpupri hotplug support

[Peter Zijlstra]

On Tue, 2008-06-03 at 23:24 -0400, Gregory Haskins wrote:
> Hi Ingo, Steven, Thomas,
>   This patch is critical to fix a snafu in the root-domain/cpupri code.  It
>   applies to 25.4-rt4, but as mentioned below it affects most recent -rt
>   kernels as well as sched-devel.  If you accept this patch, and you have
>   non-trivial merge issues with other flavors just let me know and I will port
>   it to those kernels as well.
> 
> Regards
> -Greg
> 
> --------------------------------
> sched: fix cpupri hotplug support
> 
> The RT folks over at RedHat found an issue w.r.t. hotplug support which
> was traced to problems with the cpupri infrastructure in the scheduler:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=449676
> 
> This bug affects 23-rt12+, 24-rtX, 25-rtX, and sched-devel.  This patch
> applies to 25.4-rt4, though it should trivially apply to most cpupri enabled
> kernels mentioned above.
> 
> It turned out that the issue was that offline cpus could get inadvertently
> registered with cpupri so that they were erroneously selected during
> migration decisions.  The end result would be an OOPS as the offline cpu
> had tasks routed to it.

This is not what I saw, I traced it to select_task_rq_rt() selecting a
cpu that never was online. In my case cpu 2 on a dual core.

> This patch adds a new per-sched-class pair of interfaces:
> online/offline. They allow for the sched-class to register for
> notification when a run-queue is taken online or offline.  Additionally,
> the existing join/leave-domain code has been unified behind the new
> online/offline handlers so that they do the right thing w.r.t. the online
> status of a particular CPU.
> 
> I was able to easily reproduce the issue prior to this patch, and am no
> longer able to reproduce it after this patch.  I can offline cpus
> indefinately and everything seems to be in working order.
> 
> Thanks to Arnaldo (acme) and Thomas (tglx) for doing the legwork to point
> me in the right direction. 
> 
> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Thomas Gleixner <tglx@linutonix.de>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
> 
>  include/linux/sched.h |    2 ++
>  kernel/sched.c        |   19 +++++++++++++++++++
>  kernel/sched_rt.c     |   16 ++++++++++++----
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index bb71371..9c5b8c9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -970,6 +970,8 @@ struct sched_class {
>  
>  	void (*join_domain)(struct rq *rq);
>  	void (*leave_domain)(struct rq *rq);
> +	void (*online)(struct rq *rq);
> +	void (*offline)(struct rq *rq);

Rather unfortunate to add yet another two callbacks, can't this be done
with a creative use of leave/join?

>  	void (*switched_from) (struct rq *this_rq, struct task_struct *task,
>  			       int running);
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 0399411..fe96984 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -6278,6 +6278,9 @@ static void unregister_sched_domain_sysctl(void)
>  }
>  #endif
>  
> +#define for_each_class(class) \
> +   for (class = sched_class_highest; class; class = class->next)
> +
>  /*
>   * migration_call - callback that gets triggered when a CPU is added.
>   * Here we can start up the necessary migration thread for the new CPU.
> @@ -6314,8 +6317,15 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  		rq = cpu_rq(cpu);
>  		spin_lock_irqsave(&rq->lock, flags);
>  		if (rq->rd) {
> +			const struct sched_class *class;
> +
>  			BUG_ON(!cpu_isset(cpu, rq->rd->span));
>  			cpu_set(cpu, rq->rd->online);
> +
> +			for_each_class(class) {
> +				if (class->online)
> +					class->online(rq);
> +			}
>  		}
>  		spin_unlock_irqrestore(&rq->lock, flags);
>  		break;
> @@ -6375,8 +6385,17 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
>  		rq = cpu_rq(cpu);
>  		spin_lock_irqsave(&rq->lock, flags);
>  		if (rq->rd) {
> +			const struct sched_class *class;
> +
>  			BUG_ON(!cpu_isset(cpu, rq->rd->span));
> +
> +			for_each_class(class) {
> +				if (class->offline)
> +					class->offline(rq);
> +			}
> +
>  			cpu_clear(cpu, rq->rd->online);
> +
>  		}
>  		spin_unlock_irqrestore(&rq->lock, flags);
>  		break;

I'm thinking this should be done in update_sched_domains().

> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 425cf01..8baf2e8 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1101,8 +1101,11 @@ static void set_cpus_allowed_rt(struct task_struct *p, cpumask_t *new_mask)
>  }
>  
>  /* Assumes rq->lock is held */
> -static void join_domain_rt(struct rq *rq)
> +static void rq_online_rt(struct rq *rq)
>  {
> +	if (!cpu_isset(rq->cpu, rq->rd->online))
> +		return;
> +
>  	if (rq->rt.overloaded)
>  		rt_set_overload(rq);
>  
> @@ -1110,8 +1113,11 @@ static void join_domain_rt(struct rq *rq)
>  }
>  
>  /* Assumes rq->lock is held */
> -static void leave_domain_rt(struct rq *rq)
> +static void rq_offline_rt(struct rq *rq)
>  {
> +	if (!cpu_isset(rq->cpu, rq->rd->online))
> +		return;
> +
>  	if (rq->rt.overloaded)
>  		rt_clear_overload(rq);

So you now fully remove the leave/join calls because switching between
root domains doesn't need to migrate the overload status between those
domains? - seems unlikely.

> @@ -1278,8 +1284,10 @@ const struct sched_class rt_sched_class = {
>  	.load_balance		= load_balance_rt,
>  	.move_one_task		= move_one_task_rt,
>  	.set_cpus_allowed       = set_cpus_allowed_rt,
> -	.join_domain            = join_domain_rt,
> -	.leave_domain           = leave_domain_rt,
> +	.join_domain            = rq_online_rt,
> +	.leave_domain           = rq_offline_rt,
> +	.online                 = rq_online_rt,
> +	.offline                = rq_offline_rt,
>  	.pre_schedule		= pre_schedule_rt,
>  	.post_schedule		= post_schedule_rt,
>  	.task_wake_up		= task_wake_up_rt,

Re: [PATCH] sched: fix cpupri hotplug support

[Gregory Haskins]

>>> On Wed, Jun 4, 2008 at  7:59 AM, in message <1212580796.23439.10.camel@twins>,
Peter Zijlstra <peterz@infradead.org> wrote: 
> On Tue, 2008-06-03 at 23:24 -0400, Gregory Haskins wrote:
>> Hi Ingo, Steven, Thomas,
>>   This patch is critical to fix a snafu in the root-domain/cpupri code.  It
>>   applies to 25.4-rt4, but as mentioned below it affects most recent -rt
>>   kernels as well as sched-devel.  If you accept this patch, and you have
>>   non-trivial merge issues with other flavors just let me know and I will 
> port
>>   it to those kernels as well.
>> 
>> Regards
>> -Greg
>> 
>> --------------------------------
>> sched: fix cpupri hotplug support
>> 
>> The RT folks over at RedHat found an issue w.r.t. hotplug support which
>> was traced to problems with the cpupri infrastructure in the scheduler:
>> 
>> https://bugzilla.redhat.com/show_bug.cgi?id=449676
>> 
>> This bug affects 23-rt12+, 24-rtX, 25-rtX, and sched-devel.  This patch
>> applies to 25.4-rt4, though it should trivially apply to most cpupri enabled
>> kernels mentioned above.
>> 
>> It turned out that the issue was that offline cpus could get inadvertently
>> registered with cpupri so that they were erroneously selected during
>> migration decisions.  The end result would be an OOPS as the offline cpu
>> had tasks routed to it.
> 
> This is not what I saw, I traced it to select_task_rq_rt() selecting a
> cpu that never was online. In my case cpu 2 on a dual core.

First off, Peter, I didnt realize you were involved too.  So I apologize for omitting you in the credits. :(

Note that despite the reproduction case difference, I think we are still talking about the same basic problem here...and I think your observation would also be fixed by this patch.

The issue is that a cpu that is not online is being set to something other than INVALID in cpupri, so it comes up as a valid target during the various migration decisions.  This should now be fixed.


> 
>> This patch adds a new per-sched-class pair of interfaces:
>> online/offline. They allow for the sched-class to register for
>> notification when a run-queue is taken online or offline.  Additionally,
>> the existing join/leave-domain code has been unified behind the new
>> online/offline handlers so that they do the right thing w.r.t. the online
>> status of a particular CPU.
>> 
>> I was able to easily reproduce the issue prior to this patch, and am no
>> longer able to reproduce it after this patch.  I can offline cpus
>> indefinately and everything seems to be in working order.
>> 
>> Thanks to Arnaldo (acme) and Thomas (tglx) for doing the legwork to point
>> me in the right direction. 
>> 
>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>> CC: Ingo Molnar <mingo@elte.hu>
>> CC: Thomas Gleixner <tglx@linutonix.de>
>> CC: Steven Rostedt <rostedt@goodmis.org>
>> CC: Arnaldo Carvalho de Melo <acme@redhat.com>
>> ---
>> 
>>  include/linux/sched.h |    2 ++
>>  kernel/sched.c        |   19 +++++++++++++++++++
>>  kernel/sched_rt.c     |   16 ++++++++++++----
>>  3 files changed, 33 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index bb71371..9c5b8c9 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -970,6 +970,8 @@ struct sched_class {
>>  
>>  	void (*join_domain)(struct rq *rq);
>>  	void (*leave_domain)(struct rq *rq);
>> +	void (*online)(struct rq *rq);
>> +	void (*offline)(struct rq *rq);
> 
> Rather unfortunate to add yet another two callbacks, can't this be done
> with a creative use of leave/join?

Yeah, I was debating how to do this.  In fact, as you can see the implementation of join/online and leave/offline is identical.  I wasnt sure if putting a join/leave in the hotplug code made sense, so I added a new callback that maps to the same logic.  Perhaps what I should have done was to s/join/online and s/leave/offline and fixed the root-domain code to use the online/offline variant.  I like this better so I will put out a v2 patch in a few minutes.

> 
>>  	void (*switched_from) (struct rq *this_rq, struct task_struct *task,
>>  			       int running);
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index 0399411..fe96984 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -6278,6 +6278,9 @@ static void unregister_sched_domain_sysctl(void)
>>  }
>>  #endif
>>  
>> +#define for_each_class(class) \
>> +   for (class = sched_class_highest; class; class = class->next)
>> +
>>  /*
>>   * migration_call - callback that gets triggered when a CPU is added.
>>   * Here we can start up the necessary migration thread for the new CPU.
>> @@ -6314,8 +6317,15 @@ migration_call(struct notifier_block *nfb, unsigned 
> long action, void *hcpu)
>>  		rq = cpu_rq(cpu);
>>  		spin_lock_irqsave(&rq->lock, flags);
>>  		if (rq->rd) {
>> +			const struct sched_class *class;
>> +
>>  			BUG_ON(!cpu_isset(cpu, rq->rd->span));
>>  			cpu_set(cpu, rq->rd->online);
>> +
>> +			for_each_class(class) {
>> +				if (class->online)
>> +					class->online(rq);
>> +			}
>>  		}
>>  		spin_unlock_irqrestore(&rq->lock, flags);
>>  		break;
>> @@ -6375,8 +6385,17 @@ migration_call(struct notifier_block *nfb, unsigned 
> long action, void *hcpu)
>>  		rq = cpu_rq(cpu);
>>  		spin_lock_irqsave(&rq->lock, flags);
>>  		if (rq->rd) {
>> +			const struct sched_class *class;
>> +
>>  			BUG_ON(!cpu_isset(cpu, rq->rd->span));
>> +
>> +			for_each_class(class) {
>> +				if (class->offline)
>> +					class->offline(rq);
>> +			}
>> +
>>  			cpu_clear(cpu, rq->rd->online);
>> +
>>  		}
>>  		spin_unlock_irqrestore(&rq->lock, flags);
>>  		break;
> 
> I'm thinking this should be done in update_sched_domains().

Hmm...I think I agree with you.  I will add this to v2 as well.

> 
>> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
>> index 425cf01..8baf2e8 100644
>> --- a/kernel/sched_rt.c
>> +++ b/kernel/sched_rt.c
>> @@ -1101,8 +1101,11 @@ static void set_cpus_allowed_rt(struct task_struct *p, 
> cpumask_t *new_mask)
>>  }
>>  
>>  /* Assumes rq->lock is held */
>> -static void join_domain_rt(struct rq *rq)
>> +static void rq_online_rt(struct rq *rq)
>>  {
>> +	if (!cpu_isset(rq->cpu, rq->rd->online))
>> +		return;
>> +
>>  	if (rq->rt.overloaded)
>>  		rt_set_overload(rq);
>>  
>> @@ -1110,8 +1113,11 @@ static void join_domain_rt(struct rq *rq)
>>  }
>>  
>>  /* Assumes rq->lock is held */
>> -static void leave_domain_rt(struct rq *rq)
>> +static void rq_offline_rt(struct rq *rq)
>>  {
>> +	if (!cpu_isset(rq->cpu, rq->rd->online))
>> +		return;
>> +
>>  	if (rq->rt.overloaded)
>>  		rt_clear_overload(rq);
> 
> So you now fully remove the leave/join calls because switching between
> root domains doesn't need to migrate the overload status between those
> domains? - seems unlikely.

I think you are misinterpreting that part.  leave/join is fully functional and intact.  I just merged the two implementations to call one function (see the sched_class assignment below)

> 
>> @@ -1278,8 +1284,10 @@ const struct sched_class rt_sched_class = {
>>  	.load_balance		= load_balance_rt,
>>  	.move_one_task		= move_one_task_rt,
>>  	.set_cpus_allowed       = set_cpus_allowed_rt,
>> -	.join_domain            = join_domain_rt,
>> -	.leave_domain           = leave_domain_rt,
>> +	.join_domain            = rq_online_rt,
>> +	.leave_domain           = rq_offline_rt,
>> +	.online                 = rq_online_rt,
>> +	.offline                = rq_offline_rt,
>>  	.pre_schedule		= pre_schedule_rt,
>>  	.post_schedule		= post_schedule_rt,
>>  	.task_wake_up		= task_wake_up_rt,

Re: [PATCH] sched: fix cpupri hotplug support

[Peter Zijlstra]

On Wed, 2008-06-04 at 08:28 -0600, Gregory Haskins wrote:

> >> diff --git a/include/linux/sched.h b/include/linux/sched.h
> >> index bb71371..9c5b8c9 100644
> >> --- a/include/linux/sched.h
> >> +++ b/include/linux/sched.h
> >> @@ -970,6 +970,8 @@ struct sched_class {
> >>  
> >>  	void (*join_domain)(struct rq *rq);
> >>  	void (*leave_domain)(struct rq *rq);
> >> +	void (*online)(struct rq *rq);
> >> +	void (*offline)(struct rq *rq);
> > 
> > Rather unfortunate to add yet another two callbacks, can't this be done
> > with a creative use of leave/join?
> 
> Yeah, I was debating how to do this.  In fact, as you can see the
> implementation of join/online and leave/offline is identical.  I wasnt
> sure if putting a join/leave in the hotplug code made sense, so I
> added a new callback that maps to the same logic.  Perhaps what I
> should have done was to s/join/online and s/leave/offline and fixed
> the root-domain code to use the online/offline variant.  I like this
> better so I will put out a v2 patch in a few minutes.
> 

> I think you are misinterpreting that part.  leave/join is fully
> functional and intact.  I just merged the two implementations to call
> one function (see the sched_class assignment below)
> 
> > 
> >> @@ -1278,8 +1284,10 @@ const struct sched_class rt_sched_class = {
> >>  	.load_balance		= load_balance_rt,
> >>  	.move_one_task		= move_one_task_rt,
> >>  	.set_cpus_allowed       = set_cpus_allowed_rt,
> >> -	.join_domain            = join_domain_rt,
> >> -	.leave_domain           = leave_domain_rt,
> >> +	.join_domain            = rq_online_rt,
> >> +	.leave_domain           = rq_offline_rt,
> >> +	.online                 = rq_online_rt,
> >> +	.offline                = rq_offline_rt,
> >>  	.pre_schedule		= pre_schedule_rt,
> >>  	.post_schedule		= post_schedule_rt,
> >>  	.task_wake_up		= task_wake_up_rt,

Ah, right - I missed that.

Right - I was just looking to get the cpu-down path to do one ->leave()
extra (aside from leave/join the def_root_domain) so it's basically
unattached - which makes sense for a downed cpu.

Conversely the cpu-up should start with an extra ->join() attaching the
downed cpu again.

I'm thinking doing it that way saves us the extra callbacks. We can do
those if and when the leave/offline behaviour actually divert.