[PATCH] perf_events: fix bug in AMD per-cpu initialization

[PATCH] perf_events: fix bug in AMD per-cpu initialization

[Stephane Eranian]

	On AMD processors, we need to allocate a data structure per Northbridge
	to handle certain events.

	On CPU initialization, we need to query the Northbridge id and check
	whether the structure is already allocated or not. We use the
	amd_get_nb_id() function to request the Northbridge identification.

	The recent cleanup of the CPU online/offline initialization introduced
	a bug. AMD cpu initialization is invoked on CPU_UP_PREPARE callback.
	This is before the CPU Northbridge id is calculated. Therefore no
	processor had a Northbridge structure allocated except the boot
	processor. That was causing bogus NB event scheduling.

	This patch uses the CPU_ONLINE callback to initialize the AMD
	Northbridge structure. This way amd_get_nb_id() returns valid
	information.

	The x86_cpu_up() callback was added. Could not call it cpu_online
	because of existing macro.

	Signed-off-by: Stephane Eranian <eranian@google.com>
	
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 978d297..016c25a 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -213,6 +213,7 @@ struct x86_pmu {
 	void		(*quirks)(void);
 
 	void		(*cpu_prepare)(int cpu);
+	void		(*cpu_up)(int cpu);
 	void		(*cpu_starting)(int cpu);
 	void		(*cpu_dying)(int cpu);
 	void		(*cpu_dead)(int cpu);
@@ -1342,6 +1343,11 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 			x86_pmu.cpu_starting(cpu);
 		break;
 
+	case CPU_ONLINE:
+		if (x86_pmu.cpu_up)
+			x86_pmu.cpu_up(cpu);
+		break;
+
 	case CPU_DYING:
 		if (x86_pmu.cpu_dying)
 			x86_pmu.cpu_dying(cpu);
diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index 358a8e3..584df49 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -346,6 +346,9 @@ static void amd_pmu_cpu_offline(int cpu)
 
 	cpuhw = &per_cpu(cpu_hw_events, cpu);
 
+	if (!cpuhw->amd_nb)
+		return;
+
 	raw_spin_lock(&amd_nb_lock);
 
 	if (--cpuhw->amd_nb->refcnt == 0)
@@ -379,7 +382,7 @@ static __initconst struct x86_pmu amd_pmu = {
 	.get_event_constraints	= amd_get_event_constraints,
 	.put_event_constraints	= amd_put_event_constraints,
 
-	.cpu_prepare		= amd_pmu_cpu_online,
+	.cpu_up			= amd_pmu_cpu_online,
 	.cpu_dead		= amd_pmu_cpu_offline,
 };
 

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Peter Zijlstra]

On Wed, 2010-03-17 at 10:40 +0200, Stephane Eranian wrote:
> 	On AMD processors, we need to allocate a data structure per Northbridge
> 	to handle certain events.
> 
> 	On CPU initialization, we need to query the Northbridge id and check
> 	whether the structure is already allocated or not. We use the
> 	amd_get_nb_id() function to request the Northbridge identification.
> 
> 	The recent cleanup of the CPU online/offline initialization introduced
> 	a bug. AMD cpu initialization is invoked on CPU_UP_PREPARE callback.
> 	This is before the CPU Northbridge id is calculated. Therefore no
> 	processor had a Northbridge structure allocated except the boot
> 	processor. That was causing bogus NB event scheduling.
> 
> 	This patch uses the CPU_ONLINE callback to initialize the AMD
> 	Northbridge structure. This way amd_get_nb_id() returns valid
> 	information.
> 
> 	The x86_cpu_up() callback was added. Could not call it cpu_online
> 	because of existing macro.
> 
> 	Signed-off-by: Stephane Eranian <eranian@google.com>


No, ONLINE is not exposed for a good reason, its always wrong.

Use prepare to allocate data, and starting to initialize stuff on the
cpu proper once its up. Online is after the cpu is up and running and we
are already scheduling stuff on it so its too late to initialize things.

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Stephane Eranian]

