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

[Lukasz Luba <lukasz.luba@arm.com>]

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>