[PATCH v2] oprofile, x86: Move memory allocation for ppro out of per cpu

[PATCH v2] oprofile, x86: Move memory allocation for ppro out of per cpu

[Maarten Lankhorst]

ppro_setup_ctrs is called on all cpu's, while init is only called once.

Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>

---
Oprofile shutdown is still broken. Doing kfree in the shutdown call gave
me a warning of in_atomic, so I moved to exit. This also allowed me to
remove the null pointer checks, by zeroing reset_value on shutdown. But
even with shutdown being broken on -rt at least I can run oprofile now. :)

diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index 96646b3..fef08b2 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -790,5 +790,7 @@ int __init op_nmi_init(struct oprofile_operations *ops)
 
 void op_nmi_exit(void)
 {
+	if (model->exit)
+		model->exit();
 	exit_suspend_resume();
 }
diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
index 94b7450..aefefcc 100644
--- a/arch/x86/oprofile/op_model_ppro.c
+++ b/arch/x86/oprofile/op_model_ppro.c
@@ -30,6 +30,20 @@ static int counter_width = 32;
 
 static u64 *reset_value;
 
+static int ppro_init(struct oprofile_operations *ignore)
+{
+	reset_value = kzalloc(sizeof(reset_value[0]) * num_counters,
+				GFP_KERNEL);
+	if (!reset_value)
+		return -ENOMEM;
+	return 0;
+}
+
+static void ppro_exit(void)
+{
+	kfree(reset_value);
+}
+
 static void ppro_shutdown(struct op_msrs const * const msrs)
 {
 	int i;
@@ -39,10 +53,7 @@ static void ppro_shutdown(struct op_msrs const * const msrs)
 			continue;
 		release_perfctr_nmi(MSR_P6_PERFCTR0 + i);
 		release_evntsel_nmi(MSR_P6_EVNTSEL0 + i);
-	}
-	if (reset_value) {
-		kfree(reset_value);
-		reset_value = NULL;
+		reset_value[i] = 0;
 	}
 }
 
