Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Don Zickus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1951 bytes --]

Top posting because droid won't let me bottom post

This patch was the result of a regression with acpi and preempt.  Akpm asked that I not change the semantics of the old touch_nmi_watchdog.  So I tried to revert to the old behaviour.

Sorry for not properly explaining that.

Cheers,
Don

Ingo Molnar <mingo@elte.hu> wrote:

>
>* Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> On 9/1/10, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > On 9/1/10, Ingo Molnar <mingo@elte.hu> wrote:
>> >>
>> >> * Don Zickus <dzickus@redhat.com> wrote:
>> >>
>> >>>  void touch_nmi_watchdog(void)
>> >>>  {
>> >>> -	__get_cpu_var(watchdog_nmi_touch) = true;
>> >>> +	if (watchdog_enabled) {
>> >>> +		unsigned cpu;
>> >>> +
>> >>> +		for_each_present_cpu(cpu) {
>> >>> +			if (per_cpu(watchdog_nmi_touch, cpu) != true)
>> >>> +				per_cpu(watchdog_nmi_touch, cpu) = true;
>> >>> +		}
>> >>
>> >> Hm, this is going to be a scalability nightmare with lots of CPUs. Not
>> >> only do we have a nr_cpus loop, but we touch per-cpu areas of _other_
>> >> CPUs - a big scalability nono.
>> >>
>> >> Why do we need to do this? We never needed to touch other CPU's NMI
>> >> lockup accounting data areas - why has this changed? The changelog does
>> >> not explain this.
>> >>
>> >> Thanks,
>> >>
>> >> 	Ingo
>> >>
>> > I believe this came from old nmi watchdog code where it might be
>> > useful when nmi watchdog activated via io-apic. I'm trying to figure
>> > out if we really need it still.
>>
>> Well, we can't drop it or make per-cpu specific, for example we need 
>> it in case of panic with watchdog enabled and panic timeout set, or 
>> boot delay set and etc. Seems same applies to printk_delay. Hmm...
>
>Ok - can you cite the old watchdog code, did it really do a nr_cpus 
>loop?
>
>Thanks,
>
>	Ingo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Cyrill Gorcunov]

On Wed, Sep 01, 2010 at 11:51:12AM -0400, Don Zickus wrote:
> Top posting because droid won't let me bottom post
> 
> This patch was the result of a regression with acpi and preempt.
> Akpm asked that I not change the semantics of the old touch_nmi_watchdog.
> So I tried to revert to the old behaviour.
> 
> Sorry for not properly explaining that.
> 

yup, stareing at old behaviour I think there is a place where we could
get rid of traversing all cpus: native_cpu_up() -- I don't get the reason why
watchdog counter should be reset on every other cpu as well. Perhaps
I miss something. On the other hands I think changing behaviour of
touch_nmi_watchdog just for one entry might not be worth thing to do :)

> Cheers,
> Don
> 
> Ingo Molnar <mingo@elte.hu> wrote:
> 
...
> >Ok - can you cite the old watchdog code, did it really do a nr_cpus 
> >loop?
> >
> >Thanks,
> >
> >	Ingo

	-- Cyrill

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

> Top posting because droid won't let me bottom post
> 
> This patch was the result of a regression with acpi and preempt.  Akpm 
> asked that I not change the semantics of the old touch_nmi_watchdog.  
> So I tried to revert to the old behaviour.

that's ok - i extended the changelog a bit before applying the patch.

Thanks,

	Ingo

[PATCH 0/3] lockup detector fixes

[Don Zickus]

A bunch of fixes for the lockup detector.

Tested on intel/amd on top of ingo's tip branch.

Akinobu Mita (2):
  lockup_detector: convert cpu notifier to return encapsulate errno
    value
  lockup_detector: remove unnecessary panic_notifier

Don Zickus (1):
  [lockup detector] sync touch_*_watchdog back to old semantics

 kernel/watchdog.c |   53 +++++++++++++++++++++++------------------------------
 1 files changed, 23 insertions(+), 30 deletions(-)

-- 
1.7.2.2

[PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Don Zickus]

During my rewrite, the semantics of touch_nmi_watchdog and
touch_softlockup_watchdog changed enough to break some drivers
(mostly over preemptable regions).