On Thu, Mar 18, 2010 at 12:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2010-03-17 at 10:40 +0200, Stephane Eranian wrote:
>>       On AMD processors, we need to allocate a data structure per Northbridge
>>       to handle certain events.
>>
>>       On CPU initialization, we need to query the Northbridge id and check
>>       whether the structure is already allocated or not. We use the
>>       amd_get_nb_id() function to request the Northbridge identification.
>>
>>       The recent cleanup of the CPU online/offline initialization introduced
>>       a bug. AMD cpu initialization is invoked on CPU_UP_PREPARE callback.
>>       This is before the CPU Northbridge id is calculated. Therefore no
>>       processor had a Northbridge structure allocated except the boot
>>       processor. That was causing bogus NB event scheduling.
>>
>>       This patch uses the CPU_ONLINE callback to initialize the AMD
>>       Northbridge structure. This way amd_get_nb_id() returns valid
>>       information.
>>
>>       The x86_cpu_up() callback was added. Could not call it cpu_online
>>       because of existing macro.
>>
>>       Signed-off-by: Stephane Eranian <eranian@google.com>
>
>
> No, ONLINE is not exposed for a good reason, its always wrong.
>
> Use prepare to allocate data, and starting to initialize stuff on the
> cpu proper once its up. Online is after the cpu is up and running and we
> are already scheduling stuff on it so its too late to initialize things.
>
>
I can't because amd_get_nb_id() returns garbage for the CPU
when you call from CPU_STARTING. So looks like initialization
of cpu_llc_id needs to be moved way earlier. I don't want to have
to duplicate that code.

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Peter Zijlstra]

On Thu, 2010-03-18 at 01:33 +0100, Stephane Eranian wrote: 
> On Thu, Mar 18, 2010 at 12:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, 2010-03-17 at 10:40 +0200, Stephane Eranian wrote:
> >>       On AMD processors, we need to allocate a data structure per Northbridge
> >>       to handle certain events.
> >>
> >>       On CPU initialization, we need to query the Northbridge id and check
> >>       whether the structure is already allocated or not. We use the
> >>       amd_get_nb_id() function to request the Northbridge identification.
> >>
> >>       The recent cleanup of the CPU online/offline initialization introduced
> >>       a bug. AMD cpu initialization is invoked on CPU_UP_PREPARE callback.
> >>       This is before the CPU Northbridge id is calculated. Therefore no
> >>       processor had a Northbridge structure allocated except the boot
> >>       processor. That was causing bogus NB event scheduling.
> >>
> >>       This patch uses the CPU_ONLINE callback to initialize the AMD
> >>       Northbridge structure. This way amd_get_nb_id() returns valid
> >>       information.
> >>
> >>       The x86_cpu_up() callback was added. Could not call it cpu_online
> >>       because of existing macro.
> >>
> >>       Signed-off-by: Stephane Eranian <eranian@google.com>
> >
> >
> > No, ONLINE is not exposed for a good reason, its always wrong.
> >
> > Use prepare to allocate data, and starting to initialize stuff on the
> > cpu proper once its up. Online is after the cpu is up and running and we
> > are already scheduling stuff on it so its too late to initialize things.
> >
> >
> I can't because amd_get_nb_id() returns garbage for the CPU
> when you call from CPU_STARTING. So looks like initialization
> of cpu_llc_id needs to be moved way earlier. I don't want to have
> to duplicate that code.

Crap, you're right, either notify_cpu_starting() is done too early or
smp_store_cpu_info() is done too late.

Since smp_store_cpu_info() relies on the result of calibrate_delay() we
can't easily change that order, but since there really isn't any other
CPU_STARTING user in tree (I appear to have created the first?!) we can
easily move that notifier thing later.

(What's up with that IRQ-enable over calibrate_delay(), can't we simply
enable the NMI watchdog later?)

So I guess something like the below should work:

---
arch/x86/kernel/smpboot.c |    4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 06d98ae..6808b93 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -242,8 +242,6 @@ static void __cpuinit smp_callin(void)
end_local_APIC_setup();
map_cpu_to_logical_apicid();

- notify_cpu_starting(cpuid);
-
/*
* Need to setup vector mappings before we enable interrupts.
*/
@@ -264,6 +262,8 @@ static void __cpuinit smp_callin(void)
*/
smp_store_cpu_info(cpuid);

+ notify_cpu_starting(cpuid);
+
/*
* Allow the master to continue.
*/


Which then allows us to write something like:

---

 arch/x86/kernel/cpu/perf_event.c     |    8 ++-
 arch/x86/kernel/cpu/perf_event_amd.c |   69 ++++++++++++++++++---------------
 2 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index f571f51..4db6eaf 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -209,7 +209,7 @@ struct x86_pmu {
 	struct event_constraint *event_constraints;
 	void		(*quirks)(void);
 
-	void		(*cpu_prepare)(int cpu);
+	int		(*cpu_prepare)(int cpu);
 	void		(*cpu_starting)(int cpu);
 	void		(*cpu_dying)(int cpu);
 	void		(*cpu_dead)(int cpu);
@@ -1330,11 +1330,12 @@ static int __cpuinit
 x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 {
 	unsigned int cpu = (long)hcpu;
+	int ret = NOTIFY_OK;
 
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_UP_PREPARE:
 		if (x86_pmu.cpu_prepare)
-			x86_pmu.cpu_prepare(cpu);
+			ret = x86_pmu.cpu_prepare(cpu);
 		break;
 
 	case CPU_STARTING:
@@ -1347,6 +1348,7 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 			x86_pmu.cpu_dying(cpu);
 		break;
 
+	case CPU_UP_CANCELED:
 	case CPU_DEAD:
 		if (x86_pmu.cpu_dead)
 			x86_pmu.cpu_dead(cpu);
@@ -1356,7 +1358,7 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 		break;
 	}
 
-	return NOTIFY_OK;
+	return ret;
 }
 
 static void __init pmu_check_apic(void)
diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index bdd8f8e..b78c6c5 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -293,51 +293,55 @@ static struct amd_nb *amd_alloc_nb(int cpu, int nb_id)
 	return nb;
 }
 
-static void amd_pmu_cpu_online(int cpu)
+static int amd_pmu_cpu_prepare(int cpu)
 {
-	struct cpu_hw_events *cpu1, *cpu2;
-	struct amd_nb *nb = NULL;
+	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
+
+	WARN_ON_ONCE(cpuc->amd_nb);
+
+	if (boot_cpu_data.x86_max_cores < 2)
+		return NOTIFY_OK;
+
+	cpuc->amd_nb = amd_alloc_nb(cpu, -1);
+	if (!cpuc->amd_nb)
+		return NOTIFY_BAD;
+
+	return NOTIFY_OK;
+}
+
+static void amd_pmu_cpu_starting(int cpu)
+{
+	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
+	struct amd_nb *nb;
 	int i, nb_id;
 
 	if (boot_cpu_data.x86_max_cores < 2)
 		return;
 
-	/*
-	 * function may be called too early in the
-	 * boot process, in which case nb_id is bogus
-	 */
 	nb_id = amd_get_nb_id(cpu);
-	if (nb_id == BAD_APICID)
-		return;
-
-	cpu1 = &per_cpu(cpu_hw_events, cpu);
-	cpu1->amd_nb = NULL;
+	WARN_ON_ONCE(nb_id == BAD_APICID);
 
 	raw_spin_lock(&amd_nb_lock);
 
 	for_each_online_cpu(i) {
-		cpu2 = &per_cpu(cpu_hw_events, i);
-		nb = cpu2->amd_nb;
-		if (!nb)
+		nb = per_cpu(cpu_hw_events, i).amd_nb;
+		if (WARN_ON_ONCE(!nb))
 			continue;
-		if (nb->nb_id == nb_id)
-			goto found;
-	}
 
-	nb = amd_alloc_nb(cpu, nb_id);
-	if (!nb) {
-		pr_err("perf_events: failed NB allocation for CPU%d\n", cpu);
-		raw_spin_unlock(&amd_nb_lock);
-		return;
+		if (nb->nb_id == nb_id) {
+			kfree(cpuc->amd_nb);
+			cpuc->amd_nb = nb;
+			break;
+		}
 	}
-found:
-	nb->refcnt++;
-	cpu1->amd_nb = nb;
+
+	cpuc->amd_nb->nb_id = nb_id;
+	cpuc->amd_nb->refcnt++;
 
 	raw_spin_unlock(&amd_nb_lock);
 }
 
-static void amd_pmu_cpu_offline(int cpu)
+static void amd_pmu_cpu_dead(int cpu)
 {
 	struct cpu_hw_events *cpuhw;
 
@@ -349,8 +353,10 @@ static void amd_pmu_cpu_offline(int cpu)
 	raw_spin_lock(&amd_nb_lock);
 
 	if (cpuhw->amd_nb) {
-		if (--cpuhw->amd_nb->refcnt == 0)
-			kfree(cpuhw->amd_nb);
+		struct amd_nb *nb = cpuhw->amd_nb;
+
+		if (nb->nb_id == -1 || --nb->refcnt == 0)
+			kfree(nb);
 
 		cpuhw->amd_nb = NULL;
 	}
@@ -381,8 +387,9 @@ static __initconst struct x86_pmu amd_pmu = {
 	.get_event_constraints	= amd_get_event_constraints,
 	.put_event_constraints	= amd_put_event_constraints,
 
-	.cpu_prepare		= amd_pmu_cpu_online,
-	.cpu_dead		= amd_pmu_cpu_offline,
+	.cpu_prepare		= amd_pmu_cpu_prepare,
+	.cpu_starting		= amd_pmu_cpu_starting,
+	.cpu_dead		= amd_pmu_cpu_dead,
 };
 
 static __init int amd_pmu_init(void)

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Stephane Eranian]

