[Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Rafael J. Wysocki]

Hi Peter,

Your commit 3f6da3905398826d85731247e7fbcf53400c18bd
(perf: Rework and fix the arch CPU-hotplug hooks) unfortunately broke suspend
to RAM (probably hibernation too, but I haven't checked) on my HP nx6325.
I also expect more (if not all) AMD-based boxes to be broken by it.

The machine simply hangs solid while trying to disable CPU1 during suspend.

The patch below fixes it, although I'm not exactly sure why calling
amd_pmu_cpu_offline() during suspend causes the hang to happen.  If there's a
better way to fix this, please let me know.

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: x86 / perf: Fix suspend to RAM on HP nx6325

Commit 3f6da3905398826d85731247e7fbcf53400c18bd
(perf: Rework and fix the arch CPU-hotplug hooks) broke suspend to
RAM on my HP nx6325 (and most likely on other AMD-based boxes too)
by allowing amd_pmu_cpu_offline() to be executed for CPUs that are
going offline as part of the suspend process.  Fix this by modifying
x86_pmu_notifier() so that x86_pmu.cpu_dead(cpu) is only called if
the CPU_TASK_FROZEN bit is unset.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86/kernel/cpu/perf_event.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/cpu/perf_event.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/perf_event.c
+++ linux-2.6/arch/x86/kernel/cpu/perf_event.c
@@ -1334,18 +1334,21 @@ x86_pmu_notifier(struct notifier_block *
 {
 	unsigned int cpu = (long)hcpu;
 
-	switch (action & ~CPU_TASKS_FROZEN) {
+	switch (action) {
 	case CPU_UP_PREPARE:
+	case CPU_UP_PREPARE_FROZEN:
 		if (x86_pmu.cpu_prepare)
 			x86_pmu.cpu_prepare(cpu);
 		break;
 
 	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
 		if (x86_pmu.cpu_starting)
 			x86_pmu.cpu_starting(cpu);
 		break;
 
 	case CPU_DYING:
+	case CPU_DYING_FROZEN:
 		if (x86_pmu.cpu_dying)
 			x86_pmu.cpu_dying(cpu);
 		break;

Re: [Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Peter Zijlstra]

On Sat, 2010-03-20 at 15:07 +0100, Rafael J. Wysocki wrote:
> Hi Peter,
> 
> Your commit 3f6da3905398826d85731247e7fbcf53400c18bd
> (perf: Rework and fix the arch CPU-hotplug hooks) unfortunately broke suspend
> to RAM (probably hibernation too, but I haven't checked) on my HP nx6325.
> I also expect more (if not all) AMD-based boxes to be broken by it.
> 
> The machine simply hangs solid while trying to disable CPU1 during suspend.
> 
> The patch below fixes it, although I'm not exactly sure why calling
> amd_pmu_cpu_offline() during suspend causes the hang to happen.  If there's a
> better way to fix this, please let me know.

Hrmm, I'll have a poke at my AMD once I'm back home again, but does
something like the below cure things for you?

---
 arch/x86/kernel/cpu/perf_event_amd.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index 358a8e3..0189af4 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -345,6 +345,8 @@ static void amd_pmu_cpu_offline(int cpu)
 		return;
 
 	cpuhw = &per_cpu(cpu_hw_events, cpu);
+	if (!cpuhw)
+		return;
 
 	raw_spin_lock(&amd_nb_lock);
 

Re: [Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Linus Torvalds]

On Sat, 20 Mar 2010, Peter Zijlstra wrote:
>  
>  	cpuhw = &per_cpu(cpu_hw_events, cpu);
> +	if (!cpuhw)
> +		return;

How can an address-of expression be NULL?

Yes, 'per_cpu()' is magic, but it shouldn't be possible to be _that_ 
magic. It's rather against the whole C model.

		Linus

Re: [Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Rafael J. Wysocki]

On Saturday 20 March 2010, Linus Torvalds wrote:
> 
> On Sat, 20 Mar 2010, Peter Zijlstra wrote:
> >  
> >  	cpuhw = &per_cpu(cpu_hw_events, cpu);
> > +	if (!cpuhw)
> > +		return;
> 
> How can an address-of expression be NULL?
> 
> Yes, 'per_cpu()' is magic, but it shouldn't be possible to be _that_ 
> magic. It's rather against the whole C model.

Yeah.

A field in the object pointed to by it can be NULL, though.

The appended patch fixes the breakage for me too.

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: x86 / perf: Fix suspend to RAM on HP nx6325

Commit 3f6da3905398826d85731247e7fbcf53400c18bd
(perf: Rework and fix the arch CPU-hotplug hooks) broke suspend to
RAM on my HP nx6325 (and most likely on other AMD-based boxes too)
by allowing amd_pmu_cpu_offline() to be executed for CPUs that are
going offline as part of the suspend process.  The problem is that
cpuhw->amd_nb may be NULL already when amd_pmu_cpu_offline() is called, so the
function should make sure it's not NULL before accessing the object pointed to
by it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86/kernel/cpu/perf_event_amd.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-2.6/arch/x86/kernel/cpu/perf_event_amd.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/perf_event_amd.c
+++ linux-2.6/arch/x86/kernel/cpu/perf_event_amd.c
@@ -348,11 +348,15 @@ static void amd_pmu_cpu_offline(int cpu)
 
 	raw_spin_lock(&amd_nb_lock);
 
+	if (!cpuhw->amd_nb)
+		goto unlock;
+
 	if (--cpuhw->amd_nb->refcnt == 0)
 		kfree(cpuhw->amd_nb);
 
 	cpuhw->amd_nb = NULL;
 
+ unlock:
 	raw_spin_unlock(&amd_nb_lock);
 }
 

Re: [Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Peter Zijlstra]

On Sat, 2010-03-20 at 21:48 +0100, Rafael J. Wysocki wrote:
> On Saturday 20 March 2010, Linus Torvalds wrote:
> > 
> > On Sat, 20 Mar 2010, Peter Zijlstra wrote:
> > >  
> > >  	cpuhw = &per_cpu(cpu_hw_events, cpu);
> > > +	if (!cpuhw)
> > > +		return;
> > 
> > How can an address-of expression be NULL?
> > 
> > Yes, 'per_cpu()' is magic, but it shouldn't be possible to be _that_ 
> > magic. It's rather against the whole C model.
> 
> Yeah.
> 
> A field in the object pointed to by it can be NULL, though.

That's what I was aiming for,. might have missed it by a mile though :/

> The appended patch fixes the breakage for me too.

ok, works for me, thanks

Re: [Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Linus Torvalds]

On Sat, 20 Mar 2010, Rafael J. Wysocki wrote:
>
> The appended patch fixes the breakage for me too.

Please don't use a 'goto' for something like this.

>  	raw_spin_lock(&amd_nb_lock);
>  
> +	if (!cpuhw->amd_nb)
> +		goto unlock;
> +
>  	if (--cpuhw->amd_nb->refcnt == 0)
>  		kfree(cpuhw->amd_nb);
>  
>  	cpuhw->amd_nb = NULL;
>  
> + unlock:
>  	raw_spin_unlock(&amd_nb_lock);
>  }

Just do

	raw_spin_lock(&amd_nb_lock);
	if (cpuhw->amd_nb) {
		if (!--cpuhw->amd_nb->refcnt)
			kfree(cpuhw->amd_nb);
		cpuhw->amd_nb = NULL;
	}
	raw_spin_unlock(&amd_nb_lock);

instead. Much more readable.

Let's keep 'goto' for cases where we have error returns that we don't want 
to nest, not trivial stuff like this.

		Linus

Re: [Regression, post-2.6.34-rc1][PATCH] x86 / perf: Fix suspend to RAM on HP nx6325

[Rafael J. Wysocki]

On Sunday 21 March 2010, Linus Torvalds wrote:
> 
> On Sat, 20 Mar 2010, Rafael J. Wysocki wrote:
> >
> > The appended patch fixes the breakage for me too.
> 
> Please don't use a 'goto' for something like this.
> 
> >  	raw_spin_lock(&amd_nb_lock);
> >  
> > +	if (!cpuhw->amd_nb)
> > +		goto unlock;
> > +
> >  	if (--cpuhw->amd_nb->refcnt == 0)
> >  		kfree(cpuhw->amd_nb);
> >  
> >  	cpuhw->amd_nb = NULL;
> >  
> > + unlock:
> >  	raw_spin_unlock(&amd_nb_lock);
> >  }
> 
> Just do
> 
> 	raw_spin_lock(&amd_nb_lock);
> 	if (cpuhw->amd_nb) {
> 		if (!--cpuhw->amd_nb->refcnt)
> 			kfree(cpuhw->amd_nb);
> 		cpuhw->amd_nb = NULL;
> 	}
> 	raw_spin_unlock(&amd_nb_lock);
> 
> instead. Much more readable.
> 
> Let's keep 'goto' for cases where we have error returns that we don't want 
> to nest, not trivial stuff like this.

OK

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: x86 / perf: Fix suspend to RAM on HP nx6325

Commit 3f6da3905398826d85731247e7fbcf53400c18bd
(perf: Rework and fix the arch CPU-hotplug hooks) broke suspend to
RAM on my HP nx6325 (and most likely on other AMD-based boxes too)
by allowing amd_pmu_cpu_offline() to be executed for CPUs that are
going offline as part of the suspend process.  The problem is that
cpuhw->amd_nb may be NULL already, so the function should make sure
it's not NULL before accessing the object pointed to by it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86/kernel/cpu/perf_event_amd.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/arch/x86/kernel/cpu/perf_event_amd.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/perf_event_amd.c
+++ linux-2.6/arch/x86/kernel/cpu/perf_event_amd.c
@@ -348,10 +348,12 @@ static void amd_pmu_cpu_offline(int cpu)
 
 	raw_spin_lock(&amd_nb_lock);
 
-	if (--cpuhw->amd_nb->refcnt == 0)
-		kfree(cpuhw->amd_nb);
+	if (cpuhw->amd_nb) {
+		if (--cpuhw->amd_nb->refcnt == 0)
+			kfree(cpuhw->amd_nb);
 
-	cpuhw->amd_nb = NULL;
+		cpuhw->amd_nb = NULL;
+	}
 
 	raw_spin_unlock(&amd_nb_lock);
 }