@@ -79,13 +90,6 @@ static void ppro_setup_ctrs(struct op_x86_model_spec const *model,
 	u64 val;
 	int i;
 
-	if (!reset_value) {
-		reset_value = kzalloc(sizeof(reset_value[0]) * num_counters,
-					GFP_ATOMIC);
-		if (!reset_value)
-			return;
-	}
-
 	if (cpu_has_arch_perfmon) {
 		union cpuid10_eax eax;
 		eax.full = cpuid_eax(0xa);
@@ -141,13 +145,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 	u64 val;
 	int i;
 
-	/*
-	 * This can happen if perf counters are in use when
-	 * we steal the die notifier NMI.
-	 */
-	if (unlikely(!reset_value))
-		goto out;
-
 	for (i = 0; i < num_counters; ++i) {
 		if (!reset_value[i])
 			continue;
@@ -158,7 +155,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 		wrmsrl(msrs->counters[i].addr, -reset_value[i]);
 	}
 
-out:
 	/* Only P6 based Pentium M need to re-unmask the apic vector but it
 	 * doesn't hurt other P6 variant */
 	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
@@ -179,8 +175,6 @@ static void ppro_start(struct op_msrs const * const msrs)
 	u64 val;
 	int i;
 
-	if (!reset_value)
-		return;
 	for (i = 0; i < num_counters; ++i) {
 		if (reset_value[i]) {
 			rdmsrl(msrs->controls[i].addr, val);
@@ -196,8 +190,6 @@ static void ppro_stop(struct op_msrs const * const msrs)
 	u64 val;
 	int i;
 
-	if (!reset_value)
-		return;
 	for (i = 0; i < num_counters; ++i) {
 		if (!reset_value[i])
 			continue;
@@ -211,6 +203,8 @@ struct op_x86_model_spec op_ppro_spec = {
 	.num_counters		= 2,
 	.num_controls		= 2,
 	.reserved		= MSR_PPRO_EVENTSEL_RESERVED,
+	.init			= &ppro_init,
+	.exit			= &ppro_exit,
 	.fill_in_addresses	= &ppro_fill_in_addresses,
 	.setup_ctrs		= &ppro_setup_ctrs,
 	.check_ctrs		= &ppro_check_ctrs,
@@ -251,12 +245,13 @@ static void arch_perfmon_setup_counters(void)
 static int arch_perfmon_init(struct oprofile_operations *ignore)
 {
 	arch_perfmon_setup_counters();
-	return 0;
+	return ppro_init(ignore);
 }
 
 struct op_x86_model_spec op_arch_perfmon_spec = {
 	.reserved		= MSR_PPRO_EVENTSEL_RESERVED,
 	.init			= &arch_perfmon_init,
+	.exit			= &ppro_exit,
 	/* num_counters/num_controls filled in at runtime */
 	.fill_in_addresses	= &ppro_fill_in_addresses,
 	/* user space does the cpuid check for available events */
diff --git a/arch/x86/oprofile/op_x86_model.h b/arch/x86/oprofile/op_x86_model.h
index 89017fa..e77fd91 100644
--- a/arch/x86/oprofile/op_x86_model.h
+++ b/arch/x86/oprofile/op_x86_model.h
@@ -40,6 +40,7 @@ struct op_x86_model_spec {
 	u64		reserved;
 	u16		event_mask;
 	int		(*init)(struct oprofile_operations *ops);
+	void		(*exit)(void);
 	int		(*fill_in_addresses)(struct op_msrs * const msrs);
 	void		(*setup_ctrs)(struct op_x86_model_spec const *model,
 				      struct op_msrs const * const msrs);

Re: [PATCH v2] oprofile, x86: Move memory allocation for ppro out of per cpu

[Robert Richter]

On 31.07.11 14:39:10, Maarten Lankhorst wrote:
> ppro_setup_ctrs is called on all cpu's, while init is only called once.

Can you please describe the root problem more precisely. Why can't you
run it on -rt? What is broken?

> 
> Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>
> 
> ---
> Oprofile shutdown is still broken. Doing kfree in the shutdown call gave

Do you mean it is broken and your patch fixes it, or is it still
broken even with your path applied?

> me a warning of in_atomic, so I moved to exit. This also allowed me to
> remove the null pointer checks, by zeroing reset_value on shutdown. But
> even with shutdown being broken on -rt at least I can run oprofile now. :)

If there is a problem I tend to fix it by using a hard coded array:

 static unsigned long reset_value[OP_MAX_COUNTER];

See arch/x86/oprofile/op_model_amd.c.

But still can't see the problem you want to fix.

Please cc Andi.

Thanks,

-Robert

> 
> diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
> index 96646b3..fef08b2 100644
> --- a/arch/x86/oprofile/nmi_int.c
> +++ b/arch/x86/oprofile/nmi_int.c
> @@ -790,5 +790,7 @@ int __init op_nmi_init(struct oprofile_operations *ops)
>  
>  void op_nmi_exit(void)
>  {
> +	if (model->exit)
> +		model->exit();
>  	exit_suspend_resume();
>  }
> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
> index 94b7450..aefefcc 100644
> --- a/arch/x86/oprofile/op_model_ppro.c
> +++ b/arch/x86/oprofile/op_model_ppro.c
> @@ -30,6 +30,20 @@ static int counter_width = 32;
>  
>  static u64 *reset_value;
>  
> +static int ppro_init(struct oprofile_operations *ignore)
> +{
> +	reset_value = kzalloc(sizeof(reset_value[0]) * num_counters,
> +				GFP_KERNEL);
> +	if (!reset_value)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static void ppro_exit(void)
> +{
> +	kfree(reset_value);
> +}
> +
>  static void ppro_shutdown(struct op_msrs const * const msrs)
>  {
>  	int i;
> @@ -39,10 +53,7 @@ static void ppro_shutdown(struct op_msrs const * const msrs)
>  			continue;
>  		release_perfctr_nmi(MSR_P6_PERFCTR0 + i);
>  		release_evntsel_nmi(MSR_P6_EVNTSEL0 + i);
> -	}
> -	if (reset_value) {
> -		kfree(reset_value);
> -		reset_value = NULL;
> +		reset_value[i] = 0;
>  	}
>  }
>  
> @@ -79,13 +90,6 @@ static void ppro_setup_ctrs(struct op_x86_model_spec const *model,
>  	u64 val;
>  	int i;
>  
> -	if (!reset_value) {
> -		reset_value = kzalloc(sizeof(reset_value[0]) * num_counters,
> -					GFP_ATOMIC);
> -		if (!reset_value)
> -			return;
> -	}
> -
>  	if (cpu_has_arch_perfmon) {
>  		union cpuid10_eax eax;
>  		eax.full = cpuid_eax(0xa);
> @@ -141,13 +145,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
>  	u64 val;
>  	int i;
>  
> -	/*
> -	 * This can happen if perf counters are in use when
> -	 * we steal the die notifier NMI.
> -	 */
> -	if (unlikely(!reset_value))
> -		goto out;
> -
>  	for (i = 0; i < num_counters; ++i) {
>  		if (!reset_value[i])
>  			continue;
> @@ -158,7 +155,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
>  		wrmsrl(msrs->counters[i].addr, -reset_value[i]);
>  	}
>  
> -out:
>  	/* Only P6 based Pentium M need to re-unmask the apic vector but it
>  	 * doesn't hurt other P6 variant */
>  	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
> @@ -179,8 +175,6 @@ static void ppro_start(struct op_msrs const * const msrs)
>  	u64 val;
>  	int i;
>  
> -	if (!reset_value)
> -		return;
>  	for (i = 0; i < num_counters; ++i) {
>  		if (reset_value[i]) {
>  			rdmsrl(msrs->controls[i].addr, val);
> @@ -196,8 +190,6 @@ static void ppro_stop(struct op_msrs const * const msrs)
>  	u64 val;
>  	int i;
>  
> -	if (!reset_value)
> -		return;
>  	for (i = 0; i < num_counters; ++i) {
>  		if (!reset_value[i])
>  			continue;
> @@ -211,6 +203,8 @@ struct op_x86_model_spec op_ppro_spec = {
>  	.num_counters		= 2,
>  	.num_controls		= 2,
>  	.reserved		= MSR_PPRO_EVENTSEL_RESERVED,
> +	.init			= &ppro_init,
> +	.exit			= &ppro_exit,
>  	.fill_in_addresses	= &ppro_fill_in_addresses,
>  	.setup_ctrs		= &ppro_setup_ctrs,
>  	.check_ctrs		= &ppro_check_ctrs,
> @@ -251,12 +245,13 @@ static void arch_perfmon_setup_counters(void)
>  static int arch_perfmon_init(struct oprofile_operations *ignore)
>  {
>  	arch_perfmon_setup_counters();
> -	return 0;
> +	return ppro_init(ignore);
>  }
>  
>  struct op_x86_model_spec op_arch_perfmon_spec = {
>  	.reserved		= MSR_PPRO_EVENTSEL_RESERVED,
>  	.init			= &arch_perfmon_init,
> +	.exit			= &ppro_exit,
>  	/* num_counters/num_controls filled in at runtime */
>  	.fill_in_addresses	= &ppro_fill_in_addresses,
>  	/* user space does the cpuid check for available events */
> diff --git a/arch/x86/oprofile/op_x86_model.h b/arch/x86/oprofile/op_x86_model.h
> index 89017fa..e77fd91 100644
> --- a/arch/x86/oprofile/op_x86_model.h
> +++ b/arch/x86/oprofile/op_x86_model.h
> @@ -40,6 +40,7 @@ struct op_x86_model_spec {
>  	u64		reserved;
>  	u16		event_mask;
>  	int		(*init)(struct oprofile_operations *ops);
> +	void		(*exit)(void);
>  	int		(*fill_in_addresses)(struct op_msrs * const msrs);
>  	void		(*setup_ctrs)(struct op_x86_model_spec const *model,
>  				      struct op_msrs const * const msrs);
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH v2] oprofile, x86: Move memory allocation for ppro out of per cpu

[Peter Zijlstra]

On Mon, 2011-08-01 at 09:07 +0200, Robert Richter wrote:
> Can you please describe the root problem more precisely. Why can't you
> run it on -rt? What is broken? 

Because the allocators don't work from atomic context anymore. Now -rt
doesn't have a lot of real atomic contexts left so its usually not much
of a problem, but CPU_STARTING and actual IPIs are of course still
atomic.

Re: [PATCH v2] oprofile, x86: Move memory allocation for ppro out of per cpu

[Maarten Lankhorst]

On 08/01/2011 09:07 AM, Robert Richter wrote:
> On 31.07.11 14:39:10, Maarten Lankhorst wrote:
>> ppro_setup_ctrs is called on all cpu's, while init is only called once.
> Can you please describe the root problem more precisely. Why can't you
> run it on -rt? What is broken?
on -rt it warns about the allocation.
>> Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>
>>
>> ---
>> Oprofile shutdown is still broken. Doing kfree in the shutdown call gave
> Do you mean it is broken and your patch fixes it, or is it still
> broken even with your path applied?
With the patch I no longer get the warning on setup_ctrs, but on rt
just starting and shutting down is enough to cause badness. The
original backtrace is below, but here's what happens with it patched.
opcontrol --start and then --shutdown:

[17318.720971] BUG: sleeping function called from invalid context at kernel/rtmutex.c:645
[17318.720972] in_atomic(): 1, irqs_disabled(): 0, pid: 18108, name: opjitconv
[17318.720974] Pid: 18108, comm: opjitconv Tainted: P        WC  3.0.0-rt6-patser+ #5
[17318.720975] Call Trace:
[17318.720979]  [<ffffffff8103fc0a>] __might_sleep+0xca/0xf0
[17318.720981]  [<ffffffff8160d424>] rt_spin_lock+0x24/0x40
[17318.720983]  [<ffffffff811453c3>] kfree+0xd3/0x370
[17318.720985]  [<ffffffffa0c20204>] free_msrs+0x44/0x130 [oprofile]
[17318.720987]  [<ffffffffa0c2037c>] nmi_shutdown+0x8c/0xc0 [oprofile]
[17318.720989]  [<ffffffffa0c1d2b6>] oprofile_shutdown+0x36/0x70 [oprofile]
[17318.720990]  [<ffffffffa0c1e8db>] event_buffer_release+0x1b/0x50 [oprofile]
[17318.720992]  [<ffffffff8115814e>] fput+0xfe/0x240
[17318.720993]  [<ffffffff811546ec>] filp_close+0x6c/0x90
[17318.720995]  [<ffffffff8105a810>] put_files_struct+0xa0/0x120
[17318.720996]  [<ffffffff8105a96c>] exit_files+0x5c/0x70
[17318.720997]  [<ffffffff8105aeaf>] do_exit+0x17f/0x900
[17318.720998]  [<ffffffff8105b9af>] do_group_exit+0x4f/0xd0
[17318.720999]  [<ffffffff8105ba47>] sys_exit_group+0x17/0x20
[17318.721001]  [<ffffffff8161456b>] system_call_fastpath+0x16/0x1b
[17318.721303] slab error in kmem_cache_destroy(): cache `dcookie_cache': Can't free all objects
[17318.721304] Pid: 18108, comm: opjitconv Tainted: P        WC  3.0.0-rt6-patser+ #5
[17318.721305] Call Trace:
[17318.721306]  [<ffffffff81145f4c>] kmem_cache_destroy+0xdc/0x120
[17318.721309]  [<ffffffff811cd4e5>] dcookie_unregister+0x175/0x180
[17318.721310]  [<ffffffffa0c1e8e7>] event_buffer_release+0x27/0x50 [oprofile]
[17318.721312]  [<ffffffff8115814e>] fput+0xfe/0x240
[17318.721313]  [<ffffffff811546ec>] filp_close+0x6c/0x90
[17318.721314]  [<ffffffff8105a810>] put_files_struct+0xa0/0x120
[17318.721315]  [<ffffffff8105a96c>] exit_files+0x5c/0x70
[17318.721316]  [<ffffffff8105aeaf>] do_exit+0x17f/0x900
[17318.721317]  [<ffffffff8105b9af>] do_group_exit+0x4f/0xd0
[17318.721318]  [<ffffffff8105ba47>] sys_exit_group+0x17/0x20
[17318.721319]  [<ffffffff8161456b>] system_call_fastpath+0x16/0x1b

After that opcontrol can no longer start.

>> me a warning of in_atomic, so I moved to exit. This also allowed me to
>> remove the null pointer checks, by zeroing reset_value on shutdown. But
>> even with shutdown being broken on -rt at least I can run oprofile now. :)
> If there is a problem I tend to fix it by using a hard coded array:
>
>  static unsigned long reset_value[OP_MAX_COUNTER];
Ah, that looks better, thanks. I wanted to use a static array,
but didn't know the max value I should use.
> See arch/x86/oprofile/op_model_amd.c.
>
> But still can't see the problem you want to fix.
It gives a warning on rt because it allocates memory in something
that runs on all cpus. I think the OP_MAX_COUNTER fix is fine, since
amd does similar.

~Maarten

Original bug:

[   46.460786] oprofile: using NMI interrupt.
[   47.492263] BUG: sleeping function called from invalid context at kernel/rtmutex.c:645
[   47.492265] in_atomic(): 1, irqs_disabled(): 1, pid: 0, name: kworker/0:1
[   47.492267] Pid: 0, comm: kworker/0:1 Tainted: G         C  3.0.0-rt3-patser+ #39
[   47.492269] Call Trace:
[   47.492270]  <IRQ>  [<ffffffff8103fc0a>] __might_sleep+0xca/0xf0
[   47.492279]  [<ffffffff8160d424>] rt_spin_lock+0x24/0x40
[   47.492282]  [<ffffffff811476c7>] __kmalloc+0xc7/0x370
[   47.492287]  [<ffffffffa0275c85>] ? ppro_setup_ctrs+0x215/0x260 [oprofile]
[   47.492291]  [<ffffffffa0273de0>] ? oprofile_cpu_notifier+0x60/0x60 [oprofile]
[   47.492295]  [<ffffffffa0275c85>] ppro_setup_ctrs+0x215/0x260 [oprofile]
[   47.492305]  [<ffffffffa0273de0>] ? oprofile_cpu_notifier+0x60/0x60 [oprofile]
[   47.492307]  [<ffffffffa0273de0>] ? oprofile_cpu_notifier+0x60/0x60 [oprofile]
[   47.492308]  [<ffffffffa0273ea4>] nmi_cpu_setup+0xc4/0x110 [oprofile]
[   47.492310]  [<ffffffff81094455>] generic_smp_call_function_interrupt+0x95/0x190
[   47.492313]  [<ffffffff8101df77>] smp_call_function_interrupt+0x27/0x40
[   47.492315]  [<ffffffff81615093>] call_function_interrupt+0x13/0x20
[   47.492316]  <EOI>  [<ffffffff8131d0c4>] ? plist_check_head+0x54/0xc0
[   47.492321]  [<ffffffff81371fe8>] ? intel_idle+0xc8/0x120
[   47.492322]  [<ffffffff81371fc7>] ? intel_idle+0xa7/0x120
[   47.492324]  [<ffffffff814a57b0>] cpuidle_idle_call+0xb0/0x230
[   47.492326]  [<ffffffff810011db>] cpu_idle+0x8b/0xe0
[   47.492328]  [<ffffffff815fc82f>] start_secondary+0x1d3/0x1d8

[PATCH v3] oprofile, x86: Convert memory allocation to static array

[Maarten Lankhorst]

On -rt, allocators don't work from atomic context any more,
and the maximum size of the array is known at compile time.

Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>

---

diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
index 94b7450..608874b 100644
--- a/arch/x86/oprofile/op_model_ppro.c
+++ b/arch/x86/oprofile/op_model_ppro.c
@@ -28,7 +28,7 @@ static int counter_width = 32;
 
 #define MSR_PPRO_EVENTSEL_RESERVED	((0xFFFFFFFFULL<<32)|(1ULL<<21))
 
-static u64 *reset_value;
+static u64 reset_value[OP_MAX_COUNTER];
 
 static void ppro_shutdown(struct op_msrs const * const msrs)
 {
@@ -40,10 +40,6 @@ static void ppro_shutdown(struct op_msrs const * const msrs)
 		release_perfctr_nmi(MSR_P6_PERFCTR0 + i);
 		release_evntsel_nmi(MSR_P6_EVNTSEL0 + i);
 	}
-	if (reset_value) {
-		kfree(reset_value);
-		reset_value = NULL;
-	}
 }
 
 static int ppro_fill_in_addresses(struct op_msrs * const msrs)
@@ -79,13 +75,6 @@ static void ppro_setup_ctrs(struct op_x86_model_spec const *model,
 	u64 val;
 	int i;
 
-	if (!reset_value) {
-		reset_value = kzalloc(sizeof(reset_value[0]) * num_counters,
-					GFP_ATOMIC);
-		if (!reset_value)
-			return;
-	}
-
 	if (cpu_has_arch_perfmon) {
 		union cpuid10_eax eax;
 		eax.full = cpuid_eax(0xa);
@@ -141,13 +130,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 	u64 val;
 	int i;
 
-	/*
-	 * This can happen if perf counters are in use when
-	 * we steal the die notifier NMI.
-	 */
-	if (unlikely(!reset_value))
-		goto out;
-
 	for (i = 0; i < num_counters; ++i) {
 		if (!reset_value[i])
 			continue;
@@ -179,8 +161,6 @@ static void ppro_start(struct op_msrs const * const msrs)
 	u64 val;
 	int i;
 
-	if (!reset_value)
-		return;
 	for (i = 0; i < num_counters; ++i) {
 		if (reset_value[i]) {
 			rdmsrl(msrs->controls[i].addr, val);
@@ -196,8 +176,6 @@ static void ppro_stop(struct op_msrs const * const msrs)
 	u64 val;
 	int i;
 
-	if (!reset_value)
-		return;
 	for (i = 0; i < num_counters; ++i) {
 		if (!reset_value[i])
 			continue;

Re: [PATCH v3] oprofile, x86: Convert memory allocation to static array

[Andi Kleen]

On Mon, Aug 01, 2011 at 05:08:59PM +0200, Maarten Lankhorst wrote:
> On -rt, allocators don't work from atomic context any more,
> and the maximum size of the array is known at compile time.

What!?! Can you please fix -rt instead? That sounds very broken.

-Andi

Re: [PATCH v3] oprofile, x86: Convert memory allocation to static array

[Robert Richter]

On 01.08.11 11:08:59, Maarten Lankhorst wrote:
> On -rt, allocators don't work from atomic context any more,
> and the maximum size of the array is known at compile time.
> 
> Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>

Applied to oprofile/core. Thanks, Maarten.

Ingo, Peter,

do you want this for 3.1 (perf/urgent) or 3.2?

-Robert

> 
> ---
> 
> diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
> index 94b7450..608874b 100644
> --- a/arch/x86/oprofile/op_model_ppro.c
> +++ b/arch/x86/oprofile/op_model_ppro.c
> @@ -28,7 +28,7 @@ static int counter_width = 32;
>  
>  #define MSR_PPRO_EVENTSEL_RESERVED	((0xFFFFFFFFULL<<32)|(1ULL<<21))
>  
> -static u64 *reset_value;
> +static u64 reset_value[OP_MAX_COUNTER];
>  
>  static void ppro_shutdown(struct op_msrs const * const msrs)
>  {
> @@ -40,10 +40,6 @@ static void ppro_shutdown(struct op_msrs const * const msrs)
>  		release_perfctr_nmi(MSR_P6_PERFCTR0 + i);
>  		release_evntsel_nmi(MSR_P6_EVNTSEL0 + i);
>  	}
> -	if (reset_value) {
> -		kfree(reset_value);
> -		reset_value = NULL;
> -	}
>  }
>  
>  static int ppro_fill_in_addresses(struct op_msrs * const msrs)
> @@ -79,13 +75,6 @@ static void ppro_setup_ctrs(struct op_x86_model_spec const *model,
>  	u64 val;
>  	int i;
>  
> -	if (!reset_value) {
> -		reset_value = kzalloc(sizeof(reset_value[0]) * num_counters,
> -					GFP_ATOMIC);
> -		if (!reset_value)
> -			return;
> -	}
> -
>  	if (cpu_has_arch_perfmon) {
>  		union cpuid10_eax eax;
>  		eax.full = cpuid_eax(0xa);
> @@ -141,13 +130,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
>  	u64 val;
>  	int i;
>  
> -	/*
> -	 * This can happen if perf counters are in use when
> -	 * we steal the die notifier NMI.
> -	 */
> -	if (unlikely(!reset_value))
> -		goto out;
> -
>  	for (i = 0; i < num_counters; ++i) {
>  		if (!reset_value[i])
>  			continue;
> @@ -179,8 +161,6 @@ static void ppro_start(struct op_msrs const * const msrs)
>  	u64 val;
>  	int i;
>  
> -	if (!reset_value)
> -		return;
>  	for (i = 0; i < num_counters; ++i) {
>  		if (reset_value[i]) {
>  			rdmsrl(msrs->controls[i].addr, val);
> @@ -196,8 +176,6 @@ static void ppro_stop(struct op_msrs const * const msrs)
>  	u64 val;
>  	int i;
>  
> -	if (!reset_value)
> -		return;
>  	for (i = 0; i < num_counters; ++i) {
>  		if (!reset_value[i])
>  			continue;
> 
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH v3] oprofile, x86: Convert memory allocation to static array

[Andi Kleen]

On Mon, Aug 01, 2011 at 11:31:42PM +0200, Robert Richter wrote:
> On 01.08.11 11:08:59, Maarten Lankhorst wrote:
> > On -rt, allocators don't work from atomic context any more,
> > and the maximum size of the array is known at compile time.
> > 
> > Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>
> 
> Applied to oprofile/core. Thanks, Maarten.

And what happens when the CPU reports more than 32 counters?
You have a silent buffer overflow then.

Besides I bet there are other cases like this all over the tree.

And the whole thing is tasteless. 

Nacked-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCH v3] oprofile, x86: Convert memory allocation to static array

[Robert Richter]

On 01.08.11 17:41:30, Andi Kleen wrote:
> On Mon, Aug 01, 2011 at 11:31:42PM +0200, Robert Richter wrote:
> > On 01.08.11 11:08:59, Maarten Lankhorst wrote:
> > > On -rt, allocators don't work from atomic context any more,
> > > and the maximum size of the array is known at compile time.
> > > 
> > > Signed-off-by: Maarten Lankhorst <m.b.lankhorst@gmail.com>
> > 
> > Applied to oprofile/core. Thanks, Maarten.
> 
> And what happens when the CPU reports more than 32 counters?
> You have a silent buffer overflow then.

>From the layout of IA32_PERF_GLOBAL_CTRL MSR it seems to be limited to
32. Anyway, if there might be cpus out soon with more than 32 counters
we can either extend the array to 256 or limit the counters used to 32.

> Besides I bet there are other cases like this all over the tree.
> 
> And the whole thing is tasteless. 
> 
> Nacked-by: Andi Kleen <ak@linux.intel.com>

This dynamic allocation is causing trouble from the beginning. Using a
static array removes a lot of NULL pointer checks:

 arch/x86/oprofile/op_model_ppro.c |   24 +-----------------------

The previous implementation silently dropped counters during setup on
failure. Also we have had many bugs caused by this dynamic allocation
(I counted 3 fixes). This all isn't it worth and you did not yet
explain the concerns you have (besides the potentially buffer overflow
which is fixable and probably of academic nature).

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH] oprofile, x86: Fix overflow and warning (commit 1d12d35)

[Robert Richter]

On 02.08.11 00:16:25, Robert Richter wrote:
> On 01.08.11 17:41:30, Andi Kleen wrote:
> > And what happens when the CPU reports more than 32 counters?
> > You have a silent buffer overflow then.
> 
> From the layout of IA32_PERF_GLOBAL_CTRL MSR it seems to be limited to
> 32. Anyway, if there might be cpus out soon with more than 32 counters
> we can either extend the array to 256 or limit the counters used to 32.

See my patch below.

-Robert


>From 298557db42eb2d6efca81669dc369425b46c5be6 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@amd.com>
Date: Tue, 16 Aug 2011 23:39:53 +0200
Subject: [PATCH] oprofile, x86: Fix overflow and warning (commit 1d12d35)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Following fixes for:

 1d12d35 oprofile, x86: Convert memory allocation to static array

Fix potential buffer overflow.

Fix the following warning:

 arch/x86/oprofile/op_model_ppro.c: In function ‘ppro_check_ctrs’:
 arch/x86/oprofile/op_model_ppro.c:143: warning: label ‘out’ defined but not used

Cc: Maarten Lankhorst <m.b.lankhorst@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/oprofile/op_model_ppro.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/oprofile/op_model_ppro.c b/arch/x86/oprofile/op_model_ppro.c
index 608874b..d90528e 100644
--- a/arch/x86/oprofile/op_model_ppro.c
+++ b/arch/x86/oprofile/op_model_ppro.c
@@ -140,7 +140,6 @@ static int ppro_check_ctrs(struct pt_regs * const regs,
 		wrmsrl(msrs->counters[i].addr, -reset_value[i]);
 	}
 
-out:
 	/* Only P6 based Pentium M need to re-unmask the apic vector but it
 	 * doesn't hurt other P6 variant */
 	apic_write(APIC_LVTPC, apic_read(APIC_LVTPC) & ~APIC_LVT_MASKED);
@@ -220,7 +219,7 @@ static void arch_perfmon_setup_counters(void)
 		eax.split.bit_width = 40;
 	}
 
-	num_counters = eax.split.num_counters;
+	num_counters = min((int)eax.split.num_counters, OP_MAX_COUNTER);
 
 	op_arch_perfmon_spec.num_counters = num_counters;
 	op_arch_perfmon_spec.num_controls = num_counters;
-- 
1.7.5.3



-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH v3] oprofile, x86: Convert memory allocation to static array

[Peter Zijlstra]

On Mon, 2011-08-01 at 23:31 +0200, Robert Richter wrote:
> do you want this for 3.1 (perf/urgent) or 3.2?

No particular hurry on my side..