🚀 go-pugleaf

* Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Mon, 2005-09-19 at 15:12 +0800, Shaohua Li wrote:
> > On Mon, 2005-09-19 at 16:36 +1000, Nick Piggin wrote:
>
> > > Ah, actually I have a patch which makes all CPU idle threads
> > > run with preempt disabled and only enable preempt when scheduling.
> > > Would that help?
> > It should solve the issue to me. Should we take care of the latency?
> > acpi_processor_idle might execute for a long time.
> >
>
> Oh really? I think yes, the latency should be taken care of because we
> want to be able to provide good latency even for !preempt kernels. If
> a solution can be found for acpi_processor_idle, that would be ideal.

the ACPI idle code runs with irqs disabled anyway, so there's no issue
here. If something takes long there, we can do little about it. (but in
practice ACPI sleep latencies are pretty ok - the only latencies i found
in the past were due to need_resched bugs in the ACPI idle routine)

> IMO it always felt kind of hackish to run the idle threads with
> preempt on.

Yes, idle threads can have preemption disabled. There's not any big
difference in terms of latencies, the execution paths are all very
short.

	Ingo

On Mon, Sep 19, 2005 at 12:48:38PM +0800, Li, Shaohua wrote:
> I guess Nigel's point is cpu_idle is preempted before take_cpu_down. If
> the preempt occurs after the cpu_is_offline check, when the cpu (after

How can that happen? Idle task is running at max priority (MAX_RT_PRIO-1)
and with SCHED_FIFO policy at this point. If that is indeed happening,
then we need to modify sched_idle_next not to allow that.

> sched_idle_next) goes into idle again, nobody can wake it up. Nigel,
> isn't it?


--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, Sep 19, 2005 at 03:23:01PM +1000, Nigel Cunningham wrote:
> Maybe I'm just an ignoramus, but I was thinking (without being a
> scheduler expert at all) that if the idle thread was already running,
> trying to set it up to run next might possibly have zero effect. I've

sched_idle_next actually adds the idle task to its runqueue (normally
it is not present in a runqueue while running) and as well changes its
priority/policy. So it does have *some* effect!

> added a bit of debugging code to try and see in better detail what's
> happening.

Could you elaborate (with some stack traces maybe) on the deadlock you are
seeing during resume? Maybe that can throw some light.

--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, Sep 19, 2005 at 01:31:27PM +0800, Shaohua Li wrote:
> A CPU is idle and then is preempted and starting offline CPU. After
> calling stop_machine_run, the CPU goes into idle and it will resume last
> idle loop. If the CPU is broken at specific point, then the CPU will
> continue executing previous idle and have no chance to call play_dead.

Ok, that makes sense. Nigel, could you confirm which idle routine you are
using?

> Am I missing anything? Nigel's patch seems can fix the situation for
> mwait_idle and poll_idle but can't fix for default_idle in i386 to me.

I would say the right fix here is for poll_idle and mwait_idle (& similar
other idle routines) to monitor 'cpu_offline' flag in addition to need_resched
flag, rather than what Nigel has suggested.

--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, Sep 19, 2005 at 04:11:11PM +1000, Nigel Cunningham wrote:
> > Ok, that makes sense. Nigel, could you confirm which idle routine you are
> > using?
>
> >From dmesg:
>
> monitor/mwait feature present.
> using mwait in idle threads.

Ok, that may explain why __cpu_die is timing out for you! Can you try a
(untested, I am afraid) patch on these lines:

--- process.c.org	2005-09-19 11:44:57.000000000 +0530
+++ process.c	2005-09-19 11:48:28.000000000 +0530
@@ -245,16 +245,18 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
  */
 static void mwait_idle(void)
 {
+	int cpu = raw_smp_processor_id();
+
 	local_irq_enable();

 	if (!need_resched()) {
 		set_thread_flag(TIF_POLLING_NRFLAG);
 		do {
 			__monitor((void *)¤t_thread_info()->flags, 0, 0);
-			if (need_resched())
+			if (need_resched() || cpu_is_offline(cpu))
 				break;
 			__mwait(0, 0);
-		} while (!need_resched());
+		} while (!need_resched() || !cpu_is_offline(cpu));
 		clear_thread_flag(TIF_POLLING_NRFLAG);
 	}
 }