On Tue, Mar 23, 2010 at 3:11 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, 2010-03-18 at 01:33 +0100, Stephane Eranian wrote:
>> On Thu, Mar 18, 2010 at 12:47 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Wed, 2010-03-17 at 10:40 +0200, Stephane Eranian wrote:
>> >>       On AMD processors, we need to allocate a data structure per Northbridge
>> >>       to handle certain events.
>> >>
>> >>       On CPU initialization, we need to query the Northbridge id and check
>> >>       whether the structure is already allocated or not. We use the
>> >>       amd_get_nb_id() function to request the Northbridge identification.
>> >>
>> >>       The recent cleanup of the CPU online/offline initialization introduced
>> >>       a bug. AMD cpu initialization is invoked on CPU_UP_PREPARE callback.
>> >>       This is before the CPU Northbridge id is calculated. Therefore no
>> >>       processor had a Northbridge structure allocated except the boot
>> >>       processor. That was causing bogus NB event scheduling.
>> >>
>> >>       This patch uses the CPU_ONLINE callback to initialize the AMD
>> >>       Northbridge structure. This way amd_get_nb_id() returns valid
>> >>       information.
>> >>
>> >>       The x86_cpu_up() callback was added. Could not call it cpu_online
>> >>       because of existing macro.
>> >>
>> >>       Signed-off-by: Stephane Eranian <eranian@google.com>
>> >
>> >
>> > No, ONLINE is not exposed for a good reason, its always wrong.
>> >
>> > Use prepare to allocate data, and starting to initialize stuff on the
>> > cpu proper once its up. Online is after the cpu is up and running and we
>> > are already scheduling stuff on it so its too late to initialize things.
>> >
>> >
>> I can't because amd_get_nb_id() returns garbage for the CPU
>> when you call from CPU_STARTING. So looks like initialization
>> of cpu_llc_id needs to be moved way earlier. I don't want to have
>> to duplicate that code.
>
> Crap, you're right, either notify_cpu_starting() is done too early or
> smp_store_cpu_info() is done too late.
>
> Since smp_store_cpu_info() relies on the result of calibrate_delay() we
> can't easily change that order, but since there really isn't any other
> CPU_STARTING user in tree (I appear to have created the first?!) we can
> easily move that notifier thing later.
>
What's the point of CPU_ONLINE vs. CPU_STARTING if you're saying the
former is never right? Why not move CPU_ONLINE to the right place and drop
CPU_STARTING?



