Patchwork [1/1] powerpc: Ignore IPIs to offline CPUs

login
register
mail settings
Submitter Brian King
Date April 21, 2010, 1:54 a.m.
Message ID <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com>
Download mbox | patch
Permalink /patch/50607/
State Not Applicable
Headers show

Comments

Brian King - April 21, 2010, 1:54 a.m.
Since there is nothing to stop an IPI from occurring to an
offline CPU, rather than printing a warning to the logs,
just ignore the IPI. This was seen while stress testing
SMT enable/disable.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 arch/powerpc/platforms/pseries/xics.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Michael Neuling - April 21, 2010, 2:04 a.m.
In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
> 
> Since there is nothing to stop an IPI from occurring to an
> offline CPU, rather than printing a warning to the logs,
> just ignore the IPI. This was seen while stress testing
> SMT enable/disable.

This seems like a recipe for disaster.  Do we at least need a
WARN_ON_ONCE?

> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> ---
> 
>  arch/powerpc/platforms/pseries/xics.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff -puN arch/powerpc/platforms/pseries/xics.c~powerpc_xics_ipi_offline arch
/powerpc/platforms/pseries/xics.c
> --- linux-2.6/arch/powerpc/platforms/pseries/xics.c~powerpc_xics_ipi_offline
	2010-04-20 20:46:06.000000000 -0500