Other idle routines will need similar fix.

> Ok, but what about default_idle?

default_idle should be fine as it is. IOW it should not cause __cpu_die to
timeout.

--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, Sep 19, 2005 at 04:29:39PM +1000, Nick Piggin wrote:
> It seems to me that it would be much nicer to just go with Nigel's
> first fix. That is, rather than clutter up all idle routines with
> this.

I felt it to be more of a hack to get things working (do we really
want the idle thread to be rescheduled at that point?). Plus the fact
that it can cause the idle thread to call schedule() before the cpu_is_offline
check made me uncomfortable (maybe it is just fine).

--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, Sep 19, 2005 at 02:37:09PM +0800, Shaohua Li wrote:
> > -			if (need_resched())
> > +			if (need_resched() || cpu_is_offline(cpu))
> >  				break;
> if the breakpoint is here, you will still have trouble.

[snip]

> Why default_idle should be fine? it can be preempted before the
> 'local_irq_disable' check.
> Even with Nigel's patch, there is a very
> small window at safe_halt (after 'sti' but before 'hlt').

Good point. Sounds like the patch that Nick has for disabling premption
while it is idle may be a cure for these problems.

--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, Sep 19, 2005 at 04:31:36PM +1000, Nigel Cunningham wrote:
> > -		} while (!need_resched());
> > +		} while (!need_resched() || !cpu_is_offline(cpu));
>
> Shouldn't this be !need_resched() && !cpu_is_offline(cpu)?

Yes!


--


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

On Mon, 2005-09-19 at 10:40 +0530, Srivatsa Vaddagiri wrote:
> On Mon, Sep 19, 2005 at 12:48:38PM +0800, Li, Shaohua wrote:
> > I guess Nigel's point is cpu_idle is preempted before take_cpu_down. If
> > the preempt occurs after the cpu_is_offline check, when the cpu (after
>
> How can that happen? Idle task is running at max priority (MAX_RT_PRIO-1)
> and with SCHED_FIFO policy at this point. If that is indeed happening,
> then we need to modify sched_idle_next not to allow that.
A CPU is idle and then is preempted and starting offline CPU. After
calling stop_machine_run, the CPU goes into idle and it will resume last
idle loop. If the CPU is broken at specific point, then the CPU will
continue executing previous idle and have no chance to call play_dead.
Am I missing anything? Nigel's patch seems can fix the situation for
mwait_idle and poll_idle but can't fix for default_idle in i386 to me.

Thanks,
Shaohua

On Mon, 2005-09-19 at 11:53 +0530, Srivatsa Vaddagiri wrote:
> On Mon, Sep 19, 2005 at 04:11:11PM +1000, Nigel Cunningham wrote:
> > > Ok, that makes sense. Nigel, could you confirm which idle routine you are
> > > using?
> >
> > >From dmesg:
> >
> > monitor/mwait feature present.
> > using mwait in idle threads.
>
> Ok, that may explain why __cpu_die is timing out for you! Can you try a
> (untested, I am afraid) patch on these lines:
>
> --- process.c.org	2005-09-19 11:44:57.000000000 +0530
> +++ process.c	2005-09-19 11:48:28.000000000 +0530
> @@ -245,16 +245,18 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
>   */
>  static void mwait_idle(void)
>  {
> +	int cpu = raw_smp_processor_id();
> +
>  	local_irq_enable();
>
>  	if (!need_resched()) {
>  		set_thread_flag(TIF_POLLING_NRFLAG);
>  		do {
>  			__monitor((void *)¤t_thread_info()->flags, 0, 0);
> -			if (need_resched())
> +			if (need_resched() || cpu_is_offline(cpu))
>  				break;
if the breakpoint is here, you will still have trouble.

>  			__mwait(0, 0);
> -		} while (!need_resched());
> +		} while (!need_resched() || !cpu_is_offline(cpu));
>  		clear_thread_flag(TIF_POLLING_NRFLAG);
>  	}
>  }
>
>
> Other idle routines will need similar fix.
>
> > Ok, but what about default_idle?
>
> default_idle should be fine as it is. IOW it should not cause __cpu_die to
> timeout.
Why default_idle should be fine? it can be preempted before the
'local_irq_disable' check. Even with Nigel's patch, there is a very
small window at safe_halt (after 'sti' but before 'hlt').