> (What's up with that IRQ-enable over calibrate_delay(), can't we simply
> enable the NMI watchdog later?)
>
> So I guess something like the below should work:
>
> ---
> arch/x86/kernel/smpboot.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 06d98ae..6808b93 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -242,8 +242,6 @@ static void __cpuinit smp_callin(void)
> end_local_APIC_setup();
> map_cpu_to_logical_apicid();
>
> - notify_cpu_starting(cpuid);
> -
> /*
> * Need to setup vector mappings before we enable interrupts.
> */
> @@ -264,6 +262,8 @@ static void __cpuinit smp_callin(void)
> */
> smp_store_cpu_info(cpuid);
>
> + notify_cpu_starting(cpuid);
> +
> /*
> * Allow the master to continue.
> */
>
>
> Which then allows us to write something like:
>
> ---
>
>  arch/x86/kernel/cpu/perf_event.c     |    8 ++-
>  arch/x86/kernel/cpu/perf_event_amd.c |   69 ++++++++++++++++++---------------
>  2 files changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index f571f51..4db6eaf 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -209,7 +209,7 @@ struct x86_pmu {
>        struct event_constraint *event_constraints;
>        void            (*quirks)(void);
>
> -       void            (*cpu_prepare)(int cpu);
> +       int             (*cpu_prepare)(int cpu);
>        void            (*cpu_starting)(int cpu);
>        void            (*cpu_dying)(int cpu);
>        void            (*cpu_dead)(int cpu);
> @@ -1330,11 +1330,12 @@ static int __cpuinit
>  x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
>  {
>        unsigned int cpu = (long)hcpu;
> +       int ret = NOTIFY_OK;
>
>        switch (action & ~CPU_TASKS_FROZEN) {
>        case CPU_UP_PREPARE:
>                if (x86_pmu.cpu_prepare)
> -                       x86_pmu.cpu_prepare(cpu);
> +                       ret = x86_pmu.cpu_prepare(cpu);
>                break;
>
>        case CPU_STARTING:
> @@ -1347,6 +1348,7 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
>                        x86_pmu.cpu_dying(cpu);
>                break;
>
> +       case CPU_UP_CANCELED:
>        case CPU_DEAD:
>                if (x86_pmu.cpu_dead)
>                        x86_pmu.cpu_dead(cpu);
> @@ -1356,7 +1358,7 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
>                break;
>        }
>
> -       return NOTIFY_OK;
> +       return ret;
>  }
>
>  static void __init pmu_check_apic(void)
> diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
> index bdd8f8e..b78c6c5 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd.c
> @@ -293,51 +293,55 @@ static struct amd_nb *amd_alloc_nb(int cpu, int nb_id)
>        return nb;
>  }
>
> -static void amd_pmu_cpu_online(int cpu)
> +static int amd_pmu_cpu_prepare(int cpu)
>  {
> -       struct cpu_hw_events *cpu1, *cpu2;
> -       struct amd_nb *nb = NULL;
> +       struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
> +
> +       WARN_ON_ONCE(cpuc->amd_nb);
> +
> +       if (boot_cpu_data.x86_max_cores < 2)
> +               return NOTIFY_OK;
> +
> +       cpuc->amd_nb = amd_alloc_nb(cpu, -1);
> +       if (!cpuc->amd_nb)
> +               return NOTIFY_BAD;
> +
> +       return NOTIFY_OK;
> +}
> +
> +static void amd_pmu_cpu_starting(int cpu)
> +{
> +       struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
> +       struct amd_nb *nb;
>        int i, nb_id;
>
>        if (boot_cpu_data.x86_max_cores < 2)
>                return;
>
> -       /*
> -        * function may be called too early in the
> -        * boot process, in which case nb_id is bogus
> -        */
>        nb_id = amd_get_nb_id(cpu);
> -       if (nb_id == BAD_APICID)
> -               return;
> -
> -       cpu1 = &per_cpu(cpu_hw_events, cpu);
> -       cpu1->amd_nb = NULL;
> +       WARN_ON_ONCE(nb_id == BAD_APICID);
>
>        raw_spin_lock(&amd_nb_lock);
>
>        for_each_online_cpu(i) {
> -               cpu2 = &per_cpu(cpu_hw_events, i);
> -               nb = cpu2->amd_nb;
> -               if (!nb)
> +               nb = per_cpu(cpu_hw_events, i).amd_nb;
> +               if (WARN_ON_ONCE(!nb))
>                        continue;
> -               if (nb->nb_id == nb_id)
> -                       goto found;
> -       }
>
> -       nb = amd_alloc_nb(cpu, nb_id);
> -       if (!nb) {
> -               pr_err("perf_events: failed NB allocation for CPU%d\n", cpu);
> -               raw_spin_unlock(&amd_nb_lock);
> -               return;
> +               if (nb->nb_id == nb_id) {
> +                       kfree(cpuc->amd_nb);
> +                       cpuc->amd_nb = nb;
> +                       break;
> +               }
>        }
> -found:
> -       nb->refcnt++;
> -       cpu1->amd_nb = nb;
> +
> +       cpuc->amd_nb->nb_id = nb_id;
> +       cpuc->amd_nb->refcnt++;
>
>        raw_spin_unlock(&amd_nb_lock);
>  }
>
> -static void amd_pmu_cpu_offline(int cpu)
> +static void amd_pmu_cpu_dead(int cpu)
>  {
>        struct cpu_hw_events *cpuhw;
>
> @@ -349,8 +353,10 @@ static void amd_pmu_cpu_offline(int cpu)
>        raw_spin_lock(&amd_nb_lock);
>
>        if (cpuhw->amd_nb) {
> -               if (--cpuhw->amd_nb->refcnt == 0)
> -                       kfree(cpuhw->amd_nb);
> +               struct amd_nb *nb = cpuhw->amd_nb;
> +
> +               if (nb->nb_id == -1 || --nb->refcnt == 0)
> +                       kfree(nb);
>
>                cpuhw->amd_nb = NULL;
>        }
> @@ -381,8 +387,9 @@ static __initconst struct x86_pmu amd_pmu = {
>        .get_event_constraints  = amd_get_event_constraints,
>        .put_event_constraints  = amd_put_event_constraints,
>
> -       .cpu_prepare            = amd_pmu_cpu_online,
> -       .cpu_dead               = amd_pmu_cpu_offline,
> +       .cpu_prepare            = amd_pmu_cpu_prepare,
> +       .cpu_starting           = amd_pmu_cpu_starting,
> +       .cpu_dead               = amd_pmu_cpu_dead,
>  };
>
>  static __init int amd_pmu_init(void)
>
>



-- 
Stephane Eranian  | EMEA Software Engineering
Google France | 38 avenue de l'Opéra | 75002 Paris
Tel : +33 (0) 1 42 68 53 00
This email may be confidential or privileged. If you received this
communication by mistake, please
don't forward it to anyone else, please erase all copies and
attachments, and please let me know that
it went to the wrong person. Thanks

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Peter Zijlstra]

