Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1336442 > unrolled thread

[patch 00/11] x86/perf/intel_uncore: Cleanup and enhancements

Back to article view | Back to linux.kernel

Contents

  [patch 00/11] x86/perf/intel_uncore: Cleanup and enhancements Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 14:50 +0100
    [patch 05/11] x86/perf/intel_uncore: Make code readable Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 01/11] x86/perf/intel_uncore: Remove pointless mask check Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 02/11] x86/perf/intel_uncore: Simplify error rollback Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 09/11] x86/perf/intel_uncore: Make PCI and MSR uncore  independent Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 06/11] x86/topology: Provide helper to retrieve number of cpu  packages Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 03/11] x86/perf/intel_uncore: Fix error handling Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 11/11] x86/perf/intel_uncore: Make it modular Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 10/11] cpumask: Export cpumask_any_but Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
      RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit "Liang, Kan" <kan.liang@intel.com> - 2016-02-17 16:50 +0100
        RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 19:20 +0100
          RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit "Liang, Kan" <kan.liang@intel.com> - 2016-02-17 23:00 +0100
            RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 23:10 +0100

#1336442 — [patch 00/11] x86/perf/intel_uncore: Cleanup and enhancements

While working on the hotplug rewrite I stumbled over the uncore drivers. The
intel_uncore driver particular is a complete trainwreck:

 - Lacks any form of proper error handling. Most errors are simply ignored.

 - Leaks memory and hardware state in case of failures

 - Tries to mimick a per cpu machinery for a facility which is strictly per
   package. That is implemented with convoluted alloc/free dancing during cpu
   hotplug with magic loops over the online cpus

The series cleans up the mess

 - Implement proper error handling

 - Switch to a per package storage model

 - Make MSR and PCI independent as far as it goes

 - Allow it to build as a module

Thanks,

	tglx
---
 arch/x86/Kconfig                                    |   14 
 arch/x86/include/asm/topology.h                     |    3 
 arch/x86/kernel/cpu/Makefile                        |    3 
 arch/x86/kernel/cpu/perf_event_intel_uncore.c       |  587 +++++++++-----------
 arch/x86/kernel/cpu/perf_event_intel_uncore.h       |   24 
 arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c |    6 
 arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c   |   13 
 arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c |   57 +
 lib/cpumask.c                                       |    1 
 9 files changed, 400 insertions(+), 308 deletions(-)

[toc] | [next] | [standalone]

#1336444 — [patch 05/11] x86/perf/intel_uncore: Make code readable

