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

Groups > linux.kernel > #1527822 > unrolled thread

[patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking

Back to article view | Back to linux.kernel

Contents

  [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking Thomas Gleixner <tglx@linutronix.de> - 2016-11-22 19:10 +0100
    [patch V2 04/12] thermal/x86_pkg_temp: Sanitize callback  (de)initialization Thomas Gleixner <tglx@linutronix.de> - 2016-11-22 19:10 +0100
    Re: [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and  locking "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com> - 2016-11-22 21:00 +0100

#1527822 — [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking

Changes vs. V1: Fix the package removal wreckage reported by Srinivas

We solely intended to convert that driver to the hotplug state machine and
stumbled over a large pile of insanities, which are all interwoven with the
package management:

 - The work cancelation code, the thermal zone unregistering, the work code
   and the interrupt notification function are racy against each other and
   against cpu hotplug and module exit. The random locking sprinkeled all
   over the place does not help anything and probably exists to make people
   feel good. The resulting issues (mainly use after free) are probably
   hard to trigger, but they clearly exist

 - Work cancelation in the cpu hotplug code can leave the work marked
   scheduled and the package interrupts disabled forever.

 - Storage for a boolean information whether work is scheduled for a
   package is kept in separate allocated storage, which is resized when the
   number of detected packages grows.

 - Delayed work structs are held in a static percpu storage, which makes no
   sense at all because work is strictly per package.

 - Packages are kept in a list, which must be searched over and over.

Fixing the whole pile of races with a few duct tape fixes was pretty much
impossible, so I decided to do a major rewrite to fix all of the
above. Here are the major changes:

 - Rewrite the offline code with proper locking against interrupts and work
   function and make sure that canceled work is rescheduled if there is
   another online cpu in the package.

 - Use the cpu offline callback on module exit to fix the work cancelation
   race.

 - Move the bool which denotes scheduled work into the package struct
   where it belongs.

 - Move the delayed work struct into the package struct, which is the only
   sensible place to have it and schedule the work on the cpu which is the
   target for the sysfs files as this makes the cancellation and
   rescheduling in the cpu offline path simple.

 - Add a large pile of comments documenting the cpu teardown mechanism

 - Code sanitizing, revamp the horrible name choices plus a general coding
   style cleanup.

   Note, that I did the namespace and code cleanup in the middle of the
   series, because staring at that mess just made my eyes bleeding.

 - Store the package pointers in an array which is allocated at init
   time. Sizing of the array is determined from the topology
   information. That makes the package lookup a simple array lookup.

As a last step the driver is converted to the hotplug state machine.

Thanks,
	
	tglx
---
 x86_pkg_temp_thermal.c |  593 ++++++++++++++++++++-----------------------------
 1 file changed, 249 insertions(+), 344 deletions(-)

[toc] | [next] | [standalone]

#1527823 — [patch V2 04/12] thermal/x86_pkg_temp: Sanitize callback (de)initialization

The threshold callbacks are installed before the initialization of the
online cpus has succeeded and removed after the teardown has been
done. That's both wrong as callbacks might be invoked into a half
initialized or torn down state.

Move them to the proper places: Last in init() and first in exit().

While at it shorten the insane long and horrible named function names.

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

---
 drivers/thermal/x86_pkg_temp_thermal.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -281,7 +281,7 @@ static struct thermal_zone_device_ops tz
 	.set_trip_temp = sys_set_trip_temp,
 };
 
-static bool pkg_temp_thermal_platform_thermal_rate_control(void)
+static bool pkg_thermal_rate_control(void)
 {
 	return true;
 }
@@ -355,7 +355,7 @@ static void pkg_temp_thermal_threshold_w
 	}
 }
 
-static int pkg_temp_thermal_platform_thermal_notify(__u64 msr_val)
+static int pkg_thermal_notify(__u64 msr_val)
 {
 	unsigned long flags;
 	int cpu = smp_processor_id();
@@ -579,10 +579,6 @@ static int __init pkg_temp_thermal_init(
 		return -ENODEV;
 
 	spin_lock_init(&pkg_work_lock);
-	platform_thermal_package_notify =
-			pkg_temp_thermal_platform_thermal_notify;
-	platform_thermal_package_rate_control =
-			pkg_temp_thermal_platform_thermal_rate_control;
 
 	cpu_notifier_register_begin();
 	for_each_online_cpu(i)
@@ -591,6 +587,9 @@ static int __init pkg_temp_thermal_init(
 	__register_hotcpu_notifier(&pkg_temp_thermal_notifier);
 	cpu_notifier_register_done();
 
+	platform_thermal_package_notify = pkg_thermal_notify;
+	platform_thermal_package_rate_control = pkg_thermal_rate_control;
+
 	pkg_temp_debugfs_init(); /* Don't care if fails */
 
 	return 0;
@@ -600,9 +599,6 @@ static int __init pkg_temp_thermal_init(
 		put_core_offline(i);
 	cpu_notifier_register_done();
 	kfree(pkg_work_scheduled);
-	platform_thermal_package_notify = NULL;
-	platform_thermal_package_rate_control = NULL;
-
 	return -ENODEV;
 }
 
@@ -611,6 +607,9 @@ static void __exit pkg_temp_thermal_exit
 	struct phy_dev_entry *phdev, *n;
 	int i;
 
+	platform_thermal_package_notify = NULL;
+	platform_thermal_package_rate_control = NULL;
+
 	cpu_notifier_register_begin();
 	__unregister_hotcpu_notifier(&pkg_temp_thermal_notifier);
 	mutex_lock(&phy_dev_list_mutex);
@@ -625,8 +624,6 @@ static void __exit pkg_temp_thermal_exit
 		kfree(phdev);
 	}
 	mutex_unlock(&phy_dev_list_mutex);
-	platform_thermal_package_notify = NULL;
-	platform_thermal_package_rate_control = NULL;
 	for_each_online_cpu(i)
 		cancel_delayed_work_sync(
 			&per_cpu(pkg_temp_thermal_threshold_work, i));

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

#1527884 — Re: [patch V2 00/12] thermal/x86_pkg_temp: Sanitize hotplug and locking

On Tue, 2016-11-22 at 17:57 +0000, Thomas Gleixner wrote:
> Changes vs. V1: Fix the package removal wreckage reported by Srinivas
> 

I haven't looked at individual patch but tested the series as a whole. 

So Rui, you can add

Tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web