This change brings those touch_*_watchdog functions back in line
to how they used to work.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 kernel/watchdog.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 0d53c8e..7f9c3c5 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -122,7 +122,7 @@ static void __touch_watchdog(void)
 
 void touch_softlockup_watchdog(void)
 {
-	__get_cpu_var(watchdog_touch_ts) = 0;
+	__raw_get_cpu_var(watchdog_touch_ts) = 0;
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 
@@ -142,7 +142,14 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	if (watchdog_enabled) {
+		unsigned cpu;
+
+		for_each_present_cpu(cpu) {
+			if (per_cpu(watchdog_nmi_touch, cpu) != true)
+				per_cpu(watchdog_nmi_touch, cpu) = true;
+		}
+	}
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);
@@ -433,6 +440,9 @@ static int watchdog_enable(int cpu)
 		wake_up_process(p);
 	}
 
+	/* if any cpu succeeds, watchdog is considered enabled for the system */
+	watchdog_enabled = 1;
+
 	return 0;
 }
 
@@ -455,9 +465,6 @@ static void watchdog_disable(int cpu)
 		per_cpu(softlockup_watchdog, cpu) = NULL;
 		kthread_stop(p);
 	}
-
-	/* if any cpu succeeds, watchdog is considered enabled for the system */
-	watchdog_enabled = 1;
 }
 
 static void watchdog_enable_all_cpus(void)
-- 
1.7.2.2

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Ingo Molnar]

* Don Zickus <dzickus@redhat.com> wrote:

>  void touch_nmi_watchdog(void)
>  {
> -	__get_cpu_var(watchdog_nmi_touch) = true;
> +	if (watchdog_enabled) {
> +		unsigned cpu;
> +
> +		for_each_present_cpu(cpu) {
> +			if (per_cpu(watchdog_nmi_touch, cpu) != true)
> +				per_cpu(watchdog_nmi_touch, cpu) = true;
> +		}

Hm, this is going to be a scalability nightmare with lots of CPUs. Not 
only do we have a nr_cpus loop, but we touch per-cpu areas of _other_ 
CPUs - a big scalability nono.

Why do we need to do this? We never needed to touch other CPU's NMI 
lockup accounting data areas - why has this changed? The changelog does 
not explain this.

Thanks,

	Ingo

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Cyrill Gorcunov]

On 9/1/10, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Don Zickus <dzickus@redhat.com> wrote:
>
>>  void touch_nmi_watchdog(void)
>>  {
>> -	__get_cpu_var(watchdog_nmi_touch) = true;
>> +	if (watchdog_enabled) {
>> +		unsigned cpu;
>> +
>> +		for_each_present_cpu(cpu) {
>> +			if (per_cpu(watchdog_nmi_touch, cpu) != true)
>> +				per_cpu(watchdog_nmi_touch, cpu) = true;
>> +		}
>
> Hm, this is going to be a scalability nightmare with lots of CPUs. Not
> only do we have a nr_cpus loop, but we touch per-cpu areas of _other_
> CPUs - a big scalability nono.
>
> Why do we need to do this? We never needed to touch other CPU's NMI
> lockup accounting data areas - why has this changed? The changelog does
> not explain this.
>
> Thanks,
>
> 	Ingo
>
I believe this came from old nmi watchdog code where it might be
useful when nmi watchdog activated via io-apic. I'm trying to figure
out if we really need it still.

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Cyrill Gorcunov]

On 9/1/10, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On 9/1/10, Ingo Molnar <mingo@elte.hu> wrote:
>>
>> * Don Zickus <dzickus@redhat.com> wrote:
>>
>>>  void touch_nmi_watchdog(void)
>>>  {
>>> -	__get_cpu_var(watchdog_nmi_touch) = true;
>>> +	if (watchdog_enabled) {
>>> +		unsigned cpu;
>>> +
>>> +		for_each_present_cpu(cpu) {
>>> +			if (per_cpu(watchdog_nmi_touch, cpu) != true)
>>> +				per_cpu(watchdog_nmi_touch, cpu) = true;
>>> +		}
>>
>> Hm, this is going to be a scalability nightmare with lots of CPUs. Not
>> only do we have a nr_cpus loop, but we touch per-cpu areas of _other_
>> CPUs - a big scalability nono.
>>
>> Why do we need to do this? We never needed to touch other CPU's NMI
>> lockup accounting data areas - why has this changed? The changelog does
>> not explain this.
>>
>> Thanks,
>>
>> 	Ingo
>>
> I believe this came from old nmi watchdog code where it might be
> useful when nmi watchdog activated via io-apic. I'm trying to figure
> out if we really need it still.
>
Well, we can't drop it or make per-cpu specific, for example we need
it in case of panic with watchdog enabled and panic timeout set, or
boot delay set and etc. Seems same applies to printk_delay. Hmm...

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On 9/1/10, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > On 9/1/10, Ingo Molnar <mingo@elte.hu> wrote:
> >>
> >> * Don Zickus <dzickus@redhat.com> wrote:
> >>
> >>>  void touch_nmi_watchdog(void)
> >>>  {
> >>> -	__get_cpu_var(watchdog_nmi_touch) = true;
> >>> +	if (watchdog_enabled) {
> >>> +		unsigned cpu;
> >>> +
> >>> +		for_each_present_cpu(cpu) {
> >>> +			if (per_cpu(watchdog_nmi_touch, cpu) != true)
> >>> +				per_cpu(watchdog_nmi_touch, cpu) = true;
> >>> +		}
> >>
> >> Hm, this is going to be a scalability nightmare with lots of CPUs. Not
> >> only do we have a nr_cpus loop, but we touch per-cpu areas of _other_
> >> CPUs - a big scalability nono.
> >>
> >> Why do we need to do this? We never needed to touch other CPU's NMI
> >> lockup accounting data areas - why has this changed? The changelog does
> >> not explain this.
> >>
> >> Thanks,
> >>
> >> 	Ingo
> >>
> > I believe this came from old nmi watchdog code where it might be
> > useful when nmi watchdog activated via io-apic. I'm trying to figure
> > out if we really need it still.
>
> Well, we can't drop it or make per-cpu specific, for example we need 
> it in case of panic with watchdog enabled and panic timeout set, or 
> boot delay set and etc. Seems same applies to printk_delay. Hmm...

Ok - can you cite the old watchdog code, did it really do a nr_cpus 
loop?

Thanks,

	Ingo

Re: [PATCH 1/3] [lockup detector] sync touch_*_watchdog back to old semantics

[Cyrill Gorcunov]

On Wednesday, September 1, 2010, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> On 9/1/10, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > On 9/1/10, Ingo Molnar <mingo@elte.hu> wrote:
>> >>
>> >> * Don Zickus <dzickus@redhat.com> wrote:
>> >>
>> >>>  void touch_nmi_watchdog(void)
>> >>>  {
>> >>> - __get_cpu_var(watchdog_nmi_touch) = true;
>> >>> + if (watchdog_enabled) {
>> >>> +         unsigned cpu;
>> >>> +
>> >>> +         for_each_present_cpu(cpu) {
>> >>> +                 if (per_cpu(watchdog_nmi_touch, cpu) != true)
>> >>> +                         per_cpu(watchdog_nmi_touch, cpu) = true;
>> >>> +         }
>> >>
>> >> Hm, this is going to be a scalability nightmare with lots of CPUs. Not
>> >> only do we have a nr_cpus loop, but we touch per-cpu areas of _other_
>> >> CPUs - a big scalability nono.
>> >>
>> >> Why do we need to do this? We never needed to touch other CPU's NMI
>> >> lockup accounting data areas - why has this changed? The changelog does
>> >> not explain this.
>> >>
>> >> Thanks,
>> >>
>> >>    Ingo
>> >>
>> > I believe this came from old nmi watchdog code where it might be
>> > useful when nmi watchdog activated via io-apic. I'm trying to figure
>> > out if we really need it still.
>>
>> Well, we can't drop it or make per-cpu specific, for example we need
>> it in case of panic with watchdog enabled and panic timeout set, or
>> boot delay set and etc. Seems same applies to printk_delay. Hmm...
>
> Ok - can you cite the old watchdog code, did it really do a nr_cpus
> loop?
>
> Thanks,
>
>         Ingo
>

Yes, previous touch_nmi_watchdog really did a loop as
for_each_present_cpu and touching per-cpu variable

(bad format)

void touch_nmi_watchdog(void)
{
if (nmi_watchdog_active()) {
  unsigned cpu;
  for_each_present_cpu(cpu) {
    if (per_cpu(nmi_touch,cpu) != 1)
       per_cpu(nmi_touch, cpu) = 1;
    }
  }
touch_softlockup_watchdog();
}