Re: [PATCH] fix BUG using smp_processor_id() in touch_nmi_watchdogand touch_softlockup_watchdog

[Andrew Morton]

On Wed, 18 Aug 2010 04:48:05 +0200
Frederic Weisbecker <fweisbec@xxxxxxxxx> wrote:

> On Tue, Aug 17, 2010 at 09:13:20AM -0400, Don Zickus wrote:
> > On Tue, Aug 17, 2010 at 01:39:48PM +0300, Sergey Senozhatsky wrote:
> > > Please kindly review.
> > 
> > I don't have a deep enough understanding of the subtleties between
> > per_cpu, __get_cpu_var, and __raw_get_cpu_var to really say which is
> > correct.  To me, all three versions of your patch look they do the same
> > thing.
> > 
> > Technically, it seems like preempt_disable/enable would be the correct
> > thing to do.  But as someone pointed out earlier, if the code is preempted
> > and switches cpu, then the touch_*_watchdog effectively becomes a no-op
> > (which I guess it can do even with the preempt_disable/enable surrounding
> > it).  So I have no idea.  I am going to wait for smarter people than me to
> > provide an opinion. :-)
> > 
> > Cheers,
> > Don
> 
> 
> (Adding Len Brown in Cc.

I'm not sure who looks after osl.c.  I added linux-acpi to cc.

> Len, this is about acpi_os_stall() that touches the watchdog while
> running in a preemptable section, this triggers warnings because of
> the use of local cpu accessors. We are debating about the appropriate
> way to solve this).
> 
> The more I think about it, the more I think that doesn't make sense
> to have touch_nmi_watchdog() callable from preemptable code.
> 
> It is buggy by nature.
> 
> If you run in a preemptable section, then interrupts can fire, and if
> they can, the nmi watchdog is fine and doesn't need to be touched.
> 
> Here the problem is more in the softlockup watchdog, because even if you
> run in a preemptable section, if you run a !CONFIG_PREEMPT kernel, then
> you can't be preempted and the watchdog won't be scheduled until the
> udelay loop finishes. But to solve that you would need cond_resched()
> calls, not touching the watchdog.
> 
> Because touching the softlockup watchdog doesn't make sense either
> if you can migrate: you can run the udelay on CPU 0, then migrate on
> CPU 1 and call touch_softlockup_watchdog() from there. Which makes
> definetely no sense. This is buggy.
> 
> And because we want to avoid such buggy uses of the touch_whatever_watchdog()
> APIs, these function must continue to check they are called from non-preemptable
> code. Randomly touching the watchdog could hide real lockups to the user.
> 
> The problem is on the caller. Considering such udelays loop:
> 
> * if it's in a irq disabled section, call touch_nmi_watchdog(), because this
>   could prevent the nmi watchdog irq from firing
> * if it's in a non-preemptable section, call touch_softlockup_watchdog(), because
>   this could prevent the softlockup watchdog task from beeing scheduled
> * if it's from a preemptable task context, this should call cond_resched() to
>   avoid huge latencies on !CONFIG_PREEMPT
> 
> 
> But acpi_os_stall() seem to be called from 4 different places, and these places
> may run in different context like the above described.
> 
> The ACPI code should probably use more specific busy-loop APIs, depending on the
> context it runs.

The touch_nmi_watchdog() was added to acpi_os_stall() by little old me
in 2003.  It was committed by Andy with the patch title "ACPI:
Correctly handle NMI watchdog during long stalls (Andrew Morton)".  My
title was "ACPI poweroff trigers the NMI watchdog".  My changelog was

	ACPI poweroff trigers the NMI watchdog.  Fix.

(My spelling has improved with age).

So.  If we remove that touch, will poweroff still trigger the NMI? 
Dunno.


The surprise new requirement that touch_nmi_watchdog() be called from
non-preemptible code does seem to make sense IMO.  It's hard to see why
anyone would be touching the watchdog unless he's spinning in irqs-off
code.  Except, of course, when we have a utility function which can be
called from wither irqs-on or irqs-off: acpi_os_stall().

That being said, it's not good to introduce new API requirements by
accident!  An audit of all callers should first be performed, at least.


The surprise new requirement that touch_softlockup_watchdog() be called
from non-preemptible code doesn't make sense IMO.  If I have a piece of
code in the kernel which I expect to sit in TASK_UNINTERRUPTIBLE state
for three minutes waiting for my egg to boil, I should be able to do
that and I should be able to touch the softlockup detector without
needing to go non-preemptible.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/