fix BUG: using smp_processor_id() in touch_nmi_watchdog and touch_softlockup_watchdog

[   67.703556] BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/5139
[   67.703563] caller is touch_nmi_watchdog+0x15/0x2c
[   67.703566] Pid: 5139, comm: s2disk Not tainted 2.6.36-rc0-git12-07921-g60bf26a-dirty #116
[   67.703568] Call Trace:
[   67.703575]  [<ffffffff811f6bf1>] debug_smp_processor_id+0xc9/0xe4
[   67.703578]  [<ffffffff81092766>] touch_nmi_watchdog+0x15/0x2c
[   67.703584]  [<ffffffff81222950>] acpi_os_stall+0x34/0x40
[   67.703589]  [<ffffffff812398d2>] acpi_ex_system_do_stall+0x34/0x38

---

kernel/watchdog.c +0 -0

index 613bc1f..8822f1e 100644

kernel/watchdog.c +3 -1

 void touch_softlockup_watchdog(void)
 {
-	__get_cpu_var(watchdog_touch_ts) = 0;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_touch_ts, this_cpu) = 0;
+	put_cpu();
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 

Quoted 8 lines · Sergey Senozhatsky

[   67.703556] BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/5139
[   67.703563] caller is touch_nmi_watchdog+0x15/0x2c
[   67.703566] Pid: 5139, comm: s2disk Not tainted 2.6.36-rc0-git12-07921-g60bf26a-dirty #116
[   67.703568] Call Trace:
[   67.703575]  [<ffffffff811f6bf1>] debug_smp_processor_id+0xc9/0xe4
[   67.703578]  [<ffffffff81092766>] touch_nmi_watchdog+0x15/0x2c
[   67.703584]  [<ffffffff81222950>] acpi_os_stall+0x34/0x40
[   67.703589]  [<ffffffff812398d2>] acpi_ex_system_do_stall+0x34/0x38

Which could mean two things, either ACPI got funny on us, or Don's new watchdog stuff has a hole in it.

Quoted 4 lines

---

kernel/watchdog.c static unsigned long get_sample_period(void) · line 120

Quoted 12 lines

kernel/watchdog.c +1 -2

index 613bc1f..8822f1e 100644
@@ -116,13 +116,14 @@ static unsigned long get_sample_period(void)
 static void __touch_watchdog(void)
 {
 	int this_cpu = smp_processor_id();
-
-	__get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+	per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
 }

That change seems sensible enough..

Quoted 11 lines

kernel/watchdog.c +3 -1

 void touch_softlockup_watchdog(void)
 {
-	__get_cpu_var(watchdog_touch_ts) = 0;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_touch_ts, this_cpu) = 0;
+	put_cpu();
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 

kernel/watchdog.c void touch_all_softlockup_watchdogs(void) · line 151

Quoted 10 lines

kernel/watchdog.c +3 -1

@@ -142,7 +143,9 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_nmi_touch, this_cpu) = true;
+	put_cpu();
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);

These other two really are about assumptions we make on the call sites, which at the very least are violated by ACPI.

Don/Ingo, remember if we require touch_*_watchdog callers to have preemption disabled? Or is the proposed patch sensible?

Quoted 2 lines

Don/Ingo, remember if we require touch_*_watchdog callers to have preemption disabled? Or is the proposed patch sensible?

I don't recall any requirement to have preemption disabled when using those functions. It seems sensible to put it in the touch_{softlockup|nmi}_watchdog code.

I assume the reason for having preemption disabled when using smp_processor_id() is that the code could migrate to another cpu when rescheduled?

I don't see a problem with the patch, but my low level understanding of the __get_cpu_var vs. per_cpu isn't very strong.

Fix: acpi_os_stall calls touch_nmi_watchdog and touch_softlockup_watchdog with preemption enabled causing 'BUG: using smp_processor_id() in preemptible code'.

Patch also removes double smp_processor_id call (smp_processor_id itself and in __get_cpu_var) in __touch_watchdog.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

kernel/watchdog.c +7 -4

index 613bc1f..8822f1e 100644
@@ -116,13 +116,14 @@ static unsigned long get_sample_period(void)
 static void __touch_watchdog(void)
 {
 	int this_cpu = smp_processor_id();
-
-	__get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+	per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
 }
 
 void touch_softlockup_watchdog(void)
 {
-	__get_cpu_var(watchdog_touch_ts) = 0;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_touch_ts, this_cpu) = 0;
+	put_cpu();
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 