> +++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/xics.c	2010-04-20 20:4
7:53.000000000 -0500
> @@ -545,7 +545,8 @@ static irqreturn_t xics_ipi_dispatch(int
>  {
>  	unsigned long *tgt = &per_cpu(xics_ipi_message, cpu);
>  
> -	WARN_ON(cpu_is_offline(cpu));
> +	if (cpu_is_offline(cpu))
> +		return IRQ_HANDLED;
>  
>  	mb();	/* order mmio clearing qirr */
>  	while (*tgt) {
> _

FYI random white space change here.

> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

Mikey
Brian King - April 21, 2010, 3:15 a.m.
On 04/20/2010 09:04 PM, Michael Neuling wrote:
> In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
>>
>> Since there is nothing to stop an IPI from occurring to an
>> offline CPU, rather than printing a warning to the logs,
>> just ignore the IPI. This was seen while stress testing
>> SMT enable/disable.
> 
> This seems like a recipe for disaster.  Do we at least need a
> WARN_ON_ONCE?

Actually we are only seeing it once per offlining of a CPU,
and only once in a while.
 
My guess is that once the CPU is marked offline fewer IPIs
get sent to it since its no longer in the online mask.

Perhaps we should be disabling IPIs to offline CPUs instead?

-Brian


> 
>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>
>>  arch/powerpc/platforms/pseries/xics.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff -puN arch/powerpc/platforms/pseries/xics.c~powerpc_xics_ipi_offline arch
> /powerpc/platforms/pseries/xics.c
>> --- linux-2.6/arch/powerpc/platforms/pseries/xics.c~powerpc_xics_ipi_offline
> 	2010-04-20 20:46:06.000000000 -0500
>> +++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/xics.c	2010-04-20 20:4
> 7:53.000000000 -0500
>> @@ -545,7 +545,8 @@ static irqreturn_t xics_ipi_dispatch(int
>>  {
>>  	unsigned long *tgt = &per_cpu(xics_ipi_message, cpu);
>>  
>> -	WARN_ON(cpu_is_offline(cpu));
>> +	if (cpu_is_offline(cpu))
>> +		return IRQ_HANDLED;
>>  
>>  	mb();	/* order mmio clearing qirr */
>>  	while (*tgt) {
>> _
> 
> FYI random white space change here.
> 
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
> 
> Mikey
Michael Ellerman - April 21, 2010, 1:35 p.m.
On Tue, 2010-04-20 at 22:15 -0500, Brian King wrote:
> On 04/20/2010 09:04 PM, Michael Neuling wrote:
> > In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
> >>
> >> Since there is nothing to stop an IPI from occurring to an
> >> offline CPU, rather than printing a warning to the logs,
> >> just ignore the IPI. This was seen while stress testing
> >> SMT enable/disable.
> > 
> > This seems like a recipe for disaster.  Do we at least need a
> > WARN_ON_ONCE?
> 
> Actually we are only seeing it once per offlining of a CPU,
> and only once in a while.
>  
> My guess is that once the CPU is marked offline fewer IPIs
> get sent to it since its no longer in the online mask.

Hmm, right. Once it's offline it shouldn't get _any_ IPIs, AFAICS.

> Perhaps we should be disabling IPIs to offline CPUs instead?

You mean not sending them? We do:

void smp_xics_message_pass(int target, int msg)
{
        unsigned int i;

        if (target < NR_CPUS) {
                smp_xics_do_message(target, msg);
        } else {
                for_each_online_cpu(i) {
                        if (target == MSG_ALL_BUT_SELF
                            && i == smp_processor_id())
                                continue;
                        smp_xics_do_message(i, msg);
                }
        }
}      

So it does sound like the IPI was sent while the cpu was online (ie.
before pseries_cpu_disable(), but xics_migrate_irqs_away() has not
caused the IPI to be cancelled.

Problem is I don't think we can just ignore the IPI. The IPI might have
been sent for a smp_call_function() which is waiting for the result, in
which case if we ignore it the caller will block for ever.

I don't see how to fix it :/

cheers
Brian King - April 21, 2010, 1:50 p.m.
On 04/21/2010 08:35 AM, Michael Ellerman wrote:
> On Tue, 2010-04-20 at 22:15 -0500, Brian King wrote:
>> On 04/20/2010 09:04 PM, Michael Neuling wrote:
>>> In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
>>>>
>>>> Since there is nothing to stop an IPI from occurring to an
>>>> offline CPU, rather than printing a warning to the logs,
>>>> just ignore the IPI. This was seen while stress testing
>>>> SMT enable/disable.
>>>
>>> This seems like a recipe for disaster.  Do we at least need a
>>> WARN_ON_ONCE?
>>
>> Actually we are only seeing it once per offlining of a CPU,
>> and only once in a while.
>>  
>> My guess is that once the CPU is marked offline fewer IPIs
>> get sent to it since its no longer in the online mask.
> 
> Hmm, right. Once it's offline it shouldn't get _any_ IPIs, AFAICS.
> 
>> Perhaps we should be disabling IPIs to offline CPUs instead?
> 
> You mean not sending them? We do:
> 
> void smp_xics_message_pass(int target, int msg)
> {
>         unsigned int i;
> 
>         if (target < NR_CPUS) {
>                 smp_xics_do_message(target, msg);
>         } else {
>                 for_each_online_cpu(i) {
>                         if (target == MSG_ALL_BUT_SELF
>                             && i == smp_processor_id())
>                                 continue;
>                         smp_xics_do_message(i, msg);
>                 }
>         }
> }      
> 
> So it does sound like the IPI was sent while the cpu was online (ie.
> before pseries_cpu_disable(), but xics_migrate_irqs_away() has not
> caused the IPI to be cancelled.
> 
> Problem is I don't think we can just ignore the IPI. The IPI might have
> been sent for a smp_call_function() which is waiting for the result, in
> which case if we ignore it the caller will block for ever.
> 
> I don't see how to fix it :/

Any objections to just removing the warning?

Thanks,

Brian
Michael Neuling - April 21, 2010, 9:03 p.m.
In message <4BCF029B.1020805@linux.vnet.ibm.com> you wrote:
> On 04/21/2010 08:35 AM, Michael Ellerman wrote:
> > On Tue, 2010-04-20 at 22:15 -0500, Brian King wrote:
> >> On 04/20/2010 09:04 PM, Michael Neuling wrote:
> >>> In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
> >>>>
> >>>> Since there is nothing to stop an IPI from occurring to an
> >>>> offline CPU, rather than printing a warning to the logs,
> >>>> just ignore the IPI. This was seen while stress testing
> >>>> SMT enable/disable.
> >>>
> >>> This seems like a recipe for disaster.  Do we at least need a
> >>> WARN_ON_ONCE?
> >>
> >> Actually we are only seeing it once per offlining of a CPU,
> >> and only once in a while.
> >>  
> >> My guess is that once the CPU is marked offline fewer IPIs
> >> get sent to it since its no longer in the online mask.
> > 
> > Hmm, right. Once it's offline it shouldn't get _any_ IPIs, AFAICS.
> > 
> >> Perhaps we should be disabling IPIs to offline CPUs instead?
> > 
> > You mean not sending them? We do:
> > 
> > void smp_xics_message_pass(int target, int msg)
> > {
> >         unsigned int i;
> > 
> >         if (target < NR_CPUS) {
> >                 smp_xics_do_message(target, msg);
> >         } else {
> >                 for_each_online_cpu(i) {
> >                         if (target == MSG_ALL_BUT_SELF
> >                             && i == smp_processor_id())
> >                                 continue;
> >                         smp_xics_do_message(i, msg);
> >                 }
> >         }
> > }      
> > 
> > So it does sound like the IPI was sent while the cpu was online (ie.
> > before pseries_cpu_disable(), but xics_migrate_irqs_away() has not
> > caused the IPI to be cancelled.
> > 
> > Problem is I don't think we can just ignore the IPI. The IPI might have
> > been sent for a smp_call_function() which is waiting for the result, in
> > which case if we ignore it the caller will block for ever.
> > 
> > I don't see how to fix it :/
> 
> Any objections to just removing the warning?

Well someone could be waiting for the result, so it could be a real
problem.  

IMHO the warning should stay.

Mikey
Brian King - April 21, 2010, 10:15 p.m.
On 04/21/2010 04:03 PM, Michael Neuling wrote:
> In message <4BCF029B.1020805@linux.vnet.ibm.com> you wrote:
>> On 04/21/2010 08:35 AM, Michael Ellerman wrote:
>>> On Tue, 2010-04-20 at 22:15 -0500, Brian King wrote:
>>>> On 04/20/2010 09:04 PM, Michael Neuling wrote:
>>>>> In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
>>>>>>
>>>>>> Since there is nothing to stop an IPI from occurring to an
>>>>>> offline CPU, rather than printing a warning to the logs,
>>>>>> just ignore the IPI. This was seen while stress testing
>>>>>> SMT enable/disable.
>>>>>
>>>>> This seems like a recipe for disaster.  Do we at least need a
>>>>> WARN_ON_ONCE?
>>>>
>>>> Actually we are only seeing it once per offlining of a CPU,
>>>> and only once in a while.
>>>>  
>>>> My guess is that once the CPU is marked offline fewer IPIs
>>>> get sent to it since its no longer in the online mask.
>>>
>>> Hmm, right. Once it's offline it shouldn't get _any_ IPIs, AFAICS.
>>>
>>>> Perhaps we should be disabling IPIs to offline CPUs instead?
>>>
>>> You mean not sending them? We do:
>>>
>>> void smp_xics_message_pass(int target, int msg)
>>> {
>>>         unsigned int i;
>>>
>>>         if (target < NR_CPUS) {
>>>                 smp_xics_do_message(target, msg);
>>>         } else {
>>>                 for_each_online_cpu(i) {
>>>                         if (target == MSG_ALL_BUT_SELF
>>>                             && i == smp_processor_id())
>>>                                 continue;
>>>                         smp_xics_do_message(i, msg);
>>>                 }
>>>         }
>>> }      
>>>
>>> So it does sound like the IPI was sent while the cpu was online (ie.
>>> before pseries_cpu_disable(), but xics_migrate_irqs_away() has not
>>> caused the IPI to be cancelled.
>>>
>>> Problem is I don't think we can just ignore the IPI. The IPI might have
>>> been sent for a smp_call_function() which is waiting for the result, in
>>> which case if we ignore it the caller will block for ever.
>>>
>>> I don't see how to fix it :/
>>
>> Any objections to just removing the warning?
> 
> Well someone could be waiting for the result, so it could be a real
> problem.  
> 
> IMHO the warning should stay.

Looking in arch/powerpc/kernel/smp.c, there are four possible IPIs:

void smp_message_recv(int msg)
{
	switch(msg) {
	case PPC_MSG_CALL_FUNCTION:
		generic_smp_call_function_interrupt();
		break;
	case PPC_MSG_RESCHEDULE:
		/* we notice need_resched on exit */
		break;
	case PPC_MSG_CALL_FUNC_SINGLE:
		generic_smp_call_function_single_interrupt();
		break;
	case PPC_MSG_DEBUGGER_BREAK:
		if (crash_ipi_function_ptr) {
			crash_ipi_function_ptr(get_irq_regs());
			break;
		}
#ifdef CONFIG_DEBUGGER
		debugger_ipi(get_irq_regs());
		break;
#endif /* CONFIG_DEBUGGER */
		/* FALLTHROUGH */

Both generic_smp_call_function_interrupt and generic_smp_call_function_single_interrupt
have WARN_ON(!cpu_online(cpu)); in them. The debugger IPI, appears to ignore the IPI
if the cpu is offline, which leaves the reschedule IPI. This is likely the one I am
seeing in test, since I'm not seeing the other WARN_ON's. 


-Brian
Michael Neuling - April 21, 2010, 10:49 p.m.
In message <4BCF78E5.9020502@linux.vnet.ibm.com> you wrote:
> On 04/21/2010 04:03 PM, Michael Neuling wrote:
> > In message <4BCF029B.1020805@linux.vnet.ibm.com> you wrote:
> >> On 04/21/2010 08:35 AM, Michael Ellerman wrote:
> >>> On Tue, 2010-04-20 at 22:15 -0500, Brian King wrote:
> >>>> On 04/20/2010 09:04 PM, Michael Neuling wrote:
> >>>>> In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
> >>>>>>
> >>>>>> Since there is nothing to stop an IPI from occurring to an
> >>>>>> offline CPU, rather than printing a warning to the logs,
> >>>>>> just ignore the IPI. This was seen while stress testing
> >>>>>> SMT enable/disable.
> >>>>>
> >>>>> This seems like a recipe for disaster.  Do we at least need a
> >>>>> WARN_ON_ONCE?
> >>>>
> >>>> Actually we are only seeing it once per offlining of a CPU,
> >>>> and only once in a while.
> >>>>  
> >>>> My guess is that once the CPU is marked offline fewer IPIs
> >>>> get sent to it since its no longer in the online mask.
> >>>
> >>> Hmm, right. Once it's offline it shouldn't get _any_ IPIs, AFAICS.
> >>>
> >>>> Perhaps we should be disabling IPIs to offline CPUs instead?
> >>>
> >>> You mean not sending them? We do:
> >>>
> >>> void smp_xics_message_pass(int target, int msg)
> >>> {
> >>>         unsigned int i;
> >>>
> >>>         if (target < NR_CPUS) {
> >>>                 smp_xics_do_message(target, msg);
> >>>         } else {
> >>>                 for_each_online_cpu(i) {
> >>>                         if (target == MSG_ALL_BUT_SELF
> >>>                             && i == smp_processor_id())
> >>>                                 continue;
> >>>                         smp_xics_do_message(i, msg);
> >>>                 }
> >>>         }
> >>> }      
> >>>
> >>> So it does sound like the IPI was sent while the cpu was online (ie.
> >>> before pseries_cpu_disable(), but xics_migrate_irqs_away() has not
> >>> caused the IPI to be cancelled.
> >>>
> >>> Problem is I don't think we can just ignore the IPI. The IPI might have
> >>> been sent for a smp_call_function() which is waiting for the result, in
> >>> which case if we ignore it the caller will block for ever.
> >>>
> >>> I don't see how to fix it :/
> >>
> >> Any objections to just removing the warning?
> > 
> > Well someone could be waiting for the result, so it could be a real
> > problem.  
> > 
> > IMHO the warning should stay.
> 
> Looking in arch/powerpc/kernel/smp.c, there are four possible IPIs:
> 
> void smp_message_recv(int msg)
> {
> 	switch(msg) {
> 	case PPC_MSG_CALL_FUNCTION:
> 		generic_smp_call_function_interrupt();
> 		break;
> 	case PPC_MSG_RESCHEDULE:
> 		/* we notice need_resched on exit */
> 		break;
> 	case PPC_MSG_CALL_FUNC_SINGLE:
> 		generic_smp_call_function_single_interrupt();
> 		break;
> 	case PPC_MSG_DEBUGGER_BREAK:
> 		if (crash_ipi_function_ptr) {
> 			crash_ipi_function_ptr(get_irq_regs());
> 			break;
> 		}
> #ifdef CONFIG_DEBUGGER
> 		debugger_ipi(get_irq_regs());
> 		break;
> #endif /* CONFIG_DEBUGGER */
> 		/* FALLTHROUGH */
> 
> 
> Both generic_smp_call_function_interrupt and
> generic_smp_call_function_single_interrupt have
> WARN_ON(!cpu_online(cpu)); in them. The debugger IPI, appears to
> ignore the IPI if the cpu is offline, which leaves the reschedule
> IPI. This is likely the one I am seeing in test, since I'm not seeing
> the other WARN_ON's.

I'm not sure what you are suggesting?

If the other methods produce the warning when a CPU is offline, surely
we should keep the warning?  Maybe we need to add one to the debugger
case too if we want to be consistent.  

Mikey
Brian King - April 21, 2010, 11:33 p.m.
On 04/21/2010 05:49 PM, Michael Neuling wrote:
> In message <4BCF78E5.9020502@linux.vnet.ibm.com> you wrote:
>> On 04/21/2010 04:03 PM, Michael Neuling wrote:
>>> In message <4BCF029B.1020805@linux.vnet.ibm.com> you wrote:
>>>> On 04/21/2010 08:35 AM, Michael Ellerman wrote:
>>>>> On Tue, 2010-04-20 at 22:15 -0500, Brian King wrote:
>>>>>> On 04/20/2010 09:04 PM, Michael Neuling wrote:
>>>>>>> In message <201004210154.o3L1sXaR001791@d01av04.pok.ibm.com> you wrote:
>>>>>>>>
>>>>>>>> Since there is nothing to stop an IPI from occurring to an
>>>>>>>> offline CPU, rather than printing a warning to the logs,
>>>>>>>> just ignore the IPI. This was seen while stress testing
>>>>>>>> SMT enable/disable.
>>>>>>>
>>>>>>> This seems like a recipe for disaster.  Do we at least need a
>>>>>>> WARN_ON_ONCE?
>>>>>>
>>>>>> Actually we are only seeing it once per offlining of a CPU,
>>>>>> and only once in a while.
>>>>>>  
>>>>>> My guess is that once the CPU is marked offline fewer IPIs
>>>>>> get sent to it since its no longer in the online mask.
>>>>>
>>>>> Hmm, right. Once it's offline it shouldn't get _any_ IPIs, AFAICS.
>>>>>
>>>>>> Perhaps we should be disabling IPIs to offline CPUs instead?
>>>>>
>>>>> You mean not sending them? We do:
>>>>>
>>>>> void smp_xics_message_pass(int target, int msg)
>>>>> {
>>>>>         unsigned int i;
>>>>>
>>>>>         if (target < NR_CPUS) {
>>>>>                 smp_xics_do_message(target, msg);
>>>>>         } else {
>>>>>                 for_each_online_cpu(i) {
>>>>>                         if (target == MSG_ALL_BUT_SELF
>>>>>                             && i == smp_processor_id())
>>>>>                                 continue;
>>>>>                         smp_xics_do_message(i, msg);
>>>>>                 }
>>>>>         }
>>>>> }      
>>>>>
>>>>> So it does sound like the IPI was sent while the cpu was online (ie.
>>>>> before pseries_cpu_disable(), but xics_migrate_irqs_away() has not
>>>>> caused the IPI to be cancelled.
>>>>>
>>>>> Problem is I don't think we can just ignore the IPI. The IPI might have
>>>>> been sent for a smp_call_function() which is waiting for the result, in
>>>>> which case if we ignore it the caller will block for ever.
>>>>>
>>>>> I don't see how to fix it :/
>>>>
>>>> Any objections to just removing the warning?
>>>
>>> Well someone could be waiting for the result, so it could be a real
>>> problem.  
>>>
>>> IMHO the warning should stay.
>>
>> Looking in arch/powerpc/kernel/smp.c, there are four possible IPIs:
>>
>> void smp_message_recv(int msg)
>> {
>> 	switch(msg) {
>> 	case PPC_MSG_CALL_FUNCTION:
>> 		generic_smp_call_function_interrupt();
>> 		break;
>> 	case PPC_MSG_RESCHEDULE:
>> 		/* we notice need_resched on exit */
>> 		break;
>> 	case PPC_MSG_CALL_FUNC_SINGLE:
>> 		generic_smp_call_function_single_interrupt();
>> 		break;
>> 	case PPC_MSG_DEBUGGER_BREAK:
>> 		if (crash_ipi_function_ptr) {
>> 			crash_ipi_function_ptr(get_irq_regs());
>> 			break;
>> 		}
>> #ifdef CONFIG_DEBUGGER
>> 		debugger_ipi(get_irq_regs());
>> 		break;
>> #endif /* CONFIG_DEBUGGER */
>> 		/* FALLTHROUGH */
>>
>>
>> Both generic_smp_call_function_interrupt and
>> generic_smp_call_function_single_interrupt have
>> WARN_ON(!cpu_online(cpu)); in them. The debugger IPI, appears to
>> ignore the IPI if the cpu is offline, which leaves the reschedule
>> IPI. This is likely the one I am seeing in test, since I'm not seeing
>> the other WARN_ON's.
> 
> I'm not sure what you are suggesting?
> 
> If the other methods produce the warning when a CPU is offline, surely
> we should keep the warning?  Maybe we need to add one to the debugger
> case too if we want to be consistent.  

I guess my point was that perhaps the warning in xics_ipi_dispatch is redundant.
I'm not sure if there are issues with not having a warning in the reschedule path,
which is really the one I care about. Am I correct in assuming that for the reschedule
IPI we wouldn't have to worry about someone waiting on that? There shouldn't be
anything running on the cpu we are disabling, or should we be scheduling anything on it...

Thanks,

Brian

Patch

diff -puN arch/powerpc/platforms/pseries/xics.c~powerpc_xics_ipi_offline arch/powerpc/platforms/pseries/xics.c
--- linux-2.6/arch/powerpc/platforms/pseries/xics.c~powerpc_xics_ipi_offline	2010-04-20 20:46:06.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/xics.c	2010-04-20 20:47:53.000000000 -0500
@@ -545,7 +545,8 @@  static irqreturn_t xics_ipi_dispatch(int
 {
 	unsigned long *tgt = &per_cpu(xics_ipi_message, cpu);
 
-	WARN_ON(cpu_is_offline(cpu));
+	if (cpu_is_offline(cpu))
+		return IRQ_HANDLED;
 
 	mb();	/* order mmio clearing qirr */
 	while (*tgt) {