On Tue, 2010-03-23 at 15:55 +0100, Stephane Eranian wrote:
> What's the point of CPU_ONLINE vs. CPU_STARTING if you're saying the
> former is never right? Why not move CPU_ONLINE to the right place and
> drop CPU_STARTING?

Its right for a lot of things, just not for perf, we need to be ready
and done by the time the cpu starts scheduling.

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Stephane Eranian]

On Tue, Mar 23, 2010 at 4:07 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-03-23 at 15:55 +0100, Stephane Eranian wrote:
>> What's the point of CPU_ONLINE vs. CPU_STARTING if you're saying the
>> former is never right? Why not move CPU_ONLINE to the right place and
>> drop CPU_STARTING?
>
> Its right for a lot of things, just not for perf, we need to be ready
> and done by the time the cpu starts scheduling.
>
You mean they need to wait until after the cpu starts scheduling?
As opposed to being called just before it starts scheduling.

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Peter Zijlstra]

On Tue, 2010-03-23 at 16:12 +0100, Stephane Eranian wrote:
> On Tue, Mar 23, 2010 at 4:07 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, 2010-03-23 at 15:55 +0100, Stephane Eranian wrote:
> >> What's the point of CPU_ONLINE vs. CPU_STARTING if you're saying the
> >> former is never right? Why not move CPU_ONLINE to the right place and
> >> drop CPU_STARTING?
> >
> > Its right for a lot of things, just not for perf, we need to be ready
> > and done by the time the cpu starts scheduling.
> >
> You mean they need to wait until after the cpu starts scheduling?
> As opposed to being called just before it starts scheduling.

As in it doesn't really matter for them, and the CPU_ONLINE call is
convenient in that it allows the callback to schedule too.

Re: [PATCH] perf_events: fix bug in AMD per-cpu initialization

[Stephane Eranian]

On Tue, Mar 23, 2010 at 4:18 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-03-23 at 16:12 +0100, Stephane Eranian wrote:
>> On Tue, Mar 23, 2010 at 4:07 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Tue, 2010-03-23 at 15:55 +0100, Stephane Eranian wrote:
>> >> What's the point of CPU_ONLINE vs. CPU_STARTING if you're saying the
>> >> former is never right? Why not move CPU_ONLINE to the right place and
>> >> drop CPU_STARTING?
>> >
>> > Its right for a lot of things, just not for perf, we need to be ready
>> > and done by the time the cpu starts scheduling.
>> >
>> You mean they need to wait until after the cpu starts scheduling?
>> As opposed to being called just before it starts scheduling.
>
> As in it doesn't really matter for them, and the CPU_ONLINE call is
> convenient in that it allows the callback to schedule too.
>
Fine, I will try your proposed patch tomorrow. A priori, it looks fine.

[tip:perf/core] x86: Move notify_cpu_starting() callback to a later stage

[tip-bot for Peter Zijlstra]

Commit-ID:  85257024096a96fc5c00ce59d685f62bbed3ad95
Gitweb:     http://git.kernel.org/tip/85257024096a96fc5c00ce59d685f62bbed3ad95
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Tue, 23 Mar 2010 19:30:52 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 2 Apr 2010 19:30:01 +0200

x86: Move notify_cpu_starting() callback to a later stage

Because we need to have cpu identification things done by the time we run
CPU_STARTING notifiers.

( This init ordering will be relied on by the next fix. )

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1269353485.5109.48.camel@twins>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/smpboot.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 06d98ae..6808b93 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -242,8 +242,6 @@ static void __cpuinit smp_callin(void)
 	end_local_APIC_setup();
 	map_cpu_to_logical_apicid();
 
-	notify_cpu_starting(cpuid);
-
 	/*
 	 * Need to setup vector mappings before we enable interrupts.
 	 */
@@ -264,6 +262,8 @@ static void __cpuinit smp_callin(void)
 	 */
 	smp_store_cpu_info(cpuid);
 
+	notify_cpu_starting(cpuid);
+
 	/*
 	 * Allow the master to continue.
 	 */

[tip:perf/core] perf, x86: Fix AMD hotplug & constraint initialization

[tip-bot for Peter Zijlstra]

Commit-ID:  b38b24ead33417146e051453d04bf60b8d2d7e25
Gitweb:     http://git.kernel.org/tip/b38b24ead33417146e051453d04bf60b8d2d7e25
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Tue, 23 Mar 2010 19:31:15 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 2 Apr 2010 19:30:02 +0200

perf, x86: Fix AMD hotplug & constraint initialization

Commit 3f6da39 ("perf: Rework and fix the arch CPU-hotplug hooks") moved
the amd northbridge allocation from CPUS_ONLINE to CPUS_PREPARE_UP
however amd_nb_id() doesn't work yet on prepare so it would simply bail
basically reverting to a state where we do not properly track node wide
constraints - causing weird perf results.