Thanks,
Shaohua

On Mon, 2005-09-19 at 16:36 +1000, Nick Piggin wrote:
> On Mon, 2005-09-19 at 14:37 +0800, Shaohua Li wrote:
> > On Mon, 2005-09-19 at 11:53 +0530, Srivatsa Vaddagiri wrote:
>
> > > default_idle should be fine as it is. IOW it should not cause __cpu_die to
> > > timeout.
> > Why default_idle should be fine? it can be preempted before the
> > 'local_irq_disable' check. Even with Nigel's patch, there is a very
> > small window at safe_halt (after 'sti' but before 'hlt').
> >
>
> Ah, actually I have a patch which makes all CPU idle threads
> run with preempt disabled and only enable preempt when scheduling.
> Would that help?
It should solve the issue to me. Should we take care of the latency?
acpi_processor_idle might execute for a long time.

Thanks,
Shaohua

Hi.

On Mon, 2005-09-19 at 14:48, Li, Shaohua wrote:
> Hi,
> >
> >On Mon, Sep 19, 2005 at 01:28:38PM +1000, Nigel Cunningham wrote:
> >> There is a race condition in taking down a cpu
> (kernel/cpu.c::cpu_down).
> >> A cpu can already be idling when we clear its online flag, and we do
> not
> >> force the idle task to reschedule. This results in __cpu_die timing
> out.
> >
> >"when we clear its online flag" - This happens in take_cpu_down in the
> >context of stopmachine thread. take_cpu_down also ensures that idle
> >thread runs when it returns (sched_idle_next). So when idle thread
> runs,
> >it should notice that it is offline and invoke play_dead.  So I don't
> >understand why __cpu_die should time out.
> I guess Nigel's point is cpu_idle is preempted before take_cpu_down. If
> the preempt occurs after the cpu_is_offline check, when the cpu (after
> sched_idle_next) goes into idle again, nobody can wake it up. Nigel,
> isn't it?

Maybe I'm just an ignoramus, but I was thinking (without being a
scheduler expert at all) that if the idle thread was already running,
trying to set it up to run next might possibly have zero effect. I've
added a bit of debugging code to try and see in better detail what's
happening.

Regards,

Nigel

> Thanks,
> Shaohua
--

On Mon, 2005-09-19 at 15:28, Srivatsa Vaddagiri wrote:
> On Mon, Sep 19, 2005 at 03:23:01PM +1000, Nigel Cunningham wrote:
> > Maybe I'm just an ignoramus, but I was thinking (without being a
> > scheduler expert at all) that if the idle thread was already running,
> > trying to set it up to run next might possibly have zero effect. I've
>
> sched_idle_next actually adds the idle task to its runqueue (normally
> it is not present in a runqueue while running) and as well changes its
> priority/policy. So it does have *some* effect!

Ok.

> > added a bit of debugging code to try and see in better detail what's
> > happening.
>
> Could you elaborate (with some stack traces maybe) on the deadlock you are
> seeing during resume? Maybe that can throw some light.

I'll try. I'm having trouble reproducing it now (yes, having reversed my
patch!).

Regards,

Nigel
--

Hi.

On Mon, 2005-09-19 at 15:57, Srivatsa Vaddagiri wrote:
> On Mon, Sep 19, 2005 at 01:31:27PM +0800, Shaohua Li wrote:
> > A CPU is idle and then is preempted and starting offline CPU. After
> > calling stop_machine_run, the CPU goes into idle and it will resume last
> > idle loop. If the CPU is broken at specific point, then the CPU will
> > continue executing previous idle and have no chance to call play_dead.
>
> Ok, that makes sense. Nigel, could you confirm which idle routine you are
> using?

