oprofile, x86: Move memory allocation for ppro out of per cpu

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

Signed-off-by Maarten Lankhorst <[email protected]>

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

arch/x86/oprofile/nmi_int.c +2 -0

index 96646b3..fef08b2 100644
@@ -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();
 }

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

index 94b7450..aefefcc 100644
@@ -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 */

arch/x86/oprofile/op_x86_model.h +1 -0

index 89017fa..e77fd91 100644
@@ -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);

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

Quoted 1 line · Maarten Lankhorst

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?

Quoted 4 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
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?

Quoted 3 lines

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.

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

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

---

arch/x86/oprofile/op_model_ppro.c +1 -23

index 94b7450..608874b 100644
@@ -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;

Quoted 4 lines · 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 <[email protected]>

Applied to oprofile/core. Thanks, Maarten.

Quoted 4 lines · Robert Richter

Quoted 3 lines · 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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Maarten Lankhorst <[email protected]>

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

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

And the whole thing is tasteless.

Nacked-by Andi Kleen <[email protected]>

Quoted 2 lines · Andi Kleen

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.

From 298557db42eb2d6efca81669dc369425b46c5be6 Mon Sep 17 00:00:00 2001

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