Fix up the AMD NorthBridge initialization code by allocating from
CPU_UP_PREPARE and installing it from CPU_STARTING once we have the
proper nb_id. It also properly deals with the allocation failing.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
[ robustify using amd_has_nb() ]
Signed-off-by: Stephane Eranian <eranian@google.com>
LKML-Reference: <1269353485.5109.48.camel@twins>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/perf_event.c     |    8 ++-
 arch/x86/kernel/cpu/perf_event_amd.c |   80 ++++++++++++++++++++--------------
 2 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 5fb490c..bd28cf9 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -158,7 +158,7 @@ struct x86_pmu {
 						 struct perf_event *event);
 	struct event_constraint *event_constraints;
 
-	void		(*cpu_prepare)(int cpu);
+	int		(*cpu_prepare)(int cpu);
 	void		(*cpu_starting)(int cpu);
 	void		(*cpu_dying)(int cpu);
 	void		(*cpu_dead)(int cpu);
@@ -1333,11 +1333,12 @@ static int __cpuinit
 x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 {
 	unsigned int cpu = (long)hcpu;
+	int ret = NOTIFY_OK;
 
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_UP_PREPARE:
 		if (x86_pmu.cpu_prepare)
-			x86_pmu.cpu_prepare(cpu);
+			ret = x86_pmu.cpu_prepare(cpu);
 		break;
 
 	case CPU_STARTING:
@@ -1350,6 +1351,7 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 			x86_pmu.cpu_dying(cpu);
 		break;
 
+	case CPU_UP_CANCELED:
 	case CPU_DEAD:
 		if (x86_pmu.cpu_dead)
 			x86_pmu.cpu_dead(cpu);
@@ -1359,7 +1361,7 @@ x86_pmu_notifier(struct notifier_block *self, unsigned long action, void *hcpu)
 		break;
 	}
 
-	return NOTIFY_OK;
+	return ret;
 }
 
 static void __init pmu_check_apic(void)
diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index b87e0b6..db6f7d4 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -137,6 +137,13 @@ static inline int amd_is_nb_event(struct hw_perf_event *hwc)
 	return (hwc->config & 0xe0) == 0xe0;
 }
 
