[patch 0/5] powercap/intel_rapl: Fixes, hotplug conversion and simplifcation

[patch 0/5] powercap/intel_rapl: Fixes, hotplug conversion and simplifcation

[Thomas Gleixner]

The driver fails to:

 - initialize packages which are not available at driver init time, though
   the value of that initialization is completely unclear as nothing ever
   uses these values. I fixed it up nevertheless and leave it to the
   maintainers to decide whether it should completely go away.

 - to propagate error codes in the hotplug online path, where a
   registration fails and the package data is freed, but return code is 0.

The initialization/removal code of that driver is a maze of duplicated code
which is more or less the same as the cpu hotplug code. After switching
over the driver to the hotplug statemachine, the whole init/removal
machinery can be replaced by installing/removing the hotplug state.

The total damage is:

 intel_rapl.c |  363 ++++++++++++++++-------------------------------------------
 1 file changed, 104 insertions(+), 259 deletions(-)

and the binary size shrinks as well:

   text	   data	    bss	    dec	    hex
   7996	    625	     32	   8653	   21cd	  Before
   7216	    593	     32	   7841	   1ea1	  After

Thanks,

	tglx

[patch 1/5] powercap/intel_rapl: Add missing domain data update on hotplug

[Thomas Gleixner]

[-- Attachment #1: powercapintel_rapl_Add_missing_domain_data_update_on_hotplug.patch --]
[-- Type: text/plain, Size: 3602 bytes --]

The domain data of packages is only updated at init time, but new packages
created by hotplug miss that treatment.

Add it there and remove the global update at init time, because it's now
obsolete.

The more interesting question is why rapl_update_domain_data() exists at
all as nothing ever uses that data.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/powercap/intel_rapl.c |   42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -1164,24 +1164,20 @@ static const struct x86_cpu_id rapl_ids[
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
 
-/* read once for all raw primitive data for all packages, domains */
-static void rapl_update_domain_data(void)
+/* Read once for all raw primitive data for domains */
+static void rapl_update_domain_data(struct rapl_package *rp)
 {
 	int dmn, prim;
 	u64 val;
-	struct rapl_package *rp;
 
-	list_for_each_entry(rp, &rapl_packages, plist) {
-		for (dmn = 0; dmn < rp->nr_domains; dmn++) {
-			pr_debug("update package %d domain %s data\n", rp->id,
-				rp->domains[dmn].name);
-			/* exclude non-raw primitives */
-			for (prim = 0; prim < NR_RAW_PRIMITIVES; prim++)
-				if (!rapl_read_data_raw(&rp->domains[dmn], prim,
-								rpi[prim].unit,
-								&val))
-					rp->domains[dmn].rdd.primitives[prim] =
-									val;
+	for (dmn = 0; dmn < rp->nr_domains; dmn++) {
+		pr_debug("update package %d domain %s data\n", rp->id,
+			 rp->domains[dmn].name);
+		/* exclude non-raw primitives */
+		for (prim = 0; prim < NR_RAW_PRIMITIVES; prim++) {
+			if (!rapl_read_data_raw(&rp->domains[dmn], prim,
+						rpi[prim].unit, &val))
+				rp->domains[dmn].rdd.primitives[prim] =	val;
 		}
 	}
 
@@ -1234,10 +1230,12 @@ static int rapl_unregister_powercap(void
 static int rapl_package_register_powercap(struct rapl_package *rp)
 {
 	struct rapl_domain *rd;
-	int ret = 0;
 	char dev_name[17]; /* max domain name = 7 + 1 + 8 for int + 1 for null*/
 	struct powercap_zone *power_zone = NULL;
-	int nr_pl;
+	int nr_pl, ret;;
+
+	/* Update the domain data of the new package */
+	rapl_update_domain_data(rp);
 
 	/* first we register package domain as the parent zone*/
 	for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
@@ -1257,8 +1255,7 @@ static int rapl_package_register_powerca
 			if (IS_ERR(power_zone)) {
 				pr_debug("failed to register package, %d\n",
 					rp->id);
-				ret = PTR_ERR(power_zone);
-				goto exit_package;
+				return PTR_ERR(power_zone);
 			}
 			/* track parent zone in per package/socket data */
 			rp->power_zone = power_zone;
@@ -1268,8 +1265,7 @@ static int rapl_package_register_powerca
 	}
 	if (!power_zone) {
 		pr_err("no package domain found, unknown topology!\n");
-		ret = -ENODEV;
-		goto exit_package;
+		return -ENODEV;
 	}
 	/* now register domains as children of the socket/package*/
 	for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
@@ -1290,9 +1286,8 @@ static int rapl_package_register_powerca
 			goto err_cleanup;
 		}
 	}
+	return 0;
 
-exit_package:
-	return ret;
 err_cleanup:
 	/* clean up previously initialized domains within the package if we
 	 * failed after the first domain setup.
@@ -1357,8 +1352,7 @@ static int rapl_register_powercap(void)
 		pr_debug("failed to register powercap control_type.\n");
 		return PTR_ERR(control_type);
 	}
-	/* read the initial data */
-	rapl_update_domain_data();
+
 	list_for_each_entry(rp, &rapl_packages, plist)
 		if (rapl_package_register_powercap(rp))
 			goto err_cleanup_package;

Re: [patch 1/5] powercap/intel_rapl: Add missing domain data update on hotplug

[Jacob Pan]

On Tue, 22 Nov 2016 21:15:58 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> The more interesting question is why rapl_update_domain_data() exists
> at all as nothing ever uses that data.

You are right, initial domain data are read at initialization time but
not used. I seem to remember I did this trying to save and restore rapl
raw settings between module load and unload. But I forgot to use it
for restore :(.

Thanks,

Jacob

[patch 2/5] powercap/intel_rapl: Propagate error code when registration fails

[Thomas Gleixner]

[-- Attachment #1: powercap-intel_rapl--Propagate-error-code-when-registration-fails.patch --]
[-- Type: text/plain, Size: 1068 bytes --]

If rapl_package_register_powercap() fails in rapl_add_package() the
function happily returns 0.

Capture the error code and propagate it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/powercap/intel_rapl.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -1563,9 +1563,8 @@ static void rapl_remove_package(struct r
 /* called from CPU hotplug notifier, hotplug lock held */
 static int rapl_add_package(int cpu)
 {
-	int ret = 0;
-	int phy_package_id;
 	struct rapl_package *rp;
+	int ret, phy_package_id;
 
 	phy_package_id = topology_physical_package_id(cpu);
 	rp = kzalloc(sizeof(struct rapl_package), GFP_KERNEL);
@@ -1583,10 +1582,11 @@ static int rapl_add_package(int cpu)
 		ret = -ENODEV;
 		goto err_free_package;
 	}
-	if (!rapl_package_register_powercap(rp)) {
+	ret = rapl_package_register_powercap(rp);
+	if (!ret) {
 		INIT_LIST_HEAD(&rp->plist);
 		list_add(&rp->plist, &rapl_packages);
-		return ret;
+		return 0;
 	}
 
 err_free_package:

[patch 3/5] powercap/intel rapl: Convert to hotplug state machine

[Thomas Gleixner]

[-- Attachment #1: powercapintel_rapl_Convert_to_hotplug_state_machine.patch --]
[-- Type: text/plain, Size: 3892 bytes --]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Install the callbacks via the state machine as a first step. The init/exit
code is a duplicate of the hotplug code. This is cleaned up in a
consecutive patch.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/powercap/intel_rapl.c |   94 +++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 45 deletions(-)

--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -1603,50 +1603,48 @@ static int rapl_add_package(int cpu)
  * associated domains. Cooling devices are handled accordingly at
  * per-domain level.
  */
-static int rapl_cpu_callback(struct notifier_block *nfb,
-				unsigned long action, void *hcpu)
+static int rapl_cpu_online(unsigned int cpu)
+{
+	struct rapl_package *rp;
+	int phy_package_id;
+
+	phy_package_id = topology_physical_package_id(cpu);
+
+	rp = find_package_by_id(phy_package_id);
+	if (rp)
+		rp->nr_cpus++;
+	else
+		rapl_add_package(cpu);
+	return 0;
+}
+
+static int rapl_cpu_down_prep(unsigned int cpu)
 {
-	unsigned long cpu = (unsigned long)hcpu;
 	int phy_package_id;
 	struct rapl_package *rp;
 	int lead_cpu;
 
 	phy_package_id = topology_physical_package_id(cpu);
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
-	case CPU_DOWN_FAILED:
-	case CPU_DOWN_FAILED_FROZEN:
-		rp = find_package_by_id(phy_package_id);
-		if (rp)
-			++rp->nr_cpus;
-		else
-			rapl_add_package(cpu);
-		break;
-	case CPU_DOWN_PREPARE:
-	case CPU_DOWN_PREPARE_FROZEN:
-		rp = find_package_by_id(phy_package_id);
-		if (!rp)
-			break;
-		if (--rp->nr_cpus == 0)
-			rapl_remove_package(rp);
-		else if (cpu == rp->lead_cpu) {
-			/* choose another active cpu in the package */
-			lead_cpu = cpumask_any_but(topology_core_cpumask(cpu), cpu);
-			if (lead_cpu < nr_cpu_ids)
-				rp->lead_cpu = lead_cpu;
-			else /* should never go here */
-				pr_err("no active cpu available for package %d\n",
-					phy_package_id);
+	rp = find_package_by_id(phy_package_id);
+	if (!rp)
+		return 0;
+	if (--rp->nr_cpus == 0) {
+		rapl_remove_package(rp);
+	} else if (cpu == rp->lead_cpu) {
+		/* choose another active cpu in the package */
+		lead_cpu = cpumask_any_but(topology_core_cpumask(cpu), cpu);
+		if (lead_cpu < nr_cpu_ids) {
+			rp->lead_cpu = lead_cpu;
+		} else {
+			/* should never go here */
+			pr_err("no active cpu available for package %d\n",
+			       phy_package_id);
 		}
 	}
-
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block rapl_cpu_notifier = {
-	.notifier_call = rapl_cpu_callback,
-};
+static enum cpuhp_state pcap_rapl_online;
 
 static int __init rapl_init(void)
 {
@@ -1663,36 +1661,42 @@ static int __init rapl_init(void)
 
 	rapl_defaults = (struct rapl_defaults *)id->driver_data;
 
-	cpu_notifier_register_begin();
-
 	/* prevent CPU hotplug during detection */
 	get_online_cpus();
 	ret = rapl_detect_topology();
 	if (ret)
-		goto done;
+		goto err;
 
 	if (rapl_register_powercap()) {
-		rapl_cleanup_data();
 		ret = -ENODEV;
-		goto done;
+		goto err_cleanup;
 	}
-	__register_hotcpu_notifier(&rapl_cpu_notifier);
-done:
+	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
+					"powercap/rapl:online",
+					rapl_cpu_online, rapl_cpu_down_prep);
+	if (ret < 0)
+		goto err_unreg;
+	pcap_rapl_online = ret;
 	put_online_cpus();
-	cpu_notifier_register_done();
+	return 0;
 
+err_unreg:
+	rapl_unregister_powercap();
+
+err_cleanup:
+	rapl_cleanup_data();
+err:
+	put_online_cpus();
 	return ret;
 }
 
 static void __exit rapl_exit(void)
 {
-	cpu_notifier_register_begin();
 	get_online_cpus();
-	__unregister_hotcpu_notifier(&rapl_cpu_notifier);
+	cpuhp_remove_state(pcap_rapl_online);
 	rapl_unregister_powercap();
 	rapl_cleanup_data();
 	put_online_cpus();
-	cpu_notifier_register_done();
 }
 
 module_init(rapl_init);

[patch 4/5] powercap/intel_rapl: Cleanup duplicated init code

[Thomas Gleixner]

[-- Attachment #1: powercapintel_rapl_Cleanup_duplicated_init_code.patch --]
[-- Type: text/plain, Size: 10603 bytes --]

The whole init/exit code is a duplicate of the cpuhotplug code. So we can
just let the hotplug code do the actual work of setting up and tearing down
the domains.

This also restores the package hardware when a package is removed during
hotplug instead of leaving it in the last configured state and only reset
it when the driver is removed.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/powercap/intel_rapl.c |  234 ++++++++----------------------------------
 1 file changed, 46 insertions(+), 188 deletions(-)

--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -275,18 +275,6 @@ static struct rapl_package *find_package
 	return NULL;
 }
 
-/* caller must hold cpu hotplug lock */
-static void rapl_cleanup_data(void)
-{
-	struct rapl_package *p, *tmp;
-
-	list_for_each_entry_safe(p, tmp, &rapl_packages, plist) {
-		kfree(p->domains);
-		list_del(&p->plist);
-		kfree(p);
-	}
-}
-
 static int get_energy_counter(struct powercap_zone *power_zone, u64 *energy_raw)
 {
 	struct rapl_domain *rd;
@@ -976,10 +964,20 @@ static void package_power_limit_irq_save
 	smp_call_function_single(rp->lead_cpu, power_limit_irq_save_cpu, rp, 1);
 }
 
-static void power_limit_irq_restore_cpu(void *info)
+/*
+ * Restore per package power limit interrupt enable state. Called from cpu
+ * hotplug code on package removal.
+ */
+static void package_power_limit_irq_restore(struct rapl_package *rp)
 {
-	u32 l, h = 0;
-	struct rapl_package *rp = (struct rapl_package *)info;
+	u32 l, h;
+
+	if (!boot_cpu_has(X86_FEATURE_PTS) || !boot_cpu_has(X86_FEATURE_PLN))
+		return;
+
+	/* irq enable state not saved, nothing to restore */
+	if (!(rp->power_limit_irq & PACKAGE_PLN_INT_SAVED))
+		return;
 
 	rdmsr_safe(MSR_IA32_PACKAGE_THERM_INTERRUPT, &l, &h);
 
@@ -991,19 +989,6 @@ static void power_limit_irq_restore_cpu(
 	wrmsr_safe(MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
 }
 
-/* restore per package power limit interrupt enable state */
-static void package_power_limit_irq_restore(struct rapl_package *rp)
-{
-	if (!boot_cpu_has(X86_FEATURE_PTS) || !boot_cpu_has(X86_FEATURE_PLN))
-		return;
-
-	/* irq enable state not saved, nothing to restore */
-	if (!(rp->power_limit_irq & PACKAGE_PLN_INT_SAVED))
-		return;
-
-	smp_call_function_single(rp->lead_cpu, power_limit_irq_restore_cpu, rp, 1);
-}
-
 static void set_floor_freq_default(struct rapl_domain *rd, bool mode)
 {
 	int nr_powerlimit = find_nr_power_limit(rd);
@@ -1183,48 +1168,14 @@ static void rapl_update_domain_data(stru
 
 }
 
-static int rapl_unregister_powercap(void)
+static void rapl_unregister_powercap(void)
 {
-	struct rapl_package *rp;
-	struct rapl_domain *rd, *rd_package = NULL;
-
-	/* unregister all active rapl packages from the powercap layer,
-	 * hotplug lock held
-	 */
-	list_for_each_entry(rp, &rapl_packages, plist) {
-		package_power_limit_irq_restore(rp);
-
-		for (rd = rp->domains; rd < rp->domains + rp->nr_domains;
-		     rd++) {
-			pr_debug("remove package, undo power limit on %d: %s\n",
-				rp->id, rd->name);
-			rapl_write_data_raw(rd, PL1_ENABLE, 0);
-			rapl_write_data_raw(rd, PL1_CLAMP, 0);
-			if (find_nr_power_limit(rd) > 1) {
-				rapl_write_data_raw(rd, PL2_ENABLE, 0);
-				rapl_write_data_raw(rd, PL2_CLAMP, 0);
-			}
-			if (rd->id == RAPL_DOMAIN_PACKAGE) {
-				rd_package = rd;
-				continue;
-			}
-			powercap_unregister_zone(control_type, &rd->power_zone);
-		}
-		/* do the package zone last */
-		if (rd_package)
-			powercap_unregister_zone(control_type,
-						&rd_package->power_zone);
-	}
-
 	if (platform_rapl_domain) {
 		powercap_unregister_zone(control_type,
 					 &platform_rapl_domain->power_zone);
 		kfree(platform_rapl_domain);
 	}
-
 	powercap_unregister_control_type(control_type);
-
-	return 0;
 }
 
 static int rapl_package_register_powercap(struct rapl_package *rp)
@@ -1289,7 +1240,8 @@ static int rapl_package_register_powerca
 	return 0;
 
 err_cleanup:
-	/* clean up previously initialized domains within the package if we
+	/*
+	 * Clean up previously initialized domains within the package if we
 	 * failed after the first domain setup.
 	 */
 	while (--rd >= rp->domains) {
@@ -1300,7 +1252,7 @@ static int rapl_package_register_powerca
 	return ret;
 }
 
-static int rapl_register_psys(void)
+static int __init rapl_register_psys(void)
 {
 	struct rapl_domain *rd;
 	struct powercap_zone *power_zone;
@@ -1341,39 +1293,14 @@ static int rapl_register_psys(void)
 	return 0;
 }
 
-static int rapl_register_powercap(void)
+static int __init rapl_register_powercap(void)
 {
-	struct rapl_domain *rd;
-	struct rapl_package *rp;
-	int ret = 0;
-
 	control_type = powercap_register_control_type(NULL, "intel-rapl", NULL);
 	if (IS_ERR(control_type)) {
 		pr_debug("failed to register powercap control_type.\n");
 		return PTR_ERR(control_type);
 	}
-
-	list_for_each_entry(rp, &rapl_packages, plist)
-		if (rapl_package_register_powercap(rp))
-			goto err_cleanup_package;
-
-	/* Don't bail out if PSys is not supported */
-	rapl_register_psys();
-
-	return ret;
-
-err_cleanup_package:
-	/* clean up previously initialized packages */
-	list_for_each_entry_continue_reverse(rp, &rapl_packages, plist) {
-		for (rd = rp->domains; rd < rp->domains + rp->nr_domains;
-		     rd++) {
-			pr_debug("unregister zone/package %d, %s domain\n",
-				rp->id, rd->name);
-			powercap_unregister_zone(control_type, &rd->power_zone);
-		}
-	}
-
-	return ret;
+	return 0;
 }
 
 static int rapl_check_domain(int cpu, int domain)
@@ -1446,9 +1373,8 @@ static void rapl_detect_powerlimit(struc
  */
 static int rapl_detect_domains(struct rapl_package *rp, int cpu)
 {
-	int i;
-	int ret = 0;
 	struct rapl_domain *rd;
+	int i;
 
 	for (i = 0; i < RAPL_DOMAIN_MAX; i++) {
 		/* use physical package id to read counters */
@@ -1460,84 +1386,20 @@ static int rapl_detect_domains(struct ra
 	rp->nr_domains = bitmap_weight(&rp->domain_map,	RAPL_DOMAIN_MAX);
 	if (!rp->nr_domains) {
 		pr_debug("no valid rapl domains found in package %d\n", rp->id);
-		ret = -ENODEV;
-		goto done;
+		return -ENODEV;
 	}
 	pr_debug("found %d domains on package %d\n", rp->nr_domains, rp->id);
 
 	rp->domains = kcalloc(rp->nr_domains + 1, sizeof(struct rapl_domain),
 			GFP_KERNEL);
-	if (!rp->domains) {
-		ret = -ENOMEM;
-		goto done;
-	}
+	if (!rp->domains)
+		return -ENOMEM;
+
 	rapl_init_domains(rp);
 
 	for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++)
 		rapl_detect_powerlimit(rd);
 
-
-
-done:
-	return ret;
-}
-
-static bool is_package_new(int package)
-{
-	struct rapl_package *rp;
-
-	/* caller prevents cpu hotplug, there will be no new packages added
-	 * or deleted while traversing the package list, no need for locking.
-	 */
-	list_for_each_entry(rp, &rapl_packages, plist)
-		if (package == rp->id)
-			return false;
-
-	return true;
-}
-
-/* RAPL interface can be made of a two-level hierarchy: package level and domain
- * level. We first detect the number of packages then domains of each package.
- * We have to consider the possiblity of CPU online/offline due to hotplug and
- * other scenarios.
- */
-static int rapl_detect_topology(void)
-{
-	int i;
-	int phy_package_id;
-	struct rapl_package *new_package, *rp;
-
-	for_each_online_cpu(i) {
-		phy_package_id = topology_physical_package_id(i);
-		if (is_package_new(phy_package_id)) {
-			new_package = kzalloc(sizeof(*rp), GFP_KERNEL);
-			if (!new_package) {
-				rapl_cleanup_data();
-				return -ENOMEM;
-			}
-			/* add the new package to the list */
-			new_package->id = phy_package_id;
-			new_package->nr_cpus = 1;
-			/* use the first active cpu of the package to access */
-			new_package->lead_cpu = i;
-			/* check if the package contains valid domains */
-			if (rapl_detect_domains(new_package, i) ||
-				rapl_defaults->check_unit(new_package, i)) {
-				kfree(new_package->domains);
-				kfree(new_package);
-				/* free up the packages already initialized */
-				rapl_cleanup_data();
-				return -ENODEV;
-			}
-			INIT_LIST_HEAD(&new_package->plist);
-			list_add(&new_package->plist, &rapl_packages);
-		} else {
-			rp = find_package_by_id(phy_package_id);
-			if (rp)
-				++rp->nr_cpus;
-		}
-	}
-
 	return 0;
 }
 
@@ -1546,12 +1408,21 @@ static void rapl_remove_package(struct r
 {
 	struct rapl_domain *rd, *rd_package = NULL;
 
+	package_power_limit_irq_restore(rp);
+
 	for (rd = rp->domains; rd < rp->domains + rp->nr_domains; rd++) {
+		rapl_write_data_raw(rd, PL1_ENABLE, 0);
+		rapl_write_data_raw(rd, PL1_CLAMP, 0);
+		if (find_nr_power_limit(rd) > 1) {
+			rapl_write_data_raw(rd, PL2_ENABLE, 0);
+			rapl_write_data_raw(rd, PL2_CLAMP, 0);
+		}
 		if (rd->id == RAPL_DOMAIN_PACKAGE) {
 			rd_package = rd;
 			continue;
 		}
-		pr_debug("remove package %d, %s domain\n", rp->id, rd->name);
+		pr_debug("remove package, undo power limit on %d: %s\n",
+			 rp->id, rd->name);
 		powercap_unregister_zone(control_type, &rd->power_zone);
 	}
 	/* do parent zone last */
@@ -1611,11 +1482,11 @@ static int rapl_cpu_online(unsigned int
 	phy_package_id = topology_physical_package_id(cpu);
 
 	rp = find_package_by_id(phy_package_id);
-	if (rp)
+	if (rp) {
 		rp->nr_cpus++;
-	else
-		rapl_add_package(cpu);
-	return 0;
+		return 0;
+	}
+	return rapl_add_package(cpu);
 }
 
 static int rapl_cpu_down_prep(unsigned int cpu)
@@ -1648,8 +1519,8 @@ static enum cpuhp_state pcap_rapl_online
 
 static int __init rapl_init(void)
 {
-	int ret = 0;
 	const struct x86_cpu_id *id;
+	int ret;
 
 	id = x86_match_cpu(rapl_ids);
 	if (!id) {
@@ -1661,42 +1532,29 @@ static int __init rapl_init(void)
 
 	rapl_defaults = (struct rapl_defaults *)id->driver_data;
 
-	/* prevent CPU hotplug during detection */
-	get_online_cpus();
-	ret = rapl_detect_topology();
+	ret = rapl_register_powercap();
 	if (ret)
-		goto err;
+		return ret;
 
-	if (rapl_register_powercap()) {
-		ret = -ENODEV;
-		goto err_cleanup;
-	}
-	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
-					"powercap/rapl:online",
-					rapl_cpu_online, rapl_cpu_down_prep);
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powercap/rapl:online",
+				rapl_cpu_online, rapl_cpu_down_prep);
 	if (ret < 0)
 		goto err_unreg;
 	pcap_rapl_online = ret;
-	put_online_cpus();
+
+	/* Don't bail out if PSys is not supported */
+	rapl_register_psys();
 	return 0;
 
 err_unreg:
 	rapl_unregister_powercap();
-
-err_cleanup:
-	rapl_cleanup_data();
-err:
-	put_online_cpus();
 	return ret;
 }
 
 static void __exit rapl_exit(void)
 {
-	get_online_cpus();
 	cpuhp_remove_state(pcap_rapl_online);
 	rapl_unregister_powercap();
-	rapl_cleanup_data();
-	put_online_cpus();
 }
 
 module_init(rapl_init);

[patch 5/5] powercap/intel_rapl: Track active CPUs internally

[Thomas Gleixner]

[-- Attachment #1: powercap-intel_rapl--Track-active-cpus-internally.patch --]
[-- Type: text/plain, Size: 3793 bytes --]

The ability of the CPU hotplug code to stop online/offline at each step
makes it necessary to track the activated CPUs in a package directly,
because outerwise a CPU offline callback can find CPUs which have already
executed the offline callback, but are not yet marked offline in the
topology mask. That could make such a CPU the package leader and in case
that CPU goes fully offline leave the package lead orphaned.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/powercap/intel_rapl.c |   59 +++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 35 deletions(-)

--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -189,14 +189,13 @@ struct rapl_package {
 	unsigned int time_unit;
 	struct rapl_domain *domains; /* array of domains, sized at runtime */
 	struct powercap_zone *power_zone; /* keep track of parent zone */
-	int nr_cpus; /* active cpus on the package, topology info is lost during
-		      * cpu hotplug. so we have to track ourselves.
-		      */
 	unsigned long power_limit_irq; /* keep track of package power limit
 					* notify interrupt enable status.
 					*/
 	struct list_head plist;
 	int lead_cpu; /* one active cpu per package for access */
+	/* Track active cpus */
+	struct cpumask cpumask;
 };
 
 struct rapl_defaults {
@@ -1432,19 +1431,17 @@ static void rapl_remove_package(struct r
 }
 
 /* called from CPU hotplug notifier, hotplug lock held */
-static int rapl_add_package(int cpu)
+static struct rapl_package *rapl_add_package(int cpu, int pkgid)
 {
 	struct rapl_package *rp;
-	int ret, phy_package_id;
+	int ret;
 
-	phy_package_id = topology_physical_package_id(cpu);
 	rp = kzalloc(sizeof(struct rapl_package), GFP_KERNEL);
 	if (!rp)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	/* add the new package to the list */
-	rp->id = phy_package_id;
-	rp->nr_cpus = 1;
+	rp->id = pkgid;
 	rp->lead_cpu = cpu;
 
 	/* check if the package contains valid domains */
@@ -1457,14 +1454,13 @@ static int rapl_add_package(int cpu)
 	if (!ret) {
 		INIT_LIST_HEAD(&rp->plist);
 		list_add(&rp->plist, &rapl_packages);
-		return 0;
+		return rp;
 	}
 
 err_free_package:
 	kfree(rp->domains);
 	kfree(rp);
-
-	return ret;
+	return ERR_PTR(ret);
 }
 
 /* Handles CPU hotplug on multi-socket systems.
@@ -1476,42 +1472,35 @@ static int rapl_add_package(int cpu)
  */
 static int rapl_cpu_online(unsigned int cpu)
 {
+	int pkgid = topology_physical_package_id(cpu);
 	struct rapl_package *rp;
-	int phy_package_id;
-
-	phy_package_id = topology_physical_package_id(cpu);
 
-	rp = find_package_by_id(phy_package_id);
-	if (rp) {
-		rp->nr_cpus++;
-		return 0;
+	rp = find_package_by_id(pkgid);
+	if (!rp) {
+		rp = rapl_add_package(cpu, pkgid);
+		if (IS_ERR(rp))
+			return PTR_ERR(rp);
 	}
-	return rapl_add_package(cpu);
+	cpumask_set_cpu(cpu, &rp->cpumask);
+	return 0;
 }
 
 static int rapl_cpu_down_prep(unsigned int cpu)
 {
-	int phy_package_id;
+	int pkgid = topology_physical_package_id(cpu);
 	struct rapl_package *rp;
 	int lead_cpu;
 
-	phy_package_id = topology_physical_package_id(cpu);
-	rp = find_package_by_id(phy_package_id);
+	rp = find_package_by_id(pkgid);
 	if (!rp)
 		return 0;
-	if (--rp->nr_cpus == 0) {
+
+	cpumask_clear_cpu(cpu, &rp->cpumask);
+	lead_cpu = cpumask_first(&rp->cpumask);
+	if (lead_cpu >= nr_cpu_ids)
 		rapl_remove_package(rp);
-	} else if (cpu == rp->lead_cpu) {
-		/* choose another active cpu in the package */
-		lead_cpu = cpumask_any_but(topology_core_cpumask(cpu), cpu);
-		if (lead_cpu < nr_cpu_ids) {
-			rp->lead_cpu = lead_cpu;
-		} else {
-			/* should never go here */
-			pr_err("no active cpu available for package %d\n",
-			       phy_package_id);
-		}
-	}
+	else if (rp->lead_cpu == cpu)
+		rp->lead_cpu = lead_cpu;
 	return 0;
 }
 

Re: [patch 0/5] powercap/intel_rapl: Fixes, hotplug conversion and simplifcation

[Jacob Pan]

On Tue, 22 Nov 2016 21:15:56 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> The driver fails to:
> 
>  - initialize packages which are not available at driver init time,
> though the value of that initialization is completely unclear as
> nothing ever uses these values. I fixed it up nevertheless and leave
> it to the maintainers to decide whether it should completely go away.
> 
>  - to propagate error codes in the hotplug online path, where a
>    registration fails and the package data is freed, but return code
> is 0.
> 
> The initialization/removal code of that driver is a maze of
> duplicated code which is more or less the same as the cpu hotplug
> code. After switching over the driver to the hotplug statemachine,
> the whole init/removal machinery can be replaced by
> installing/removing the hotplug state.
> 
> The total damage is:
> 
>  intel_rapl.c |  363
> ++++++++++++++++------------------------------------------- 1 file
> changed, 104 insertions(+), 259 deletions(-)
> 
> and the binary size shrinks as well:
> 
>    text	   data	    bss	    dec	    hex
>    7996	    625	     32	   8653
> 21cd	  Before 7216	    593	     32
> 7841	   1ea1	  After
> 
I have successfully tested this patchset with various cpu online/offline
scenarios on both single and dual socket systems.

Looks good to me. The cpu topology management is much more streamlined.
Thanks. I also sent out this patch below on top of yours.

[PATCH] powercap/intel_rapl: fix and tidy up error handling

Thanks,

Jacob
> Thanks,
> 
> 	tglx
> 
> 
> 
> 

[Jacob Pan]

Re: [patch 0/5] powercap/intel_rapl: Fixes, hotplug conversion and simplifcation

[Rafael J. Wysocki]

On Wed, Nov 23, 2016 at 8:06 PM, Jacob Pan
<jacob.jun.pan@linux.intel.com> wrote:
> On Tue, 22 Nov 2016 21:15:56 -0000
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> The driver fails to:
>>
>>  - initialize packages which are not available at driver init time,
>> though the value of that initialization is completely unclear as
>> nothing ever uses these values. I fixed it up nevertheless and leave
>> it to the maintainers to decide whether it should completely go away.
>>
>>  - to propagate error codes in the hotplug online path, where a
>>    registration fails and the package data is freed, but return code
>> is 0.
>>
>> The initialization/removal code of that driver is a maze of
>> duplicated code which is more or less the same as the cpu hotplug
>> code. After switching over the driver to the hotplug statemachine,
>> the whole init/removal machinery can be replaced by
>> installing/removing the hotplug state.
>>
>> The total damage is:
>>
>>  intel_rapl.c |  363
>> ++++++++++++++++------------------------------------------- 1 file
>> changed, 104 insertions(+), 259 deletions(-)
>>
>> and the binary size shrinks as well:
>>
>>    text          data     bss     dec     hex
>>    7996           625      32    8653
>> 21cd    Before 7216       593      32
>> 7841     1ea1   After
>>
> I have successfully tested this patchset with various cpu online/offline
> scenarios on both single and dual socket systems.
>
> Looks good to me. The cpu topology management is much more streamlined.
> Thanks. I also sent out this patch below on top of yours.
>
> [PATCH] powercap/intel_rapl: fix and tidy up error handling

OK

Thomas, I'm assuming that this series will go in via tip.  Please let
me know if you want me to take it instead.

Thanks,
Rafael

Re: [patch 0/5] powercap/intel_rapl: Fixes, hotplug conversion and simplifcation

[Thomas Gleixner]

Rafael,

On Wed, 23 Nov 2016, Rafael J. Wysocki wrote:
> On Wed, Nov 23, 2016 at 8:06 PM, Jacob Pan
> > Looks good to me. The cpu topology management is much more streamlined.
> > Thanks. I also sent out this patch below on top of yours.
> >
> > [PATCH] powercap/intel_rapl: fix and tidy up error handling
> 
> OK
> 
> Thomas, I'm assuming that this series will go in via tip.  Please let
> me know if you want me to take it instead.

It does not depend on anything in tip, so please route it through your tree.

Thanks,

	tglx

Re: [patch 0/5] powercap/intel_rapl: Fixes, hotplug conversion and simplifcation

[Rafael J. Wysocki]

On Thu, Nov 24, 2016 at 9:33 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Rafael,
>
> On Wed, 23 Nov 2016, Rafael J. Wysocki wrote:
>> On Wed, Nov 23, 2016 at 8:06 PM, Jacob Pan
>> > Looks good to me. The cpu topology management is much more streamlined.
>> > Thanks. I also sent out this patch below on top of yours.
>> >
>> > [PATCH] powercap/intel_rapl: fix and tidy up error handling
>>
>> OK
>>
>> Thomas, I'm assuming that this series will go in via tip.  Please let
>> me know if you want me to take it instead.
>
> It does not depend on anything in tip, so please route it through your tree.

OK, I'm queuing it up, then.

Thanks,
Rafael