@@ -142,7 +143,9 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_nmi_touch, this_cpu) = true;
+	put_cpu();
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);

Quoted 2 lines

Patch also removes double smp_processor_id call (smp_processor_id itself and in __get_cpu_var) in __touch_watchdog.

Quoted 2 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Acked-by Don Zickus <[email protected]>

Quoted 27 lines

---

kernel/watchdog.c +4 -3

index 613bc1f..8822f1e 100644
@@ -116,13 +116,14 @@ static unsigned long get_sample_period(void)
  static void __touch_watchdog(void)
  {
       int this_cpu = smp_processor_id();
-
-     __get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+     per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
  }
 
  void touch_softlockup_watchdog(void)
  {
-     __get_cpu_var(watchdog_touch_ts) = 0;
+     int this_cpu = get_cpu();
+     per_cpu(watchdog_touch_ts, this_cpu) = 0;
+     put_cpu();
  }
  EXPORT_SYMBOL(touch_softlockup_watchdog);
 

kernel/watchdog.c void touch_all_softlockup_watchdogs(void) · line 151

Quoted 10 lines

kernel/watchdog.c +3 -1

@@ -142,7 +143,9 @@ void touch_all_softlockup_watchdogs(void)
  #ifdef CONFIG_HARDLOCKUP_DETECTOR
  void touch_nmi_watchdog(void)
  {
-     __get_cpu_var(watchdog_nmi_touch) = true;
+     int this_cpu = get_cpu();
+     per_cpu(watchdog_nmi_touch, this_cpu) = true;
+     put_cpu();
       touch_softlockup_watchdog();
  }
  EXPORT_SYMBOL(touch_nmi_watchdog);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

EXPORT_SYMBOL(touch_softlockup_watchdog);

kernel/watchdog.c +3 -2

@@ -142,7 +143,9 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_nmi_touch, this_cpu) = true;
+	put_cpu();
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

If preemption is disabled and you deal with the current cpu, then please use __get_cpu_var, it makes the code more readable:

void touch_softlockup_watchdog(void)
{
       preempt_disable();
       __(watchdog_touch_ts) = 0;
       preempt_enable();
}

Quoted 11 lines · Frederic Weisbecker

If preemption is disabled and you deal with the current cpu, then please use __get_cpu_var, it makes the code more readable:

void touch_softlockup_watchdog(void)
{
       preempt_disable();
       __(watchdog_touch_ts) = 0;
       preempt_enable();
}

Why not use __raw_get_cpu_var() instead?
You know adding preempt protection in touch_softlockup_watchdog() just suppress the warning. Am I missing something?

Quoted 4 lines

Why not use __raw_get_cpu_var() instead?
You know adding preempt protection in touch_softlockup_watchdog() just suppress the warning. Am I missing something?

Sorry, my low level understanding of the __raw_get_cpu_var isn't very strong. I assume it uses current_thread_info()->cpu in some cases (right?) or percpu_from_op.

Should it be
acpi_os_stall
       preepmt_disable
       touch_nmi_watchdog
               touch_softlockup_watchdog
       preempt_enable

Quoted 8 lines

Quoted 4 lines

Why not use __raw_get_cpu_var() instead?
You know adding preempt protection in touch_softlockup_watchdog() just suppress the warning. Am I missing something?

Sorry, my low level understanding of the __raw_get_cpu_var isn't very strong. I assume it uses current_thread_info()->cpu in some cases (right?) or percpu_from_op.

The difference is __raw_get_cpu_var() is using raw_smp_processor_id().

Quoted 8 lines

Should it be
acpi_os_stall
       preepmt_disable
       touch_nmi_watchdog
               touch_softlockup_watchdog
       preempt_enable