Cleanup the code a bit before reworking it completely.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |   71 +++++++++++++-------------
 1 file changed, 36 insertions(+), 35 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -791,10 +791,8 @@ static void __init uncore_type_exit(stru
 
 static void __init uncore_types_exit(struct intel_uncore_type **types)
 {
-	int i;
-
-	for (i = 0; types[i]; i++)
-		uncore_type_exit(types[i]);
+	while (*types)
+		uncore_type_exit(*types++);
 }
 
 static int __init uncore_type_init(struct intel_uncore_type *type)
@@ -898,9 +896,11 @@ static int uncore_pci_probe(struct pci_d
 	 * some device types. Hence PCI device idx would be 0 for all devices.
 	 * So increment pmu pointer to point to an unused array element.
 	 */
-	if (boot_cpu_data.x86_model == 87)
+	if (boot_cpu_data.x86_model == 87) {
 		while (pmu->func_id >= 0)
 			pmu++;
+	}
+
 	if (pmu->func_id < 0)
 		pmu->func_id = pdev->devfn;
 	else
@@ -1158,44 +1158,45 @@ static int uncore_cpu_prepare(int cpu, i
 	return 0;
 }
 
-static void
-uncore_change_context(struct intel_uncore_type **uncores, int old_cpu, int new_cpu)
+static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_cpu,
+				   int new_cpu)
 {
-	struct intel_uncore_type *type;
-	struct intel_uncore_pmu *pmu;
+	struct intel_uncore_pmu *pmu = type->pmus;
 	struct intel_uncore_box *box;
-	int i, j;
+	int i;
 
-	for (i = 0; uncores[i]; i++) {
-		type = uncores[i];
-		for (j = 0; j < type->num_boxes; j++) {
-			pmu = &type->pmus[j];
-			if (old_cpu < 0)
-				box = uncore_pmu_to_box(pmu, new_cpu);
-			else
-				box = uncore_pmu_to_box(pmu, old_cpu);
-			if (!box)
-				continue;
+	for (i = 0; i < type->num_boxes; i++, pmu++) {
+		if (old_cpu < 0)
+			box = uncore_pmu_to_box(pmu, new_cpu);
+		else
+			box = uncore_pmu_to_box(pmu, old_cpu);
+		if (!box)
+			continue;
+
+		if (old_cpu < 0) {
+			WARN_ON_ONCE(box->cpu != -1);
+			box->cpu = new_cpu;
+			continue;
+		}
 
-			if (old_cpu < 0) {
-				WARN_ON_ONCE(box->cpu != -1);
-				box->cpu = new_cpu;
-				continue;
-			}
+		WARN_ON_ONCE(box->cpu != old_cpu);
+		box->cpu = -1;
+		if (new_cpu < 0)
+			continue;
 
-			WARN_ON_ONCE(box->cpu != old_cpu);
-			if (new_cpu >= 0) {
-				uncore_pmu_cancel_hrtimer(box);
-				perf_pmu_migrate_context(&pmu->pmu,
-						old_cpu, new_cpu);
-				box->cpu = new_cpu;
-			} else {
-				box->cpu = -1;
-			}
-		}
+		uncore_pmu_cancel_hrtimer(box);
+		perf_pmu_migrate_context(&pmu->pmu, old_cpu, new_cpu);
+		box->cpu = new_cpu;
 	}
 }
 
+static void uncore_change_context(struct intel_uncore_type **uncores,
+				  int old_cpu, int new_cpu)
+{
+	while (*uncores)
+		uncore_change_type_ctx(*uncores++, old_cpu, new_cpu);
+}
+
 static void uncore_event_exit_cpu(int cpu)
 {
 	int i, phys_id, target;

[toc] | [prev] | [next] | [standalone]

#1336445 — [patch 01/11] x86/perf/intel_uncore: Remove pointless mask check

uncore_cpumask_init() is only ever called from intel_uncore_init() where the
mask is guaranteed to be empty.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |    6 ------
 1 file changed, 6 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -1342,12 +1342,6 @@ static void __init uncore_cpumask_init(v
 {
 	int cpu;
 
-	/*
-	 * ony invoke once from msr or pci init code
-	 */
-	if (!cpumask_empty(&uncore_cpu_mask))
-		return;
-
 	cpu_notifier_register_begin();
 
 	for_each_online_cpu(cpu) {

[toc] | [prev] | [next] | [standalone]

#1336446 — [patch 02/11] x86/perf/intel_uncore: Simplify error rollback

No point in doing partial rollbacks. Robustify uncore_exit_type() so it does
not dereference type->pmus unconditionally and remove all the partial rollback
hackery.

Preparatory patch for proper error handling.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |   45 +++++++++++++-------------
 1 file changed, 24 insertions(+), 21 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -767,10 +767,12 @@ static void __init uncore_type_exit(stru
 {
 	int i;
 
-	for (i = 0; i < type->num_boxes; i++)
-		free_percpu(type->pmus[i].box);
-	kfree(type->pmus);
-	type->pmus = NULL;
+	if (type->pmus) {
+		for (i = 0; i < type->num_boxes; i++)
+			free_percpu(type->pmus[i].box);
+		kfree(type->pmus);
+		type->pmus = NULL;
+	}
 	kfree(type->events_group);
 	type->events_group = NULL;
 }
@@ -778,6 +780,7 @@ static void __init uncore_type_exit(stru
 static void __init uncore_types_exit(struct intel_uncore_type **types)
 {
 	int i;
+
 	for (i = 0; types[i]; i++)
 		uncore_type_exit(types[i]);
 }
@@ -806,7 +809,7 @@ static int __init uncore_type_init(struc
 		INIT_LIST_HEAD(&pmus[i].box_list);
 		pmus[i].box = alloc_percpu(struct intel_uncore_box *);
 		if (!pmus[i].box)
-			goto fail;
+			return -ENOMEM;
 	}
 
 	if (type->event_descs) {
@@ -817,7 +820,7 @@ static int __init uncore_type_init(struc
 		attr_group = kzalloc(sizeof(struct attribute *) * (i + 1) +
 					sizeof(*attr_group), GFP_KERNEL);
 		if (!attr_group)
-			goto fail;
+			return -ENOMEM;
 
 		attrs = (struct attribute **)(attr_group + 1);
 		attr_group->name = "events";
@@ -831,9 +834,6 @@ static int __init uncore_type_init(struc
 
 	type->pmu_group = &uncore_pmu_attr_group;
 	return 0;
-fail:
-	uncore_type_exit(type);
-	return -ENOMEM;
 }
 
 static int __init uncore_types_init(struct intel_uncore_type **types)
@@ -843,13 +843,9 @@ static int __init uncore_types_init(stru
 	for (i = 0; types[i]; i++) {
 		ret = uncore_type_init(types[i]);
 		if (ret)
-			goto fail;
+			return ret;
 	}
 	return 0;
-fail:
-	while (--i >= 0)
-		uncore_type_exit(types[i]);
-	return ret;
 }
 
 /*
@@ -1007,17 +1003,21 @@ static int __init uncore_pci_init(void)
 
 	ret = uncore_types_init(uncore_pci_uncores);
 	if (ret)
-		return ret;
+		goto err;
 
 	uncore_pci_driver->probe = uncore_pci_probe;
 	uncore_pci_driver->remove = uncore_pci_remove;
 
 	ret = pci_register_driver(uncore_pci_driver);
-	if (ret == 0)
-		pcidrv_registered = true;
-	else
-		uncore_types_exit(uncore_pci_uncores);
+	if (ret)
+		goto err;
+
+	pcidrv_registered = true;
+	return 0;
 
+err:
+	uncore_types_exit(uncore_pci_uncores);
+	uncore_pci_uncores = empty_uncore;
 	return ret;
 }
 
@@ -1316,9 +1316,12 @@ static int __init uncore_cpu_init(void)
 
 	ret = uncore_types_init(uncore_msr_uncores);
 	if (ret)
-		return ret;
-
+		goto err;
 	return 0;
+err:
+	uncore_types_exit(uncore_msr_uncores);
+	uncore_msr_uncores = empty_uncore;
+	return ret;
 }
 
 static int __init uncore_pmus_register(void)

[toc] | [prev] | [next] | [standalone]

#1336449 — [patch 09/11] x86/perf/intel_uncore: Make PCI and MSR uncore independent

Andi wanted to do this before, but the patch fell down the cracks. Implement
it with the proper error handling.

Requested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |   34 +++++++++++++-------------
 1 file changed, 18 insertions(+), 16 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -1014,7 +1014,7 @@ static int __init uncore_pci_init(void)
 		ret = skl_uncore_pci_init();
 		break;
 	default:
-		return 0;
+		return -ENODEV;
 	}
 
 	if (ret)
@@ -1302,7 +1302,7 @@ static int __init uncore_cpu_init(void)
 		knl_uncore_cpu_init();
 		break;
 	default:
-		return 0;
+		return -ENODEV;
 	}
 
 	ret = uncore_types_init(uncore_msr_uncores, true);
@@ -1327,7 +1327,7 @@ static void __init uncore_cpu_setup(void
 /* Lazy to avoid allocation of a few bytes for the normal case */
 static __initdata DECLARE_BITMAP(packages, NR_CPUS);
 
-static int __init uncore_cpumask_init(void)
+static int __init uncore_cpumask_init(bool msr)
 {
 	unsigned int cpu;
 
@@ -1338,12 +1338,15 @@ static int __init uncore_cpumask_init(vo
 		if (test_and_set_bit(pkg, packages))
 			continue;
 		/*
-		 * The first online cpu of each package takes the refcounts
-		 * for all other online cpus in that package.
+		 * The first online cpu of each package allocates and takes
+		 * the refcounts for all other online cpus in that package.
+		 * If msrs are not enabled no allocation is required.
 		 */
-		ret = uncore_cpu_prepare(cpu);
-		if (ret)
-			return ret;
+		if (msr) {
+			ret = uncore_cpu_prepare(cpu);
+			if (ret)
+				return ret;
+		}
 		uncore_event_init_cpu(cpu);
 		smp_call_function_single(cpu, uncore_cpu_setup, NULL, 1);
 	}
@@ -1353,7 +1356,7 @@ static int __init uncore_cpumask_init(vo
 
 static int __init intel_uncore_init(void)
 {
-	int ret;
+	int pret, cret, ret;
 
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
 		return -ENODEV;
@@ -1361,15 +1364,14 @@ static int __init intel_uncore_init(void
 	if (cpu_has_hypervisor)
 		return -ENODEV;
 
-	ret = uncore_pci_init();
-	if (ret)
-		return ret;
-	ret = uncore_cpu_init();
-	if (ret)
-		goto err;
+	pret = uncore_pci_init();
+	cret = uncore_cpu_init();
+
+	if (cret && pret)
+		return -ENODEV;
 
 	cpu_notifier_register_begin();
-	ret = uncore_cpumask_init();
+	ret = uncore_cpumask_init(!cret);
 	if (ret)
 		goto err;
 	cpu_notifier_register_done();

[toc] | [prev] | [next] | [standalone]

#1336450 — [patch 06/11] x86/topology: Provide helper to retrieve number of cpu packages

For services which are per package, e.g. intel_uncore, we need to know the
number of possible packages in the system. Provide a helper. Unfortunately a
macro to avoid inclue hell.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/topology.h |    3 +++
 1 file changed, 3 insertions(+)

--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -127,6 +127,9 @@ extern const struct cpumask *cpu_coregro
 #define topology_sibling_cpumask(cpu)		(per_cpu(cpu_sibling_map, cpu))
 #endif
 
+#define topology_max_packages()			\
+	DIV_ROUND_UP(num_possible_cpus(), boot_cpu_data.x86_max_cores * smp_num_siblings)
+
 static inline void arch_fix_phys_package_id(int num, u32 slot)
 {
 }

[toc] | [prev] | [next] | [standalone]

#1336453 — [patch 03/11] x86/perf/intel_uncore: Fix error handling

This driver lacks any form of proper error handling. If initialization fails
or hotplug prepare fails, it lets the facility with half initialized stuff
around.

Fix the state and memory leaks in a first step. As a second step we need to
undo the hardware state which is set via uncore_box_init() on some of the
uncore implementations.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |  110 +++++++++++++++++---------
 arch/x86/kernel/cpu/perf_event_intel_uncore.h |   15 +--
 2 files changed, 82 insertions(+), 43 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -760,16 +760,28 @@ static int uncore_pmu_register(struct in
 	}
 
 	ret = perf_pmu_register(&pmu->pmu, pmu->name, -1);
+	if (!ret)
+		pmu->registered = true;
 	return ret;
 }
 
+static void uncore_pmu_unregister(struct intel_uncore_pmu *pmu)
+{
+	if (!pmu->registered)
+		return;
+	perf_pmu_unregister(&pmu->pmu);
+	pmu->registered = false;
+}
+
 static void __init uncore_type_exit(struct intel_uncore_type *type)
 {
 	int i;
 
 	if (type->pmus) {
-		for (i = 0; i < type->num_boxes; i++)
+		for (i = 0; i < type->num_boxes; i++) {
+			uncore_pmu_unregister(&type->pmus[i]);
 			free_percpu(type->pmus[i].box);
+		}
 		kfree(type->pmus);
 		type->pmus = NULL;
 	}
@@ -856,8 +868,8 @@ static int uncore_pci_probe(struct pci_d
 	struct intel_uncore_pmu *pmu;
 	struct intel_uncore_box *box;
 	struct intel_uncore_type *type;
-	int phys_id;
 	bool first_box = false;
+	int phys_id, ret;
 
 	phys_id = uncore_pcibus_to_physid(pdev->bus);
 	if (phys_id < 0)
@@ -906,9 +918,18 @@ static int uncore_pci_probe(struct pci_d
 	list_add_tail(&box->list, &pmu->box_list);
 	raw_spin_unlock(&uncore_box_lock);
 
-	if (first_box)
-		uncore_pmu_register(pmu);
-	return 0;
+	if (!first_box)
+		return 0;
+
+	ret = uncore_pmu_register(pmu);
+	if (ret) {
+		pci_set_drvdata(pdev, NULL);
+		raw_spin_lock(&uncore_box_lock);
+		list_del(&box->list);
+		raw_spin_unlock(&uncore_box_lock);
+		kfree(box);
+	}
+	return ret;
 }
 
 static void uncore_pci_remove(struct pci_dev *pdev)
@@ -954,7 +975,7 @@ static void uncore_pci_remove(struct pci
 	kfree(box);
 
 	if (last_box)
-		perf_pmu_unregister(&pmu->pmu);
+		uncore_pmu_unregister(pmu);
 }
 
 static int __init uncore_pci_init(void)
@@ -1223,8 +1244,7 @@ static int uncore_cpu_notifier(struct no
 	/* allocate/free data structure for uncore box */
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_UP_PREPARE:
-		uncore_cpu_prepare(cpu, -1);
-		break;
+		return notifier_from_errno(uncore_cpu_prepare(cpu, -1));
 	case CPU_STARTING:
 		uncore_cpu_starting(cpu);
 		break;
@@ -1265,9 +1285,29 @@ static struct notifier_block uncore_cpu_
 	.priority	= CPU_PRI_PERF + 1,
 };
 
-static void __init uncore_cpu_setup(void *dummy)
+static int __init type_pmu_register(struct intel_uncore_type *type)
 {
-	uncore_cpu_starting(smp_processor_id());
+	int i, ret;
+
+	for (i = 0; i < type->num_boxes; i++) {
+		ret = uncore_pmu_register(&type->pmus[i]);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int __init uncore_msr_pmus_register(void)
+{
+	struct intel_uncore_type **types = uncore_msr_uncores;
+	int ret;
+
+	while (*types) {
+		ret = type_pmu_register(*types++);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 static int __init uncore_cpu_init(void)
@@ -1317,6 +1357,10 @@ static int __init uncore_cpu_init(void)
 	ret = uncore_types_init(uncore_msr_uncores);
 	if (ret)
 		goto err;
+
+	ret = uncore_msr_pmus_register();
+	if (ret)
+		goto err;
 	return 0;
 err:
 	uncore_types_exit(uncore_msr_uncores);
@@ -1324,26 +1368,14 @@ static int __init uncore_cpu_init(void)
 	return ret;
 }
 
-static int __init uncore_pmus_register(void)
+static void __init uncore_cpu_setup(void *dummy)
 {
-	struct intel_uncore_pmu *pmu;
-	struct intel_uncore_type *type;
-	int i, j;
-
-	for (i = 0; uncore_msr_uncores[i]; i++) {
-		type = uncore_msr_uncores[i];
-		for (j = 0; j < type->num_boxes; j++) {
-			pmu = &type->pmus[j];
-			uncore_pmu_register(pmu);
-		}
-	}
-
-	return 0;
+	uncore_cpu_starting(smp_processor_id());
 }
 
-static void __init uncore_cpumask_init(void)
+static int __init uncore_cpumask_init(void)
 {
-	int cpu;
+	int cpu, ret = 0;
 
 	cpu_notifier_register_begin();
 
@@ -1359,17 +1391,20 @@ static void __init uncore_cpumask_init(v
 		if (phys_id < 0)
 			continue;
 
-		uncore_cpu_prepare(cpu, phys_id);
+		ret = uncore_cpu_prepare(cpu, phys_id);
+		if (ret)
+			goto out;
 		uncore_event_init_cpu(cpu);
 	}
 	on_each_cpu(uncore_cpu_setup, NULL, 1);
 
 	__register_cpu_notifier(&uncore_cpu_nb);
 
+out:
 	cpu_notifier_register_done();
+	return ret;
 }
 
-
 static int __init intel_uncore_init(void)
 {
 	int ret;
@@ -1382,17 +1417,20 @@ static int __init intel_uncore_init(void
 
 	ret = uncore_pci_init();
 	if (ret)
-		goto fail;
+		return ret;
 	ret = uncore_cpu_init();
-	if (ret) {
-		uncore_pci_exit();
-		goto fail;
-	}
-	uncore_cpumask_init();
+	if (ret)
+		goto errpci;
+	ret = uncore_cpumask_init();
+	if (ret)
+		goto errcpu;
 
-	uncore_pmus_register();
 	return 0;
-fail:
+
+errcpu:
+	uncore_types_exit(uncore_msr_uncores);
+errpci:
+	uncore_pci_exit();
 	return ret;
 }
 device_initcall(intel_uncore_init);
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.h
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
@@ -73,13 +73,14 @@ struct intel_uncore_ops {
 };
 
 struct intel_uncore_pmu {
-	struct pmu pmu;
-	char name[UNCORE_PMU_NAME_LEN];
-	int pmu_idx;
-	int func_id;
-	struct intel_uncore_type *type;
-	struct intel_uncore_box ** __percpu box;
-	struct list_head box_list;
+	struct pmu			pmu;
+	char				name[UNCORE_PMU_NAME_LEN];
+	int				pmu_idx;
+	int				func_id;
+	bool				registered;
+	struct intel_uncore_type	*type;
+	struct intel_uncore_box		** __percpu box;
+	struct list_head		box_list;
 };
 
 struct intel_uncore_extra_reg {

[toc] | [prev] | [next] | [standalone]

#1336454 — [patch 11/11] x86/perf/intel_uncore: Make it modular

Now that we have a proper cleanup all over the place, it's simple to make this
a modular driver.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/Kconfig                              |   14 ++++++++++----
 arch/x86/kernel/cpu/Makefile                  |    3 ++-
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |   24 ++++++++++++++++++------
 3 files changed, 30 insertions(+), 11 deletions(-)

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -160,10 +160,6 @@ config INSTRUCTION_DECODER
 	def_bool y
 	depends on KPROBES || PERF_EVENTS || UPROBES
 
-config PERF_EVENTS_INTEL_UNCORE
-	def_bool y
-	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
-
 config OUTPUT_FORMAT
 	string
 	default "elf32-i386" if X86_32
@@ -1039,6 +1035,16 @@ config X86_THERMAL_VECTOR
 	def_bool y
 	depends on X86_MCE_INTEL
 
+config PERF_EVENTS_INTEL_UNCORE
+	tristate "Intel uncore performance events"
+	depends on PERF_EVENTS && CPU_SUP_INTEL && PCI
+	default y
+	---help---
+	  Include support for Intel uncore performance events. These are
+	  available on NehalemEX and more modern processors.
+
+	  If unsure say y.
+
 config X86_LEGACY_VM86
 	bool "Legacy VM86 support"
 	default n
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -43,7 +43,8 @@ obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_eve
 obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_pt.o perf_event_intel_bts.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= perf_event_intel_cstate.o
 
-obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)	+= perf_event_intel_uncore.o \
+obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)	+= perf_event_intel_uncores.o
+perf_event_intel_uncores-objs		:= perf_event_intel_uncore.o \
 					   perf_event_intel_uncore_snb.o \
 					   perf_event_intel_uncore_snbep.o \
 					   perf_event_intel_uncore_nhmex.o
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -20,6 +20,8 @@ static struct event_constraint uncore_co
 struct event_constraint uncore_constraint_empty =
 	EVENT_CONSTRAINT(0, 0, 0);
 
+MODULE_LICENSE("GPL");
+
 int uncore_pcibus_to_physid(struct pci_bus *bus)
 {
 	struct pci2phy_map *map;
@@ -751,7 +753,7 @@ static void uncore_pmu_unregister(struct
 	pmu->registered = false;
 }
 
-static void __init __uncore_exit_boxes(struct intel_uncore_type *type, int cpu)
+static void __uncore_exit_boxes(struct intel_uncore_type *type, int cpu)
 {
 	struct intel_uncore_pmu *pmu = type->pmus;
 	struct intel_uncore_box *box;
@@ -767,7 +769,7 @@ static void __init __uncore_exit_boxes(s
 	}
 }
 
-static void __init uncore_exit_boxes(void *dummy)
+static void uncore_exit_boxes(void *dummy)
 {
 	struct intel_uncore_type **types = uncore_msr_uncores;
 
@@ -784,7 +786,7 @@ static void uncore_free_boxes(struct int
 	kfree(pmu->boxes);
 }
 
-static void __init uncore_type_exit(struct intel_uncore_type *type)
+static void uncore_type_exit(struct intel_uncore_type *type)
 {
 	struct intel_uncore_pmu *pmu = type->pmus;
 	int i;
@@ -801,7 +803,7 @@ static void __init uncore_type_exit(stru
 	type->events_group = NULL;
 }
 
-static void __init uncore_types_exit(struct intel_uncore_type **types)
+static void uncore_types_exit(struct intel_uncore_type **types)
 {
 	while (*types)
 		uncore_type_exit(*types++);
@@ -1040,7 +1042,7 @@ static int __init uncore_pci_init(void)
 	return ret;
 }
 
-static void __init uncore_pci_exit(void)
+static void uncore_pci_exit(void)
 {
 	if (pcidrv_registered) {
 		pcidrv_registered = false;
@@ -1385,4 +1387,14 @@ static int __init intel_uncore_init(void
 	cpu_notifier_register_done();
 	return ret;
 }
-device_initcall(intel_uncore_init);
+module_init(intel_uncore_init);
+
+static void __exit intel_uncore_exit(void)
+{
+	cpu_notifier_register_done();
+	__unregister_cpu_notifier(&uncore_cpu_nb);
+	uncore_types_exit(uncore_msr_uncores);
+	uncore_pci_exit();
+	cpu_notifier_register_done();
+}
+module_exit(intel_uncore_exit);

[toc] | [prev] | [next] | [standalone]

#1336456 — [patch 10/11] cpumask: Export cpumask_any_but

Almost every cpumask function is exported, just not the one I need to make the
intel uncore driver modular.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 lib/cpumask.c |    1 +
 1 file changed, 1 insertion(+)

--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -41,6 +41,7 @@ int cpumask_any_but(const struct cpumask
 			break;
 	return i;
 }
+EXPORT_SYMBOL(cpumask_any_but);
 
 /* These are not inline because of header tangles. */
 #ifdef CONFIG_CPUMASK_OFFSTACK

[toc] | [prev] | [next] | [standalone]

#1336460 — [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

When tearing down the boxes nothing undoes the hardware state which was setup
by box->init_box(). Add a box->exit_box() callback and implement it for the
uncores which have an init_box() callback.

This misses the cleanup in the error exit pathes, but I cannot be bothered to
implement it before cleaning up the rest of the driver, which makes that task
way simpler.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c       |    6 +-
 arch/x86/kernel/cpu/perf_event_intel_uncore.h       |    9 +++
 arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c |    6 ++
 arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c   |   13 ++++
 arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c |   57 +++++++++++++++++++-
 5 files changed, 88 insertions(+), 3 deletions(-)

--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -927,6 +927,7 @@ static int uncore_pci_probe(struct pci_d
 		raw_spin_lock(&uncore_box_lock);
 		list_del(&box->list);
 		raw_spin_unlock(&uncore_box_lock);
+		uncore_box_exit(box);
 		kfree(box);
 	}
 	return ret;
@@ -972,6 +973,7 @@ static void uncore_pci_remove(struct pci
 	}
 
 	WARN_ON_ONCE(atomic_read(&box->refcnt) != 1);
+	uncore_box_exit(box);
 	kfree(box);
 
 	if (last_box)
@@ -1079,8 +1081,10 @@ static void uncore_cpu_dying(int cpu)
 			pmu = &type->pmus[j];
 			box = *per_cpu_ptr(pmu->box, cpu);
 			*per_cpu_ptr(pmu->box, cpu) = NULL;
-			if (box && atomic_dec_and_test(&box->refcnt))
+			if (box && atomic_dec_and_test(&box->refcnt)) {
 				list_add(&box->list, &boxes_to_free);
+				uncore_box_exit(box);
+			}
 		}
 	}
 }
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.h
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
@@ -61,6 +61,7 @@ struct intel_uncore_type {
 
 struct intel_uncore_ops {
 	void (*init_box)(struct intel_uncore_box *);
+	void (*exit_box)(struct intel_uncore_box *);
 	void (*disable_box)(struct intel_uncore_box *);
 	void (*enable_box)(struct intel_uncore_box *);
 	void (*disable_event)(struct intel_uncore_box *, struct perf_event *);
@@ -306,6 +307,14 @@ static inline void uncore_box_init(struc
 	}
 }
 
+static inline void uncore_box_exit(struct intel_uncore_box *box)
+{
+	if (test_and_clear_bit(UNCORE_BOX_FLAG_INITIATED, &box->flags)) {
+		if (box->pmu->type->ops->exit_box)
+			box->pmu->type->ops->exit_box(box);
+	}
+}
+
 static inline bool uncore_box_is_fake(struct intel_uncore_box *box)
 {
 	return (box->phys_id < 0);
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
@@ -201,6 +201,11 @@ static void nhmex_uncore_msr_init_box(st
 	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, NHMEX_U_PMON_GLOBAL_EN_ALL);
 }
 
+static void nhmex_uncore_msr_exit_box(struct intel_uncore_box *box)
+{
+	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, 0);
+}
+
 static void nhmex_uncore_msr_disable_box(struct intel_uncore_box *box)
 {
 	unsigned msr = uncore_msr_box_ctl(box);
@@ -250,6 +255,7 @@ static void nhmex_uncore_msr_enable_even
 
 #define NHMEX_UNCORE_OPS_COMMON_INIT()				\
 	.init_box	= nhmex_uncore_msr_init_box,		\
+	.exit_box	= nhmex_uncore_msr_exit_box,		\
 	.disable_box	= nhmex_uncore_msr_disable_box,		\
 	.enable_box	= nhmex_uncore_msr_enable_box,		\
 	.disable_event	= nhmex_uncore_msr_disable_event,	\
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
@@ -95,6 +95,12 @@ static void snb_uncore_msr_init_box(stru
 	}
 }
 
+static void snb_uncore_msr_exit_box(struct intel_uncore_box *box)
+{
+	if (box->pmu->pmu_idx == 0)
+		wrmsrl(SNB_UNC_PERF_GLOBAL_CTL, 0);
+}
+
 static struct uncore_event_desc snb_uncore_events[] = {
 	INTEL_UNCORE_EVENT_DESC(clockticks, "event=0xff,umask=0x00"),
 	{ /* end: all zeroes */ },
@@ -116,6 +122,7 @@ static struct attribute_group snb_uncore
 
 static struct intel_uncore_ops snb_uncore_msr_ops = {
 	.init_box	= snb_uncore_msr_init_box,
+	.exit_box	= snb_uncore_msr_exit_box,
 	.disable_event	= snb_uncore_msr_disable_event,
 	.enable_event	= snb_uncore_msr_enable_event,
 	.read_counter	= uncore_msr_read_counter,
@@ -231,6 +238,11 @@ static void snb_uncore_imc_init_box(stru
 	box->hrtimer_duration = UNCORE_SNB_IMC_HRTIMER_INTERVAL;
 }
 
+static void snb_uncore_imc_exit_box(struct intel_uncore_box *box)
+{
+	iounmap(box->io_addr);
+}
+
 static void snb_uncore_imc_enable_box(struct intel_uncore_box *box)
 {}
 
@@ -458,6 +470,7 @@ static struct pmu snb_uncore_imc_pmu = {
 
 static struct intel_uncore_ops snb_uncore_imc_ops = {
 	.init_box	= snb_uncore_imc_init_box,
+	.exit_box	= snb_uncore_imc_exit_box,
 	.enable_box	= snb_uncore_imc_enable_box,
 	.disable_box	= snb_uncore_imc_disable_box,
 	.disable_event	= snb_uncore_imc_disable_event,
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c
@@ -387,6 +387,14 @@ static void snbep_uncore_pci_init_box(st
 	pci_write_config_dword(pdev, box_ctl, SNBEP_PMON_BOX_CTL_INT);
 }
 
+static void snbep_uncore_pci_exit_box(struct intel_uncore_box *box)
+{
+	struct pci_dev *pdev = box->pci_dev;
+	int box_ctl = uncore_pci_box_ctl(box);
+
+	pci_write_config_dword(pdev, box_ctl, 0);
+}
+
 static void snbep_uncore_msr_disable_box(struct intel_uncore_box *box)
 {
 	u64 config;
@@ -440,6 +448,14 @@ static void snbep_uncore_msr_init_box(st
 		wrmsrl(msr, SNBEP_PMON_BOX_CTL_INT);
 }
 
+static void snbep_uncore_msr_exit_box(struct intel_uncore_box *box)
+{
+	unsigned msr = uncore_msr_box_ctl(box);
+
+	if (msr)
+		wrmsrl(msr, 0);
+}
+
 static struct attribute *snbep_uncore_formats_attr[] = {
 	&format_attr_event.attr,
 	&format_attr_umask.attr,
@@ -567,7 +583,8 @@ static struct attribute_group snbep_unco
 
 #define SNBEP_UNCORE_MSR_OPS_COMMON_INIT()			\
 	__SNBEP_UNCORE_MSR_OPS_COMMON_INIT(),			\
-	.init_box	= snbep_uncore_msr_init_box		\
+	.init_box	= snbep_uncore_msr_init_box,		\
+	.exit_box	= snbep_uncore_msr_exit_box		\
 
 static struct intel_uncore_ops snbep_uncore_msr_ops = {
 	SNBEP_UNCORE_MSR_OPS_COMMON_INIT(),
@@ -575,6 +592,7 @@ static struct intel_uncore_ops snbep_unc
 
 #define SNBEP_UNCORE_PCI_OPS_COMMON_INIT()			\
 	.init_box	= snbep_uncore_pci_init_box,		\
+	.exit_box	= snbep_uncore_pci_exit_box,		\
 	.disable_box	= snbep_uncore_pci_disable_box,		\
 	.enable_box	= snbep_uncore_pci_enable_box,		\
 	.disable_event	= snbep_uncore_pci_disable_event,	\
@@ -1236,10 +1254,19 @@ int snbep_uncore_pci_init(void)
 static void ivbep_uncore_msr_init_box(struct intel_uncore_box *box)
 {
 	unsigned msr = uncore_msr_box_ctl(box);
+
 	if (msr)
 		wrmsrl(msr, IVBEP_PMON_BOX_CTL_INT);
 }
 
+static void ivbep_uncore_msr_exit_box(struct intel_uncore_box *box)
+{
+	unsigned msr = uncore_msr_box_ctl(box);
+
+	if (msr)
+		wrmsrl(msr, 0);
+}
+
 static void ivbep_uncore_pci_init_box(struct intel_uncore_box *box)
 {
 	struct pci_dev *pdev = box->pci_dev;
@@ -1247,8 +1274,16 @@ static void ivbep_uncore_pci_init_box(st
 	pci_write_config_dword(pdev, SNBEP_PCI_PMON_BOX_CTL, IVBEP_PMON_BOX_CTL_INT);
 }
 
+static void ivbep_uncore_pci_exit_box(struct intel_uncore_box *box)
+{
+	struct pci_dev *pdev = box->pci_dev;
+
+	pci_write_config_dword(pdev, SNBEP_PCI_PMON_BOX_CTL, 0);
+}
+
 #define IVBEP_UNCORE_MSR_OPS_COMMON_INIT()			\
 	.init_box	= ivbep_uncore_msr_init_box,		\
+	.exit_box	= ivbep_uncore_msr_exit_box,		\
 	.disable_box	= snbep_uncore_msr_disable_box,		\
 	.enable_box	= snbep_uncore_msr_enable_box,		\
 	.disable_event	= snbep_uncore_msr_disable_event,	\
@@ -1261,6 +1296,7 @@ static struct intel_uncore_ops ivbep_unc
 
 static struct intel_uncore_ops ivbep_uncore_pci_ops = {
 	.init_box	= ivbep_uncore_pci_init_box,
+	.exit_box	= ivbep_uncore_pci_exit_box,
 	.disable_box	= snbep_uncore_pci_disable_box,
 	.enable_box	= snbep_uncore_pci_enable_box,
 	.disable_event	= snbep_uncore_pci_disable_event,
@@ -1497,6 +1533,7 @@ static void ivbep_cbox_enable_event(stru
 
 static struct intel_uncore_ops ivbep_uncore_cbox_ops = {
 	.init_box		= ivbep_uncore_msr_init_box,
+	.exit_box		= ivbep_uncore_msr_exit_box,
 	.disable_box		= snbep_uncore_msr_disable_box,
 	.enable_box		= snbep_uncore_msr_enable_box,
 	.disable_event		= snbep_uncore_msr_disable_event,
@@ -1613,6 +1650,7 @@ static u64 ivbep_uncore_irp_read_counter
 
 static struct intel_uncore_ops ivbep_uncore_irp_ops = {
 	.init_box	= ivbep_uncore_pci_init_box,
+	.exit_box	= ivbep_uncore_pci_exit_box,
 	.disable_box	= snbep_uncore_pci_disable_box,
 	.enable_box	= snbep_uncore_pci_enable_box,
 	.disable_event	= ivbep_uncore_irp_disable_event,
@@ -1633,6 +1671,7 @@ static struct intel_uncore_type ivbep_un
 
 static struct intel_uncore_ops ivbep_uncore_qpi_ops = {
 	.init_box	= ivbep_uncore_pci_init_box,
+	.exit_box	= ivbep_uncore_pci_exit_box,
 	.disable_box	= snbep_uncore_pci_disable_box,
 	.enable_box	= snbep_uncore_pci_enable_box,
 	.disable_event	= snbep_uncore_pci_disable_event,
@@ -1914,6 +1953,7 @@ static void hswep_cbox_enable_event(stru
 
 static struct intel_uncore_ops knl_uncore_cha_ops = {
 	.init_box		= snbep_uncore_msr_init_box,
+	.exit_box		= snbep_uncore_msr_exit_box,
 	.disable_box		= snbep_uncore_msr_disable_box,
 	.enable_box		= snbep_uncore_msr_enable_box,
 	.disable_event		= snbep_uncore_msr_disable_event,
@@ -2008,6 +2048,7 @@ static void knl_uncore_imc_enable_event(
 
 static struct intel_uncore_ops knl_uncore_imc_ops = {
 	.init_box	= snbep_uncore_pci_init_box,
+	.exit_box	= snbep_uncore_pci_exit_box,
 	.disable_box	= snbep_uncore_pci_disable_box,
 	.enable_box	= knl_uncore_imc_enable_box,
 	.read_counter	= snbep_uncore_pci_read_counter,
@@ -2397,6 +2438,7 @@ static void hswep_cbox_enable_event(stru
 
 static struct intel_uncore_ops hswep_uncore_cbox_ops = {
 	.init_box		= snbep_uncore_msr_init_box,
+	.exit_box		= snbep_uncore_msr_exit_box,
 	.disable_box		= snbep_uncore_msr_disable_box,
 	.enable_box		= snbep_uncore_msr_enable_box,
 	.disable_event		= snbep_uncore_msr_disable_event,
@@ -2442,9 +2484,19 @@ static void hswep_uncore_sbox_msr_init_b
 	}
 }
 
+static void hswep_uncore_sbox_msr_exit_box(struct intel_uncore_box *box)
+{
+	unsigned msr = uncore_msr_box_ctl(box);
+
+	/* CHECKME: Does this need the bit dance like init() ? */
+	if (msr)
+		wrmsrl(msr, 0);
+}
+
 static struct intel_uncore_ops hswep_uncore_sbox_msr_ops = {
 	__SNBEP_UNCORE_MSR_OPS_COMMON_INIT(),
-	.init_box		= hswep_uncore_sbox_msr_init_box
+	.init_box		= hswep_uncore_sbox_msr_init_box,
+	.exit_box		= hswep_uncore_sbox_msr_exit_box
 };
 
 static struct attribute *hswep_uncore_sbox_formats_attr[] = {
@@ -2584,6 +2636,7 @@ static u64 hswep_uncore_irp_read_counter
 
 static struct intel_uncore_ops hswep_uncore_irp_ops = {
 	.init_box	= snbep_uncore_pci_init_box,
+	.exit_box	= snbep_uncore_pci_exit_box,
 	.disable_box	= snbep_uncore_pci_disable_box,
 	.enable_box	= snbep_uncore_pci_enable_box,
 	.disable_event	= ivbep_uncore_irp_disable_event,

[toc] | [prev] | [next] | [standalone]

#1336533 — RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

> 
> When tearing down the boxes nothing undoes the hardware state which
> was setup by box->init_box(). Add a box->exit_box() callback and
> implement it for the uncores which have an init_box() callback.

I don't think we need exit_box.
Because in disable_box we already freezes the box.

Also, writing 0 cannot clear hardware state. It will unfreeze the box.
The counter will start to count.

> 
> This misses the cleanup in the error exit pathes, but I cannot be bothered
> to implement it before cleaning up the rest of the driver, which makes that
> task way simpler.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/cpu/perf_event_intel_uncore.c       |    6 +-
>  arch/x86/kernel/cpu/perf_event_intel_uncore.h       |    9 +++
>  arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c |    6 ++
>  arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c   |   13 ++++
>  arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c |   57
> +++++++++++++++++++-
>  5 files changed, 88 insertions(+), 3 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> @@ -927,6 +927,7 @@ static int uncore_pci_probe(struct pci_d
>  		raw_spin_lock(&uncore_box_lock);
>  		list_del(&box->list);
>  		raw_spin_unlock(&uncore_box_lock);
> +		uncore_box_exit(box);
>  		kfree(box);
>  	}
>  	return ret;
> @@ -972,6 +973,7 @@ static void uncore_pci_remove(struct pci
>  	}
> 
>  	WARN_ON_ONCE(atomic_read(&box->refcnt) != 1);
> +	uncore_box_exit(box);
>  	kfree(box);
> 
>  	if (last_box)
> @@ -1079,8 +1081,10 @@ static void uncore_cpu_dying(int cpu)
>  			pmu = &type->pmus[j];
>  			box = *per_cpu_ptr(pmu->box, cpu);
>  			*per_cpu_ptr(pmu->box, cpu) = NULL;
> -			if (box && atomic_dec_and_test(&box->refcnt))
> +			if (box && atomic_dec_and_test(&box->refcnt)) {
>  				list_add(&box->list, &boxes_to_free);
> +				uncore_box_exit(box);
> +			}
>  		}
>  	}
>  }
> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.h
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
> @@ -61,6 +61,7 @@ struct intel_uncore_type {
> 
>  struct intel_uncore_ops {
>  	void (*init_box)(struct intel_uncore_box *);
> +	void (*exit_box)(struct intel_uncore_box *);
>  	void (*disable_box)(struct intel_uncore_box *);
>  	void (*enable_box)(struct intel_uncore_box *);
>  	void (*disable_event)(struct intel_uncore_box *, struct
> perf_event *); @@ -306,6 +307,14 @@ static inline void
> uncore_box_init(struc
>  	}
>  }
> 
> +static inline void uncore_box_exit(struct intel_uncore_box *box) {
> +	if (test_and_clear_bit(UNCORE_BOX_FLAG_INITIATED, &box-
> >flags)) {
> +		if (box->pmu->type->ops->exit_box)
> +			box->pmu->type->ops->exit_box(box);
> +	}
> +}
> +
>  static inline bool uncore_box_is_fake(struct intel_uncore_box *box)  {
>  	return (box->phys_id < 0);
> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_nhmex.c
> @@ -201,6 +201,11 @@ static void nhmex_uncore_msr_init_box(st
>  	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL,
> NHMEX_U_PMON_GLOBAL_EN_ALL);  }
> 
> +static void nhmex_uncore_msr_exit_box(struct intel_uncore_box *box)
> {
> +	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, 0); }
> +
>  static void nhmex_uncore_msr_disable_box(struct intel_uncore_box
> *box)  {
>  	unsigned msr = uncore_msr_box_ctl(box); @@ -250,6 +255,7 @@
> static void nhmex_uncore_msr_enable_even
> 
>  #define NHMEX_UNCORE_OPS_COMMON_INIT()
> 	\
>  	.init_box	= nhmex_uncore_msr_init_box,		\
> +	.exit_box	= nhmex_uncore_msr_exit_box,		\
>  	.disable_box	= nhmex_uncore_msr_disable_box,		\
>  	.enable_box	= nhmex_uncore_msr_enable_box,		\
>  	.disable_event	= nhmex_uncore_msr_disable_event,	\
> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
> @@ -95,6 +95,12 @@ static void snb_uncore_msr_init_box(stru
>  	}
>  }
> 
> +static void snb_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	if (box->pmu->pmu_idx == 0)
> +		wrmsrl(SNB_UNC_PERF_GLOBAL_CTL, 0);
> +}
> +
>  static struct uncore_event_desc snb_uncore_events[] = {
>  	INTEL_UNCORE_EVENT_DESC(clockticks,
> "event=0xff,umask=0x00"),
>  	{ /* end: all zeroes */ },
> @@ -116,6 +122,7 @@ static struct attribute_group snb_uncore
> 
>  static struct intel_uncore_ops snb_uncore_msr_ops = {
>  	.init_box	= snb_uncore_msr_init_box,
> +	.exit_box	= snb_uncore_msr_exit_box,
>  	.disable_event	= snb_uncore_msr_disable_event,
>  	.enable_event	= snb_uncore_msr_enable_event,
>  	.read_counter	= uncore_msr_read_counter,
> @@ -231,6 +238,11 @@ static void snb_uncore_imc_init_box(stru
>  	box->hrtimer_duration =
> UNCORE_SNB_IMC_HRTIMER_INTERVAL;  }
> 
> +static void snb_uncore_imc_exit_box(struct intel_uncore_box *box) {
> +	iounmap(box->io_addr);
> +}
> +
>  static void snb_uncore_imc_enable_box(struct intel_uncore_box *box)  {}
> 
> @@ -458,6 +470,7 @@ static struct pmu snb_uncore_imc_pmu = {
> 
>  static struct intel_uncore_ops snb_uncore_imc_ops = {
>  	.init_box	= snb_uncore_imc_init_box,
> +	.exit_box	= snb_uncore_imc_exit_box,
>  	.enable_box	= snb_uncore_imc_enable_box,
>  	.disable_box	= snb_uncore_imc_disable_box,
>  	.disable_event	= snb_uncore_imc_disable_event,
> --- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c
> @@ -387,6 +387,14 @@ static void snbep_uncore_pci_init_box(st
>  	pci_write_config_dword(pdev, box_ctl,
> SNBEP_PMON_BOX_CTL_INT);  }
> 
> +static void snbep_uncore_pci_exit_box(struct intel_uncore_box *box) {
> +	struct pci_dev *pdev = box->pci_dev;
> +	int box_ctl = uncore_pci_box_ctl(box);
> +
> +	pci_write_config_dword(pdev, box_ctl, 0); }
> +
>  static void snbep_uncore_msr_disable_box(struct intel_uncore_box *box)
> {
>  	u64 config;
> @@ -440,6 +448,14 @@ static void snbep_uncore_msr_init_box(st
>  		wrmsrl(msr, SNBEP_PMON_BOX_CTL_INT);
>  }
> 
> +static void snbep_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}
> +
>  static struct attribute *snbep_uncore_formats_attr[] = {
>  	&format_attr_event.attr,
>  	&format_attr_umask.attr,
> @@ -567,7 +583,8 @@ static struct attribute_group snbep_unco
> 
>  #define SNBEP_UNCORE_MSR_OPS_COMMON_INIT()
> 	\
>  	__SNBEP_UNCORE_MSR_OPS_COMMON_INIT(),
> 	\
> -	.init_box	= snbep_uncore_msr_init_box		\
> +	.init_box	= snbep_uncore_msr_init_box,		\
> +	.exit_box	= snbep_uncore_msr_exit_box		\
> 
>  static struct intel_uncore_ops snbep_uncore_msr_ops = {
>  	SNBEP_UNCORE_MSR_OPS_COMMON_INIT(),
> @@ -575,6 +592,7 @@ static struct intel_uncore_ops snbep_unc
> 
>  #define SNBEP_UNCORE_PCI_OPS_COMMON_INIT()			\
>  	.init_box	= snbep_uncore_pci_init_box,		\
> +	.exit_box	= snbep_uncore_pci_exit_box,		\
>  	.disable_box	= snbep_uncore_pci_disable_box,		\
>  	.enable_box	= snbep_uncore_pci_enable_box,		\
>  	.disable_event	= snbep_uncore_pci_disable_event,	\
> @@ -1236,10 +1254,19 @@ int snbep_uncore_pci_init(void)  static void
> ivbep_uncore_msr_init_box(struct intel_uncore_box *box)  {
>  	unsigned msr = uncore_msr_box_ctl(box);
> +
>  	if (msr)
>  		wrmsrl(msr, IVBEP_PMON_BOX_CTL_INT);
>  }
> 
> +static void ivbep_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}
> +
>  static void ivbep_uncore_pci_init_box(struct intel_uncore_box *box)  {
>  	struct pci_dev *pdev = box->pci_dev;
> @@ -1247,8 +1274,16 @@ static void ivbep_uncore_pci_init_box(st
>  	pci_write_config_dword(pdev, SNBEP_PCI_PMON_BOX_CTL,
> IVBEP_PMON_BOX_CTL_INT);  }
> 
> +static void ivbep_uncore_pci_exit_box(struct intel_uncore_box *box) {
> +	struct pci_dev *pdev = box->pci_dev;
> +
> +	pci_write_config_dword(pdev, SNBEP_PCI_PMON_BOX_CTL, 0); }
> +
>  #define IVBEP_UNCORE_MSR_OPS_COMMON_INIT()
> 	\
>  	.init_box	= ivbep_uncore_msr_init_box,		\
> +	.exit_box	= ivbep_uncore_msr_exit_box,		\
>  	.disable_box	= snbep_uncore_msr_disable_box,		\
>  	.enable_box	= snbep_uncore_msr_enable_box,		\
>  	.disable_event	= snbep_uncore_msr_disable_event,	\
> @@ -1261,6 +1296,7 @@ static struct intel_uncore_ops ivbep_unc
> 
>  static struct intel_uncore_ops ivbep_uncore_pci_ops = {
>  	.init_box	= ivbep_uncore_pci_init_box,
> +	.exit_box	= ivbep_uncore_pci_exit_box,
>  	.disable_box	= snbep_uncore_pci_disable_box,
>  	.enable_box	= snbep_uncore_pci_enable_box,
>  	.disable_event	= snbep_uncore_pci_disable_event,
> @@ -1497,6 +1533,7 @@ static void ivbep_cbox_enable_event(stru
> 
>  static struct intel_uncore_ops ivbep_uncore_cbox_ops = {
>  	.init_box		= ivbep_uncore_msr_init_box,
> +	.exit_box		= ivbep_uncore_msr_exit_box,
>  	.disable_box		= snbep_uncore_msr_disable_box,
>  	.enable_box		= snbep_uncore_msr_enable_box,
>  	.disable_event		= snbep_uncore_msr_disable_event,
> @@ -1613,6 +1650,7 @@ static u64 ivbep_uncore_irp_read_counter
> 
>  static struct intel_uncore_ops ivbep_uncore_irp_ops = {
>  	.init_box	= ivbep_uncore_pci_init_box,
> +	.exit_box	= ivbep_uncore_pci_exit_box,
>  	.disable_box	= snbep_uncore_pci_disable_box,
>  	.enable_box	= snbep_uncore_pci_enable_box,
>  	.disable_event	= ivbep_uncore_irp_disable_event,
> @@ -1633,6 +1671,7 @@ static struct intel_uncore_type ivbep_un
> 
>  static struct intel_uncore_ops ivbep_uncore_qpi_ops = {
>  	.init_box	= ivbep_uncore_pci_init_box,
> +	.exit_box	= ivbep_uncore_pci_exit_box,
>  	.disable_box	= snbep_uncore_pci_disable_box,
>  	.enable_box	= snbep_uncore_pci_enable_box,
>  	.disable_event	= snbep_uncore_pci_disable_event,
> @@ -1914,6 +1953,7 @@ static void hswep_cbox_enable_event(stru
> 
>  static struct intel_uncore_ops knl_uncore_cha_ops = {
>  	.init_box		= snbep_uncore_msr_init_box,
> +	.exit_box		= snbep_uncore_msr_exit_box,
>  	.disable_box		= snbep_uncore_msr_disable_box,
>  	.enable_box		= snbep_uncore_msr_enable_box,
>  	.disable_event		= snbep_uncore_msr_disable_event,
> @@ -2008,6 +2048,7 @@ static void knl_uncore_imc_enable_event(
> 
>  static struct intel_uncore_ops knl_uncore_imc_ops = {
>  	.init_box	= snbep_uncore_pci_init_box,
> +	.exit_box	= snbep_uncore_pci_exit_box,
>  	.disable_box	= snbep_uncore_pci_disable_box,
>  	.enable_box	= knl_uncore_imc_enable_box,
>  	.read_counter	= snbep_uncore_pci_read_counter,
> @@ -2397,6 +2438,7 @@ static void hswep_cbox_enable_event(stru
> 
>  static struct intel_uncore_ops hswep_uncore_cbox_ops = {
>  	.init_box		= snbep_uncore_msr_init_box,
> +	.exit_box		= snbep_uncore_msr_exit_box,
>  	.disable_box		= snbep_uncore_msr_disable_box,
>  	.enable_box		= snbep_uncore_msr_enable_box,
>  	.disable_event		= snbep_uncore_msr_disable_event,
> @@ -2442,9 +2484,19 @@ static void hswep_uncore_sbox_msr_init_b
>  	}
>  }
> 
> +static void hswep_uncore_sbox_msr_exit_box(struct intel_uncore_box
> +*box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	/* CHECKME: Does this need the bit dance like init() ? */
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}
> +
>  static struct intel_uncore_ops hswep_uncore_sbox_msr_ops = {
>  	__SNBEP_UNCORE_MSR_OPS_COMMON_INIT(),
> -	.init_box		= hswep_uncore_sbox_msr_init_box
> +	.init_box		= hswep_uncore_sbox_msr_init_box,
> +	.exit_box		= hswep_uncore_sbox_msr_exit_box
>  };
> 
>  static struct attribute *hswep_uncore_sbox_formats_attr[] = { @@ -
> 2584,6 +2636,7 @@ static u64 hswep_uncore_irp_read_counter
> 
>  static struct intel_uncore_ops hswep_uncore_irp_ops = {
>  	.init_box	= snbep_uncore_pci_init_box,
> +	.exit_box	= snbep_uncore_pci_exit_box,
>  	.disable_box	= snbep_uncore_pci_disable_box,
>  	.enable_box	= snbep_uncore_pci_enable_box,
>  	.disable_event	= ivbep_uncore_irp_disable_event,
> 

[toc] | [prev] | [next] | [standalone]

#1336623 — RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

On Wed, 17 Feb 2016, Liang, Kan wrote:
> > When tearing down the boxes nothing undoes the hardware state which
> > was setup by box->init_box(). Add a box->exit_box() callback and
> > implement it for the uncores which have an init_box() callback.
> 
> I don't think we need exit_box.
> Because in disable_box we already freezes the box.

init_box() != enable_box() 
exit_box() != disable_box()

And we certainly want to clear stuff which enables things at a different
msr/config word location than what you do with the enable/disable callbacks.

I'm not a fan of leaving hardware in some random state.

> Also, writing 0 cannot clear hardware state. It will unfreeze the box.
> The counter will start to count.

Nonsense.
 
> > @@ -201,6 +201,11 @@ static void nhmex_uncore_msr_init_box(st
> >  	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL,
> > NHMEX_U_PMON_GLOBAL_EN_ALL);  }

> > +static void nhmex_uncore_msr_exit_box(struct intel_uncore_box *box)
> > {
> > +	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, 0); }

The reset value for this register is 0. So how is that wrong?

> > +static void snb_uncore_msr_exit_box(struct intel_uncore_box *box) {
> > +	if (box->pmu->pmu_idx == 0)
> > +		wrmsrl(SNB_UNC_PERF_GLOBAL_CTL, 0);
> > +}

Ditto.

> > +static void snb_uncore_imc_exit_box(struct intel_uncore_box *box) {
> > +	iounmap(box->io_addr);

That's definitely required, because it would leak a mapping.

I know Intel folks do not care about error handling and a few reference leaks,
but I care very much.

If there is a single instance of exit_box() in that patch which flips the
wrong bits, then please point it out with the proper reference in the manual
and not with such half baken statements as above.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1336801 — RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

> 
> > > @@ -201,6 +201,11 @@ static void nhmex_uncore_msr_init_box(st
> > >  	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL,
> > > NHMEX_U_PMON_GLOBAL_EN_ALL);  }
> 
> > > +static void nhmex_uncore_msr_exit_box(struct intel_uncore_box
> *box)
> > > {
> > > +	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, 0); }
> 
> The reset value for this register is 0. So how is that wrong?
> 
> > > +static void snb_uncore_msr_exit_box(struct intel_uncore_box *box)
> {
> > > +	if (box->pmu->pmu_idx == 0)
> > > +		wrmsrl(SNB_UNC_PERF_GLOBAL_CTL, 0); }
> 
> Ditto.
> 
> > > +static void snb_uncore_imc_exit_box(struct intel_uncore_box *box) {
> > > +	iounmap(box->io_addr);
> 
> That's definitely required, because it would leak a mapping.
> 
> I know Intel folks do not care about error handling and a few reference
> leaks, but I care very much.
> 
> If there is a single instance of exit_box() in that patch which flips the wrong
> bits, then please point it out with the proper reference in the manual and
> not with such half baken statements as above.
> 

Sorry, I didn't make it clear.
For the older server platforms like nhmex and client platforms like snb, I agree
with you on nhmex_uncore_msr_exit_box and snb_uncore_imc_exit_box.

However, for newer server platforms (start from IVB server), we cannot write 0
to rsv bit of BOX control registers. The behavior is undefined.
The following codes may have issues.
It looks we also write 0 to rsv bit in box_init. We may need to fix it.

You can find all the server uncore documents here.
https://software.intel.com/en-us/blogs/2014/07/11/documentation-for-uncore-performance-monitoring-units

> +static void snbep_uncore_pci_exit_box(struct intel_uncore_box *box) {
> +	struct pci_dev *pdev = box->pci_dev;
> +	int box_ctl = uncore_pci_box_ctl(box);
> +
> +	pci_write_config_dword(pdev, box_ctl, 0); }
> +

> +static void snbep_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}

> +static void ivbep_uncore_msr_exit_box(struct intel_uncore_box *box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}

> +static void ivbep_uncore_pci_exit_box(struct intel_uncore_box *box) {
> +	struct pci_dev *pdev = box->pci_dev;
> +
> +	pci_write_config_dword(pdev, SNBEP_PCI_PMON_BOX_CTL, 0); }
> +

> +static void hswep_uncore_sbox_msr_exit_box(struct intel_uncore_box
> +*box) {
> +	unsigned msr = uncore_msr_box_ctl(box);
> +
> +	/* CHECKME: Does this need the bit dance like init() ? */
> +	if (msr)
> +		wrmsrl(msr, 0);
> +}
> +

Thanks,

Kan

[toc] | [prev] | [next] | [standalone]

#1336804 — RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

On Wed, 17 Feb 2016, Liang, Kan wrote:
> > If there is a single instance of exit_box() in that patch which flips the wrong
> > bits, then please point it out with the proper reference in the manual and
> > not with such half baken statements as above.
> > 
> Sorry, I didn't make it clear.
> For the older server platforms like nhmex and client platforms like snb, I agree
> with you on nhmex_uncore_msr_exit_box and snb_uncore_imc_exit_box.
> 
> However, for newer server platforms (start from IVB server), we cannot write 0
> to rsv bit of BOX control registers. The behavior is undefined.
> The following codes may have issues.
> It looks we also write 0 to rsv bit in box_init. We may need to fix it.

Each platform can have its own init/exit callbacks or just omit them when not
needed. But we certainly want them for platforms where it is either needed
(i.e. undo ioremap) or makes sense from the hardware POV.

Thanks,

	tglx

 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web