>From dmesg:

monitor/mwait feature present.
using mwait in idle threads.

> > Am I missing anything? Nigel's patch seems can fix the situation for
> > mwait_idle and poll_idle but can't fix for default_idle in i386 to me.
>
> I would say the right fix here is for poll_idle and mwait_idle (& similar
> other idle routines) to monitor 'cpu_offline' flag in addition to need_resched
> flag, rather than what Nigel has suggested.

Ok, but what about default_idle?

Regards,

Nigel

On Mon, 2005-09-19 at 11:53 +0530, Srivatsa Vaddagiri wrote:
> On Mon, Sep 19, 2005 at 04:11:11PM +1000, Nigel Cunningham wrote:
> > > Ok, that makes sense. Nigel, could you confirm which idle routine you are
> > > using?
> >
> > >From dmesg:
> >
> > monitor/mwait feature present.
> > using mwait in idle threads.
>
> Ok, that may explain why __cpu_die is timing out for you! Can you try a
> (untested, I am afraid) patch on these lines:
>
> --- process.c.org	2005-09-19 11:44:57.000000000 +0530
> +++ process.c	2005-09-19 11:48:28.000000000 +0530
> @@ -245,16 +245,18 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
>   */
>  static void mwait_idle(void)
>  {
> +	int cpu = raw_smp_processor_id();
> +
>  	local_irq_enable();
>
>  	if (!need_resched()) {
>  		set_thread_flag(TIF_POLLING_NRFLAG);
>  		do {
>  			__monitor((void *)¤t_thread_info()->flags, 0, 0);
> -			if (need_resched())
> +			if (need_resched() || cpu_is_offline(cpu))
>  				break;
>  			__mwait(0, 0);
> -		} while (!need_resched());
> +		} while (!need_resched() || !cpu_is_offline(cpu));
>  		clear_thread_flag(TIF_POLLING_NRFLAG);
>  	}
>  }
>
>
> Other idle routines will need similar fix.
>

It seems to me that it would be much nicer to just go with Nigel's
first fix. That is, rather than clutter up all idle routines with
this.

Nick

--
SUSE Labs, Novell Inc.



Send instant messages to your online friends http://au.messenger.yahoo.com

Hi.

On Mon, 2005-09-19 at 16:23, Srivatsa Vaddagiri wrote:
> On Mon, Sep 19, 2005 at 04:11:11PM +1000, Nigel Cunningham wrote:
> > > Ok, that makes sense. Nigel, could you confirm which idle routine you are
> > > using?
> >
> > >From dmesg:
> >
> > monitor/mwait feature present.
> > using mwait in idle threads.
>
> Ok, that may explain why __cpu_die is timing out for you! Can you try a
> (untested, I am afraid) patch on these lines:

Will do. Given my (understandable I guess) difficulty in reproducing it
reliably, shall I add a printk in there so I can see when it would have
otherwise failed to drop out?

> --- process.c.org	2005-09-19 11:44:57.000000000 +0530
> +++ process.c	2005-09-19 11:48:28.000000000 +0530
> @@ -245,16 +245,18 @@ EXPORT_SYMBOL_GPL(cpu_idle_wait);
>   */
>  static void mwait_idle(void)
>  {
> +	int cpu = raw_smp_processor_id();
> +
>  	local_irq_enable();
>
>  	if (!need_resched()) {
>  		set_thread_flag(TIF_POLLING_NRFLAG);
>  		do {
>  			__monitor((void *)¤t_thread_info()->flags, 0, 0);
> -			if (need_resched())
> +			if (need_resched() || cpu_is_offline(cpu))
>  				break;
>  			__mwait(0, 0);
> -		} while (!need_resched());
> +		} while (!need_resched() || !cpu_is_offline(cpu));

Shouldn't this be !need_resched() && !cpu_is_offline(cpu)?

Regards,

Nigel

>  		clear_thread_flag(TIF_POLLING_NRFLAG);
>  	}
>  }
>
>
> Other idle routines will need similar fix.
>
> > Ok, but what about default_idle?
>
> default_idle should be fine as it is. IOW it should not cause __cpu_die to
> timeout.
--