Actually I don't think this is helpful for the whole function. Because if acpi_os_stall()
migrate(I don't know if it could) to another CPU just before preepmt_disable(), we'll
be on the wrong way. Adding preempt protection is just smoothing the warning.

So I prefer using __raw_get_cpu_var() as what we have been done before.

OK. Suppose (I don't know if it could) migration has happen

acpi_os_stall
       __migration__
       touch_nmi_watchdog

How calling raw_smp_processor_id() (which is current_thread_info()->cpu) vs. preepmt_disable - smp_processor_id() will give us different CPUs?

Quoted 3 lines

So I prefer using __raw_get_cpu_var() as what we have been done before.

Hm...

26e09c6eee14f4827b55137ba0eedc4e77cd50ab

Quoted 8 lines · Sergey Senozhatsky

OK. Suppose (I don't know if it could) migration has happen

acpi_os_stall
       __migration__
       touch_nmi_watchdog

How calling raw_smp_processor_id() (which is current_thread_info()->cpu) vs. preepmt_disable - smp_processor_id() will give us different CPUs?

I don't mean you will get different CPUS(sorry for my poor english). I mean if the migration could happen, you want to touch_nmi_watchdog() on CPU A(otherwise the watchdog will shout on us), but eventually we touch_nmi_watchdog() on CPU B(because of migration),
and this is not what we want.

So preempt_disable() is redundant here.

Quoted 7 lines

Quoted 3 lines

So I prefer using __raw_get_cpu_var() as what we have been done before.

Hm...

26e09c6eee14f4827b55137ba0eedc4e77cd50ab

f69bcf60c3f17aa367e16eef7bc6ab001ea6d58a
2508ce1845a3b256798532b2c6b7997c2dc6533b

you can get the previous touch_*_watchdog there.

Thanks,
Yong

Quoted 2 lines

So preempt_disable() is redundant here.

Shouldn't we be for sure not preepmtible when calling __raw_get_cpu_var?

preempt_disable is reduntant here because current_thread_info()->cpu is atomic and we just don't want preempt_(enable|disable) overhead?

Please kindly review.

---

kernel/watchdog.c +0 -0

index 613bc1f..22dd388 100644

kernel/watchdog.c +2 -2

 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 +141,7 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	__raw_get_cpu_var(watchdog_nmi_touch) = true;
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);

Please kindly review.

---

kernel/watchdog.c +3 -4

index 613bc1f..22dd388 100644
@@ -116,13 +116,12 @@ static unsigned long get_sample_period(void)
 static void __touch_watchdog(void)
 {
 	int this_cpu = smp_processor_id();
-
-	__get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+	per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
 }
 
 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 +141,7 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	__raw_get_cpu_var(watchdog_nmi_touch) = true;
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);

Quoted 1 line · Sergey Senozhatsky

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

Quoted 15 lines · Don Zickus

Quoted 1 line · Sergey Senozhatsky

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. :-)

Quoted 2 lines

Cheers,
Don

(Adding Len Brown in 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 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.

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.

From b372e821c804982438db090db6b4a2f753c78091 Mon Sep 17 00:00:00 2001

Quoted 10 lines · Andrew Morton

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.

Quoted 6 lines

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.

Ok, so here is my patch that syncs the touch_*_watchdog back in line with the old semantics. Hopefully this will undo any harm I caused.

------------cut -->---------------------------

Quoted 1 line

From b372e821c804982438db090db6b4a2f753c78091 Mon Sep 17 00:00:00 2001

From: Don Zickus <[email protected]>
Date: Thu, 19 Aug 2010 22:48:26 -0400
Subject: [PATCH] [lockup detector] sync touch_*_watchdog back to old semantics

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 <[email protected]>

---
 kernel/watchdog.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

kernel/watchdog.c +12 -2

index 613bc1f..99e35a2 100644
@@ -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);

@@ -430,6 +437,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;
 }
 

Quoted 20 lines · On Thu, 19 Aug 2010 22:57:49 -0400 Don Zickus <[email protected]> wrote:

On Wed, Aug 18, 2010 at 01:01:56PM -0700, Andrew Morton wrote:

kernel/watchdog.c +3 -3

@@ -430,6 +437,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;
 }
 

@@ -452,9 +462,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;
 }
 

kernel/watchdog.c static void watchdog_disable(int cpu) · line 467

Quoted 1 line

kernel/watchdog.c +0 -0

 static void watchdog_enable_all_cpus(void)

hm, the code seems a bit screwy. Maybe it was always thus.

watchdog_enabled gets set in the per-cpu function but it gets cleared in the all-cpus function. Asymmetric.