+static inline int amd_has_nb(struct cpu_hw_events *cpuc)
+{
+	struct amd_nb *nb = cpuc->amd_nb;
+
+	return nb && nb->nb_id != -1;
+}
+
 static void amd_put_event_constraints(struct cpu_hw_events *cpuc,
 				      struct perf_event *event)
 {
@@ -147,7 +154,7 @@ static void amd_put_event_constraints(struct cpu_hw_events *cpuc,
 	/*
 	 * only care about NB events
 	 */
-	if (!(nb && amd_is_nb_event(hwc)))
+	if (!(amd_has_nb(cpuc) && amd_is_nb_event(hwc)))
 		return;
 
 	/*
@@ -214,7 +221,7 @@ amd_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
 	/*
 	 * if not NB event or no NB, then no constraints
 	 */
-	if (!(nb && amd_is_nb_event(hwc)))
+	if (!(amd_has_nb(cpuc) && amd_is_nb_event(hwc)))
 		return &unconstrained;
 
 	/*
@@ -293,51 +300,55 @@ static struct amd_nb *amd_alloc_nb(int cpu, int nb_id)
 	return nb;
 }
 
-static void amd_pmu_cpu_online(int cpu)
+static int amd_pmu_cpu_prepare(int cpu)
+{
+	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
+
+	WARN_ON_ONCE(cpuc->amd_nb);
+
+	if (boot_cpu_data.x86_max_cores < 2)
+		return NOTIFY_OK;
+
+	cpuc->amd_nb = amd_alloc_nb(cpu, -1);
+	if (!cpuc->amd_nb)
+		return NOTIFY_BAD;
+
+	return NOTIFY_OK;
+}
+
+static void amd_pmu_cpu_starting(int cpu)
 {
-	struct cpu_hw_events *cpu1, *cpu2;
-	struct amd_nb *nb = NULL;
+	struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
+	struct amd_nb *nb;
 	int i, nb_id;
 
 	if (boot_cpu_data.x86_max_cores < 2)
 		return;
 
-	/*
-	 * function may be called too early in the
-	 * boot process, in which case nb_id is bogus
-	 */
 	nb_id = amd_get_nb_id(cpu);
-	if (nb_id == BAD_APICID)
-		return;
-
-	cpu1 = &per_cpu(cpu_hw_events, cpu);
-	cpu1->amd_nb = NULL;
+	WARN_ON_ONCE(nb_id == BAD_APICID);
 
 	raw_spin_lock(&amd_nb_lock);
 
 	for_each_online_cpu(i) {
-		cpu2 = &per_cpu(cpu_hw_events, i);
-		nb = cpu2->amd_nb;
-		if (!nb)
+		nb = per_cpu(cpu_hw_events, i).amd_nb;
+		if (WARN_ON_ONCE(!nb))
 			continue;
-		if (nb->nb_id == nb_id)
-			goto found;
-	}
 
-	nb = amd_alloc_nb(cpu, nb_id);
-	if (!nb) {
-		pr_err("perf_events: failed NB allocation for CPU%d\n", cpu);
-		raw_spin_unlock(&amd_nb_lock);
-		return;
+		if (nb->nb_id == nb_id) {
+			kfree(cpuc->amd_nb);
+			cpuc->amd_nb = nb;
+			break;
+		}
 	}
-found:
-	nb->refcnt++;
-	cpu1->amd_nb = nb;
+
+	cpuc->amd_nb->nb_id = nb_id;
+	cpuc->amd_nb->refcnt++;
 
 	raw_spin_unlock(&amd_nb_lock);
 }
 
-static void amd_pmu_cpu_offline(int cpu)
+static void amd_pmu_cpu_dead(int cpu)
 {
 	struct cpu_hw_events *cpuhw;
 
@@ -349,8 +360,10 @@ static void amd_pmu_cpu_offline(int cpu)
 	raw_spin_lock(&amd_nb_lock);
 
 	if (cpuhw->amd_nb) {
-		if (--cpuhw->amd_nb->refcnt == 0)
-			kfree(cpuhw->amd_nb);
+		struct amd_nb *nb = cpuhw->amd_nb;
+
+		if (nb->nb_id == -1 || --nb->refcnt == 0)
+			kfree(nb);
 
 		cpuhw->amd_nb = NULL;
 	}
@@ -379,8 +392,9 @@ static __initconst struct x86_pmu amd_pmu = {
 	.get_event_constraints	= amd_get_event_constraints,
 	.put_event_constraints	= amd_put_event_constraints,
 
-	.cpu_prepare		= amd_pmu_cpu_online,
-	.cpu_dead		= amd_pmu_cpu_offline,
+	.cpu_prepare		= amd_pmu_cpu_prepare,
+	.cpu_starting		= amd_pmu_cpu_starting,
+	.cpu_dead		= amd_pmu_cpu_dead,
 };
 
 static __init int amd_pmu_init(void)

[PATCH] perf_events: fix bug in AMD per-cpu initialization

[Stephane Eranian]

> Crap, you're right, either notify_cpu_starting() is done too early or
> smp_store_cpu_info() is done too late.
> 
> Since smp_store_cpu_info() relies on the result of calibrate_delay() we
> can't easily change that order, but since there really isn't any other
> CPU_STARTING user in tree (I appear to have created the first?!) we can
> easily move that notifier thing later.
> 
> (What's up with that IRQ-enable over calibrate_delay(), can't we simply
> enable the NMI watchdog later?)
> 
> So I guess something like the below should work:

The patch does work for me. I made two small modifications by
adding amd_has_nb() such that the correct checking is for on amd_nb.

	Signed-off-by: Stephane Eranian <eranian@google.com>

diff --git a/arch/x86/kernel/cpu/perf_event_amd.c b/arch/x86/kernel/cpu/perf_event_amd.c
index a61e54b..6204739 100644
--- a/arch/x86/kernel/cpu/perf_event_amd.c
+++ b/arch/x86/kernel/cpu/perf_event_amd.c
@@ -137,6 +137,13 @@ static inline int amd_is_nb_event(struct hw_perf_event *hwc)
 	return (hwc->config & 0xe0) == 0xe0;
 }
 
+static inline int amd_has_nb(struct cpu_hw_events *cpuc)
+{
+	struct amd_nb *nb = cpuc->amd_nb;
+
+	return nb && nb->nb_id != -1;
+}
+
 static void amd_put_event_constraints(struct cpu_hw_events *cpuc,
 				      struct perf_event *event)
 {
@@ -147,7 +154,7 @@ static void amd_put_event_constraints(struct cpu_hw_events *cpuc,
 	/*
 	 * only care about NB events
 	 */
-	if (!(nb && amd_is_nb_event(hwc)))
+	if (!(amd_has_nb(cpuc) && amd_is_nb_event(hwc)))
 		return;
 
 	/*
@@ -214,7 +221,7 @@ amd_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
 	/*
 	 * if not NB event or no NB, then no constraints
 	 */
-	if (!(nb && amd_is_nb_event(hwc)))
+	if (!(amd_has_nb(cpuc) && amd_is_nb_event(hwc)))
 		return &unconstrained;
 
 	/*