On Mon, 2005-09-19 at 14:37 +0800, Shaohua Li wrote:
> On Mon, 2005-09-19 at 11:53 +0530, Srivatsa Vaddagiri wrote:

> > default_idle should be fine as it is. IOW it should not cause __cpu_die to
> > timeout.
> Why default_idle should be fine? it can be preempted before the
> 'local_irq_disable' check. Even with Nigel's patch, there is a very
> small window at safe_halt (after 'sti' but before 'hlt').
>

Ah, actually I have a patch which makes all CPU idle threads
run with preempt disabled and only enable preempt when scheduling.
Would that help?

Nick

--
SUSE Labs, Novell Inc.



Send instant messages to your online friends http://au.messenger.yahoo.com

Hi.

On Mon, 2005-09-19 at 16:36, Nick Piggin wrote:
> On Mon, 2005-09-19 at 14:37 +0800, Shaohua Li wrote:
> > On Mon, 2005-09-19 at 11:53 +0530, Srivatsa Vaddagiri wrote:
>
> > > default_idle should be fine as it is. IOW it should not cause __cpu_die to
> > > timeout.
> > Why default_idle should be fine? it can be preempted before the
> > 'local_irq_disable' check. Even with Nigel's patch, there is a very
> > small window at safe_halt (after 'sti' but before 'hlt').
> >
>
> Ah, actually I have a patch which makes all CPU idle threads
> run with preempt disabled and only enable preempt when scheduling.
> Would that help?

Sounds to me like it should, but I'm not claiming any expertise in this
area. Going off your other posts, you're thinking of this plus my
original patch?

Regards,

Nigel

On Mon, 2005-09-19 at 15:12 +0800, Shaohua Li wrote:
> On Mon, 2005-09-19 at 16:36 +1000, Nick Piggin wrote:

> > Ah, actually I have a patch which makes all CPU idle threads
> > run with preempt disabled and only enable preempt when scheduling.
> > Would that help?
> It should solve the issue to me. Should we take care of the latency?
> acpi_processor_idle might execute for a long time.
>

Oh really? I think yes, the latency should be taken care of because
we want to be able to provide good latency even for !preempt kernels.
If a solution can be found for acpi_processor_idle, that would be
ideal.

IMO it always felt kind of hackish to run the idle threads with
preempt on.

Thanks,
Nick

--
SUSE Labs, Novell Inc.



Send instant messages to your online friends http://au.messenger.yahoo.com

On Mon, 2005-09-19 at 09:28 +0200, Ingo Molnar wrote:
> * Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> > Oh really? I think yes, the latency should be taken care of because we
> > want to be able to provide good latency even for !preempt kernels. If
> > a solution can be found for acpi_processor_idle, that would be ideal.
>
> the ACPI idle code runs with irqs disabled anyway, so there's no issue
> here. If something takes long there, we can do little about it. (but in
> practice ACPI sleep latencies are pretty ok - the only latencies i found
> in the past were due to need_resched bugs in the ACPI idle routine)
>

Ah, in that case I agree: we have nothing to worry about by merging
such a patch then.

> > IMO it always felt kind of hackish to run the idle threads with
> > preempt on.
>
> Yes, idle threads can have preemption disabled. There's not any big
> difference in terms of latencies, the execution paths are all very
> short.
>

Thanks for the confirmation Ingo. This is part of my "cleanup resched
and cpu_idle" patch FYI. It should already be in -mm, but has some
trivial EM64T bug in it that Andrew hits but I can't reproduce.

I'll dust it off and send it out, hopefully someone will be able to
reproduce the problem!

--
SUSE Labs, Novell Inc.



Send instant messages to your online friends http://au.messenger.yahoo.com