Also afacit the action of cpu-hotunplug+cpu-hotplug will reenable the watchdog on a CPU which was supposed to have it disabled. Perhaps you could recheck that and make sure it all makes sense - perhaps we need a separate state variable which is purely "current setting of /proc/sys/kernel/nmi_watchdog" and doesn't get altered internally.

Quoted 10 lines · Andrew Morton

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.

Quoted 6 lines

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.

Ok, so here is my patch that syncs the touch_*_watchdog back in line with the old semantics. Hopefully this will undo any harm I caused.

------------cut -->---------------------------

Quoted 1 line

From b372e821c804982438db090db6b4a2f753c78091 Mon Sep 17 00:00:00 2001

From: Don Zickus <[email protected]>
Date: Thu, 19 Aug 2010 22:48:26 -0400
Subject: [PATCH] [lockup detector] sync touch_*_watchdog back to old semantics

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 <[email protected]>

---
 kernel/watchdog.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

kernel/watchdog.c +12 -5

index 613bc1f..99e35a2 100644
@@ -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);

@@ -430,6 +437,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;
 }
 

@@ -452,9 +462,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)

Signature · 1 line

1.7.2.1

Quoted 10 lines · Andrew Morton

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.

Quoted 6 lines

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.

Ok, so here is my patch that syncs the touch_*_watchdog back in line with the old semantics. Hopefully this will undo any harm I caused.

Quoted 18 lines · Don Zickus

Quoted 16 lines · Andrew Morton

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.

Quoted 2 lines

Ok, so here is my patch that syncs the touch_*_watchdog back in line with the old semantics. Hopefully this will undo any harm I caused.

Was this patch forgotten?

Quoted 2 lines

I don't see a problem with the patch, but my low level understanding of the __get_cpu_var vs. per_cpu isn't very strong.

__get_cpu_var() gets you the value on the current cpu, per_cpu() takes a cpu argument.

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines

I don't see a problem with the patch, but my low level understanding of the __get_cpu_var vs. per_cpu isn't very strong.

__get_cpu_var() gets you the value on the current cpu, per_cpu() takes a cpu argument.

Well I know that much. :-) It seems that __get_cpu_var depends on preemption being disabled whereas per_cpu does not? Though for some reason I thought __get_cpu_var would be more atomic when it grabbed the current cpu such that you wouldn't need to disable preemption. Guess not.

I don't recall any requirement to have preemption disabled when using those functions.

It seems sensible to put it in the
touch_{softlockup|nmi}_watchdog code.

I assume the reason for having preemption disabled when using smp_processor_id() is that the code could migrate to another cpu when rescheduled?

I don't see a problem with the patch, but my low level understanding of the __get_cpu_var vs. per_cpu isn't very strong.

Hello,

Got this traces today:

...

Avoid using smp_processor_id in touch_softlockup_watchdog and touch_nmi_watchdog. Patch also "removes" second call to smp_processor_id in __touch_watchdog (smp_processor_id itself and smp_processor_id in __get_cpu_var).

---

kernel/watchdog.c +0 -0

index 613bc1f..8822f1e 100644

kernel/watchdog.c +3 -1

 void touch_softlockup_watchdog(void)
 {
-	__get_cpu_var(watchdog_touch_ts) = 0;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_touch_ts, this_cpu) = 0;
+	put_cpu();
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 

Quoted 6 lines · On Fri, 13 Aug 2010 13:21:58 +0300 Sergey Senozhatsky…

Hello,

Got this traces today:

...

Quoted 9 lines

void touch_softlockup_watchdog(void)
{

kernel/watchdog.c +3 -1

-	__get_cpu_var(watchdog_touch_ts) = 0;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_touch_ts, this_cpu) = 0;
+	put_cpu();
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);

kernel/watchdog.c void touch_all_softlockup_watchdogs(void) · line 151

Quoted 11 lines

kernel/watchdog.c +3 -1

@@ -142,7 +143,9 @@ void touch_all_softlockup_watchdogs(void)
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-	__get_cpu_var(watchdog_nmi_touch) = true;
+	int this_cpu = get_cpu();
+	per_cpu(watchdog_nmi_touch, this_cpu) = true;
+	put_cpu();
 	touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);

