[PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later

Posted by Rafael J. Wysocki 1 month, 1 week ago

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since it is not expected that multiple trip points will be crossed
in one go very often (if this happens, there are too many trip points
in the given thermal zone or they are checked too rarely), quite likely
it is more efficient to build a sorted list of crossed trip points than
to put them on an unsorted list and sort it later.

Moreover, trip points are often sorted in ascending temperature order
during thermal zone registration. so building a sorted list out of
them is quite straightforward and relatively inexpensive.

Accordingly, make handle_thermal_trip() maintain list ordering when
adding trip points to the lists and get rid of separate list sorting
in __thermal_zone_device_update().

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -15,7 +15,6 @@
 #include <linux/slab.h>
 #include <linux/kdev_t.h>
 #include <linux/idr.h>
-#include <linux/list_sort.h>
 #include <linux/thermal.h>
 #include <linux/reboot.h>
 #include <linux/string.h>
@@ -409,6 +408,21 @@ static void handle_critical_trips(struct
 		tz->ops.hot(tz);
 }
 
+static void add_trip_to_sorted_list(struct thermal_trip_desc *td,
+				    struct list_head *list)
+{
+	struct thermal_trip_desc *entry;
+
+	/* Assume that the new entry is likely to be the last one. */
+	list_for_each_entry_reverse(entry, list, notify_list_node) {
+		if (entry->notify_temp <= td->notify_temp) {
+			list_add(&td->notify_list_node, &entry->notify_list_node);
+			return;
+		}
+	}
+	list_add(&td->notify_list_node, list);
+}
+
 static void handle_thermal_trip(struct thermal_zone_device *tz,
 				struct thermal_trip_desc *td,
 				struct list_head *way_up_list,
@@ -438,8 +452,8 @@ static void handle_thermal_trip(struct t
 		 * In that case, the trip temperature becomes the new threshold.
 		 */
 		if (tz->temperature < trip->temperature - trip->hysteresis) {
-			list_add(&td->notify_list_node, way_down_list);
 			td->notify_temp = trip->temperature - trip->hysteresis;
+			add_trip_to_sorted_list(td, way_down_list);
 
 			if (trip->type == THERMAL_TRIP_PASSIVE) {
 				tz->passive--;
@@ -454,8 +468,9 @@ static void handle_thermal_trip(struct t
 		 * if the zone temperature exceeds the trip one.  The new
 		 * threshold is then set to the low temperature of the trip.
 		 */
-		list_add_tail(&td->notify_list_node, way_up_list);
 		td->notify_temp = trip->temperature;
+		add_trip_to_sorted_list(td, way_up_list);
+
 		td->threshold -= trip->hysteresis;
 
 		if (trip->type == THERMAL_TRIP_PASSIVE)
@@ -519,16 +534,6 @@ static void thermal_trip_crossed(struct
 	thermal_governor_trip_crossed(governor, tz, trip, crossed_up);
 }
 
-static int thermal_trip_notify_cmp(void *not_used, const struct list_head *a,
-				   const struct list_head *b)
-{
-	struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
-						     notify_list_node);
-	struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
-						     notify_list_node);
-	return tda->notify_temp - tdb->notify_temp;
-}
-
 void __thermal_zone_device_update(struct thermal_zone_device *tz,
 				  enum thermal_notify_event event)
 {
@@ -581,11 +586,9 @@ void __thermal_zone_device_update(struct
 
 	thermal_zone_set_trips(tz, low, high);
 
-	list_sort(NULL, &way_up_list, thermal_trip_notify_cmp);
 	list_for_each_entry(td, &way_up_list, notify_list_node)
 		thermal_trip_crossed(tz, &td->trip, governor, true);
 
-	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
 	list_for_each_entry_reverse(td, &way_down_list, notify_list_node)
 		thermal_trip_crossed(tz, &td->trip, governor, false);

Re: [PATCH v1 01/10] thermal: core: Build sorted lists instead of sorting them later

Posted by Lukasz Luba 1 month ago


On 10/16/24 12:21, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since it is not expected that multiple trip points will be crossed
> in one go very often (if this happens, there are too many trip points
> in the given thermal zone or they are checked too rarely), quite likely
> it is more efficient to build a sorted list of crossed trip points than
> to put them on an unsorted list and sort it later.
> 
> Moreover, trip points are often sorted in ascending temperature order
> during thermal zone registration. so building a sorted list out of

nit: small s

> them is quite straightforward and relatively inexpensive.
> 
> Accordingly, make handle_thermal_trip() maintain list ordering when
> adding trip points to the lists and get rid of separate list sorting
> in __thermal_zone_device_update().
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_core.c |   33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -15,7 +15,6 @@
>   #include <linux/slab.h>
>   #include <linux/kdev_t.h>
>   #include <linux/idr.h>
> -#include <linux/list_sort.h>
>   #include <linux/thermal.h>
>   #include <linux/reboot.h>
>   #include <linux/string.h>
> @@ -409,6 +408,21 @@ static void handle_critical_trips(struct
>   		tz->ops.hot(tz);
>   }
>   
> +static void add_trip_to_sorted_list(struct thermal_trip_desc *td,
> +				    struct list_head *list)
> +{
> +	struct thermal_trip_desc *entry;
> +
> +	/* Assume that the new entry is likely to be the last one. */
> +	list_for_each_entry_reverse(entry, list, notify_list_node) {
> +		if (entry->notify_temp <= td->notify_temp) {
> +			list_add(&td->notify_list_node, &entry->notify_list_node);
> +			return;
> +		}
> +	}
> +	list_add(&td->notify_list_node, list);
> +}
> +
>   static void handle_thermal_trip(struct thermal_zone_device *tz,
>   				struct thermal_trip_desc *td,
>   				struct list_head *way_up_list,
> @@ -438,8 +452,8 @@ static void handle_thermal_trip(struct t
>   		 * In that case, the trip temperature becomes the new threshold.
>   		 */
>   		if (tz->temperature < trip->temperature - trip->hysteresis) {
> -			list_add(&td->notify_list_node, way_down_list);
>   			td->notify_temp = trip->temperature - trip->hysteresis;
> +			add_trip_to_sorted_list(td, way_down_list);
>   
>   			if (trip->type == THERMAL_TRIP_PASSIVE) {
>   				tz->passive--;
> @@ -454,8 +468,9 @@ static void handle_thermal_trip(struct t
>   		 * if the zone temperature exceeds the trip one.  The new
>   		 * threshold is then set to the low temperature of the trip.
>   		 */
> -		list_add_tail(&td->notify_list_node, way_up_list);
>   		td->notify_temp = trip->temperature;
> +		add_trip_to_sorted_list(td, way_up_list);
> +
>   		td->threshold -= trip->hysteresis;
>   
>   		if (trip->type == THERMAL_TRIP_PASSIVE)
> @@ -519,16 +534,6 @@ static void thermal_trip_crossed(struct
>   	thermal_governor_trip_crossed(governor, tz, trip, crossed_up);
>   }
>   
> -static int thermal_trip_notify_cmp(void *not_used, const struct list_head *a,
> -				   const struct list_head *b)
> -{
> -	struct thermal_trip_desc *tda = container_of(a, struct thermal_trip_desc,
> -						     notify_list_node);
> -	struct thermal_trip_desc *tdb = container_of(b, struct thermal_trip_desc,
> -						     notify_list_node);
> -	return tda->notify_temp - tdb->notify_temp;
> -}
> -
>   void __thermal_zone_device_update(struct thermal_zone_device *tz,
>   				  enum thermal_notify_event event)
>   {
> @@ -581,11 +586,9 @@ void __thermal_zone_device_update(struct
>   
>   	thermal_zone_set_trips(tz, low, high);
>   
> -	list_sort(NULL, &way_up_list, thermal_trip_notify_cmp);
>   	list_for_each_entry(td, &way_up_list, notify_list_node)
>   		thermal_trip_crossed(tz, &td->trip, governor, true);
>   
> -	list_sort(NULL, &way_down_list, thermal_trip_notify_cmp);
>   	list_for_each_entry_reverse(td, &way_down_list, notify_list_node)
>   		thermal_trip_crossed(tz, &td->trip, governor, false);
>   
> 
> 
> 

Make sense, LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>