Why did this start happening? Surely we've called touch_softlockup_watchdog() from within preemptible code before now.

Methinks that

: commit 26e09c6eee14f4827b55137ba0eedc4e77cd50ab
: Author: Don Zickus <[email protected]>
: AuthorDate: Mon May 17 18:06:04 2010 -0400
: Commit: Frederic Weisbecker <[email protected]>
: CommitDate: Wed May 19 11:32:14 2010 +0200
:
: lockup_detector: Convert per_cpu to __get_cpu_var for readability

was simply broken? That would be strange, given that it's been sitting around since May 17.

If we don't want to revert 26e09c6eee14f4827b55137ba0eedc4e77cd50ab then I'd suggest that we simply switch to raw_smp_processor_id(): those newly-added get_cpu/put_cpu calls don't do anything useful.

Fair enough, although strictly speaking this should be done in a separate and later patch.

Hello,

Per our previous conversation:

Quoted 3 lines · Andrew Morton

Fair enough, although strictly speaking this should be done in a separate and later patch.

Avoid double smp_processor_id() call in __touch_watchdog (smp_processor_id() itself and later call in __get_cpu_var())

Signed-off-by Sergey Senozhatsky <[email protected]>

---

kernel/watchdog.c +1 -2

index 7f9c3c5..03d97c5 100644
@@ -116,8 +116,7 @@ static unsigned long get_sample_period(void)
 static void __touch_watchdog(void)
 {
 	int this_cpu = smp_processor_id();
-
-	__get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+	per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
 }
 
 void touch_softlockup_watchdog(void)

Hello,

Per our previous conversation:

Quoted 3 lines · Andrew Morton

Fair enough, although strictly speaking this should be done in a separate and later patch.

Avoid double smp_processor_id() call in __touch_watchdog (smp_processor_id() itself and later call in __get_cpu_var())

Signed-off-by Sergey Senozhatsky <[email protected]>

---

kernel/watchdog.c +0 -0

index 7f9c3c5..03d97c5 100644

I'm not sure we want this. This is called by the watchdog internally, from the timer or the cpu bound thread, so we probably should better keep __get_cpu_var() because it checks that we are not in a preemptable section.

Quoted 5 lines · Frederic Weisbecker

I'm not sure we want this. This is called by the watchdog internally, from the timer or the cpu bound thread, so we probably should better keep __get_cpu_var() because it checks that we are not in a preemptable section.

The smp_processor_id() right at the start already does that.

That said, I doubt it really matter one way or the other, compilers have been known to do CSE for quite a while.

Hello,

Per our previous conversation:

Quoted 3 lines · Andrew Morton

Fair enough, although strictly speaking this should be done in a separate and later patch.

Avoid double smp_processor_id() call in __touch_watchdog (smp_processor_id() itself and later call in __get_cpu_var())

Signed-off-by Sergey Senozhatsky <[email protected]>

---

kernel/watchdog.c +1 -2

index 7f9c3c5..03d97c5 100644
@@ -116,8 +116,7 @@ static unsigned long get_sample_period(void)
 static void __touch_watchdog(void)
 {
 	int this_cpu = smp_processor_id();
-
-	__get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+	per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
 }
 
 void touch_softlockup_watchdog(void)

Quoted 10 lines · Sergey Senozhatsky

Hello,

Per our previous conversation:

Quoted 3 lines · Andrew Morton

Fair enough, although strictly speaking this should be done in a separate and later patch.

Quoted 2 lines

Avoid double smp_processor_id() call in __touch_watchdog (smp_processor_id() itself and later call in __get_cpu_var())

Dropping this patch then based on Peter's and Frederic's feedback that the compiler probably already optimizes for this, leaving readability as a good excuse not to change anything.

Quoted 20 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

---

kernel/watchdog.c +1 -2

index 7f9c3c5..03d97c5 100644
@@ -116,8 +116,7 @@ static unsigned long get_sample_period(void)
 static void __touch_watchdog(void)
 {
 	int this_cpu = smp_processor_id();
-
-	__get_cpu_var(watchdog_touch_ts) = get_timestamp(this_cpu);
+	per_cpu(watchdog_touch_ts, this_cpu) = get_timestamp(this_cpu);
 }
 
 void touch_softlockup_watchdog(void)