[v1,07/13] thermal: gov_power_allocator: Use trip pointers instead of trip indices

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

Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
overhead (due to pointless bounds checking and copying of trip point
data) from the power allocator thermal governor and generally make it
use trip pointers instead of trip indices where applicable.

The general functionality is not expected to be changed.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_power_allocator.c |  102 ++++++++++++----------------------
 1 file changed, 38 insertions(+), 64 deletions(-)
  

On 21/09/2023 19:55, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
> overhead (due to pointless bounds checking and copying of trip point
> data) from the power allocator thermal governor and generally make it
> use trip pointers instead of trip indices where applicable.

Actually the __thermal_zone_get_trip() change was done on purpose to 
replace the 'throttle' callback index parameter by the trip pointer and 
removing those call to __thermal_zone_get_trip() while the code was 
using the trip pointer.

IMO, the changes should focus on changing the trip_index parameter by 
the trip pointer directly in the throttle ops. The pointer can be 
retrieved in the handle_thermal_trip() function and passed around for 
the rest of the actions on this trip point

> The general functionality is not expected to be changed.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_power_allocator.c |  102 ++++++++++++----------------------
>   1 file changed, 38 insertions(+), 64 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -16,8 +16,6 @@
>   
>   #include "thermal_core.h"
>   
> -#define INVALID_TRIP -1
> -
>   #define FRAC_BITS 10
>   #define int_to_frac(x) ((x) << FRAC_BITS)
>   #define frac_to_int(x) ((x) >> FRAC_BITS)
> @@ -55,23 +53,23 @@ static inline s64 div_frac(s64 x, s64 y)
>    * @err_integral:	accumulated error in the PID controller.
>    * @prev_err:	error in the previous iteration of the PID controller.
>    *		Used to calculate the derivative term.
> + * @sustainable_power:	Sustainable power (heat) that this thermal zone can
> + *			dissipate
>    * @trip_switch_on:	first passive trip point of the thermal zone.  The
>    *			governor switches on when this trip point is crossed.
>    *			If the thermal zone only has one passive trip point,
> - *			@trip_switch_on should be INVALID_TRIP.
> + *			@trip_switch_on should be NULL.
>    * @trip_max_desired_temperature:	last passive trip point of the thermal
>    *					zone.  The temperature we are
>    *					controlling for.
> - * @sustainable_power:	Sustainable power (heat) that this thermal zone can
> - *			dissipate
>    */
>   struct power_allocator_params {
>   	bool allocated_tzp;
>   	s64 err_integral;
>   	s32 prev_err;
> -	int trip_switch_on;
> -	int trip_max_desired_temperature;
>   	u32 sustainable_power;
> +	const struct thermal_trip *trip_switch_on;
> +	const struct thermal_trip *trip_max_desired_temperature;
>   };
>   
>   /**
> @@ -90,14 +88,12 @@ static u32 estimate_sustainable_power(st
>   	u32 sustainable_power = 0;
>   	struct thermal_instance *instance;
>   	struct power_allocator_params *params = tz->governor_data;
> -	const struct thermal_trip *trip_max_desired_temperature =
> -			&tz->trips[params->trip_max_desired_temperature];
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		struct thermal_cooling_device *cdev = instance->cdev;
>   		u32 min_power;
>   
> -		if (instance->trip != trip_max_desired_temperature)
> +		if (instance->trip != params->trip_max_desired_temperature)
>   			continue;
>   
>   		if (!cdev_is_power_actor(cdev))
> @@ -116,24 +112,23 @@ static u32 estimate_sustainable_power(st
>    * estimate_pid_constants() - Estimate the constants for the PID controller
>    * @tz:		thermal zone for which to estimate the constants
>    * @sustainable_power:	sustainable power for the thermal zone
> - * @trip_switch_on:	trip point number for the switch on temperature
> + * @trip_switch_on:	trip point for the switch on temperature
>    * @control_temp:	target temperature for the power allocator governor
>    *
>    * This function is used to update the estimation of the PID
>    * controller constants in struct thermal_zone_parameters.
>    */
>   static void estimate_pid_constants(struct thermal_zone_device *tz,
> -				   u32 sustainable_power, int trip_switch_on,
> +				   u32 sustainable_power,
> +				   const struct thermal_trip *trip_switch_on,
>   				   int control_temp)
>   {
> -	struct thermal_trip trip;
>   	u32 temperature_threshold = control_temp;
>   	int ret;
>   	s32 k_i;
>   
> -	ret = __thermal_zone_get_trip(tz, trip_switch_on, &trip);
> -	if (!ret)
> -		temperature_threshold -= trip.temperature;
> +	if (trip_switch_on)
> +		temperature_threshold -= trip_switch_on->temperature;
>   
>   	/*
>   	 * estimate_pid_constants() tries to find appropriate default
> @@ -386,7 +381,7 @@ static int allocate_power(struct thermal
>   	struct thermal_instance *instance;
>   	struct power_allocator_params *params = tz->governor_data;
>   	const struct thermal_trip *trip_max_desired_temperature =
> -			&tz->trips[params->trip_max_desired_temperature];
> +					params->trip_max_desired_temperature;
>   	u32 *req_power, *max_power, *granted_power, *extra_actor_power;
>   	u32 *weighted_req_power;
>   	u32 total_req_power, max_allocatable_power, total_weighted_req_power;
> @@ -513,46 +508,35 @@ static int allocate_power(struct thermal
>   static void get_governor_trips(struct thermal_zone_device *tz,
>   			       struct power_allocator_params *params)
>   {
> -	int i, last_active, last_passive;
> -	bool found_first_passive;
> -
> -	found_first_passive = false;
> -	last_active = INVALID_TRIP;
> -	last_passive = INVALID_TRIP;
> +	const struct thermal_trip *last_active = NULL:
> +	const struct thermal_trip *last_passive = NULL;
> +	bool found_first_passive = false;
> +	int i;
>   
>   	for (i = 0; i < tz->num_trips; i++) {
> -		struct thermal_trip trip;
> -		int ret;
> +		const struct thermal_trip *trip = &tz->trips[i];
>   
> -		ret = __thermal_zone_get_trip(tz, i, &trip);
> -		if (ret) {
> -			dev_warn(&tz->device,
> -				 "Failed to get trip point %d type: %d\n", i,
> -				 ret);
> -			continue;
> -		}
> -
> -		if (trip.type == THERMAL_TRIP_PASSIVE) {
> +		if (trip->type == THERMAL_TRIP_PASSIVE) {
>   			if (!found_first_passive) {
> -				params->trip_switch_on = i;
> +				params->trip_switch_on = trip;
>   				found_first_passive = true;
>   			} else  {
> -				last_passive = i;
> +				last_passive = trip;
>   			}
> -		} else if (trip.type == THERMAL_TRIP_ACTIVE) {
> -			last_active = i;
> +		} else if (trip->type == THERMAL_TRIP_ACTIVE) {
> +			last_active = trip;
>   		} else {
>   			break;
>   		}
>   	}
>   
> -	if (last_passive != INVALID_TRIP) {
> +	if (last_passive) {
>   		params->trip_max_desired_temperature = last_passive;
>   	} else if (found_first_passive) {
>   		params->trip_max_desired_temperature = params->trip_switch_on;
> -		params->trip_switch_on = INVALID_TRIP;
> +		params->trip_switch_on = NULL;
>   	} else {
> -		params->trip_switch_on = INVALID_TRIP;
> +		params->trip_switch_on = NULL;
>   		params->trip_max_desired_temperature = last_active;
>   	}
>   }
> @@ -567,14 +551,12 @@ static void allow_maximum_power(struct t
>   {
>   	struct thermal_instance *instance;
>   	struct power_allocator_params *params = tz->governor_data;
> -	const struct thermal_trip *trip_max_desired_temperature =
> -			&tz->trips[params->trip_max_desired_temperature];
>   	u32 req_power;
>   
>   	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
>   		struct thermal_cooling_device *cdev = instance->cdev;
>   
> -		if ((instance->trip != trip_max_desired_temperature) ||
> +		if (instance->trip != params->trip_max_desired_temperature ||
>   		    (!cdev_is_power_actor(instance->cdev)))
>   			continue;
>   
> @@ -636,7 +618,6 @@ static int power_allocator_bind(struct t
>   {
>   	int ret;
>   	struct power_allocator_params *params;
> -	struct thermal_trip trip;
>   
>   	ret = check_power_actors(tz);
>   	if (ret)
> @@ -662,12 +643,13 @@ static int power_allocator_bind(struct t
>   	get_governor_trips(tz, params);
>   
>   	if (tz->num_trips > 0) {
> -		ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> -					      &trip);
> -		if (!ret)
> +		const struct thermal_trip *trip;
> +
> +		trip = params->trip_max_desired_temperature;
> +		if (trip)
>   			estimate_pid_constants(tz, tz->tzp->sustainable_power,
>   					       params->trip_switch_on,
> -					       trip.temperature);
> +					       trip->temperature);
>   	}
>   
>   	reset_pid_controller(params);
> @@ -697,11 +679,10 @@ static void power_allocator_unbind(struc
>   	tz->governor_data = NULL;
>   }
>   
> -static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> +static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_index)
>   {
>   	struct power_allocator_params *params = tz->governor_data;
> -	struct thermal_trip trip;
> -	int ret;
> +	const struct thermal_trip *trip = &tz->trips[trip_index];
>   	bool update;
>   
>   	lockdep_assert_held(&tz->lock);
> @@ -710,12 +691,12 @@ static int power_allocator_throttle(stru
>   	 * We get called for every trip point but we only need to do
>   	 * our calculations once
>   	 */
> -	if (trip_id != params->trip_max_desired_temperature)
> +	if (trip != params->trip_max_desired_temperature)
>   		return 0;
>   
> -	ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
> -	if (!ret && (tz->temperature < trip.temperature)) {
> -		update = (tz->last_temperature >= trip.temperature);
> +	trip = params->trip_switch_on;
> +	if (trip && tz->temperature < trip->temperature) {
> +		update = tz->last_temperature >= trip->temperature;
>   		tz->passive = 0;
>   		reset_pid_controller(params);
>   		allow_maximum_power(tz, update);
> @@ -724,14 +705,7 @@ static int power_allocator_throttle(stru
>   
>   	tz->passive = 1;
>   
> -	ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature, &trip);
> -	if (ret) {
> -		dev_warn(&tz->device, "Failed to get the maximum desired temperature: %d\n",
> -			 ret);
> -		return ret;
> -	}
> -
> -	return allocate_power(tz, trip.temperature);
> +	return allocate_power(tz, params->trip_max_desired_temperature->temperature);
>   }
>   
>   static struct thermal_governor thermal_gov_power_allocator = {
> 
> 
>
  

On Wed, Sep 27, 2023 at 5:10 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 21/09/2023 19:55, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
> > overhead (due to pointless bounds checking and copying of trip point
> > data) from the power allocator thermal governor and generally make it
> > use trip pointers instead of trip indices where applicable.
>
> Actually the __thermal_zone_get_trip() change was done on purpose to
> replace the 'throttle' callback index parameter by the trip pointer and
> removing those call to __thermal_zone_get_trip() while the code was
> using the trip pointer.
>
> IMO, the changes should focus on changing the trip_index parameter by
> the trip pointer directly in the throttle ops.

So you would like .throttle() to take a trip pointer argument instead
of an index?

The difficulty here is that the user space governor needs to expose
the index to user space anyway, so it would need to find it if it gets
a trip pointer instead.

Not a big deal I suppose, but a bit of extra overhead.

Also it is easier to switch the governors over to using trip pointers
internally and then change the .throttle() argument on top of that.

> The pointer can be
> retrieved in the handle_thermal_trip() function and passed around for
> the rest of the actions on this trip point

Right, except for the user space governor which needs a trip index.
And the indices are used for tracing too.
  

On 27/09/2023 17:27, Rafael J. Wysocki wrote:
> On Wed, Sep 27, 2023 at 5:10 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 21/09/2023 19:55, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
>>> overhead (due to pointless bounds checking and copying of trip point
>>> data) from the power allocator thermal governor and generally make it
>>> use trip pointers instead of trip indices where applicable.
>>
>> Actually the __thermal_zone_get_trip() change was done on purpose to
>> replace the 'throttle' callback index parameter by the trip pointer and
>> removing those call to __thermal_zone_get_trip() while the code was
>> using the trip pointer.
>>
>> IMO, the changes should focus on changing the trip_index parameter by
>> the trip pointer directly in the throttle ops.
> 
> So you would like .throttle() to take a trip pointer argument instead
> of an index?
> 
> The difficulty here is that the user space governor needs to expose
> the index to user space anyway, so it would need to find it if it gets
> a trip pointer instead.
> 
> Not a big deal I suppose, but a bit of extra overhead.
> 
> Also it is easier to switch the governors over to using trip pointers
> internally and then change the .throttle() argument on top of that.
> 
>> The pointer can be
>> retrieved in the handle_thermal_trip() function and passed around for
>> the rest of the actions on this trip point
> 
> Right, except for the user space governor which needs a trip index.
> And the indices are used for tracing too.

Given the userspace governor is going obsolete and the notifications are 
for the userspace, which is slow, we can retrieve the index from the 
throttling ops
  

On Wed, Sep 27, 2023 at 5:46 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 27/09/2023 17:27, Rafael J. Wysocki wrote:
> > On Wed, Sep 27, 2023 at 5:10 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 21/09/2023 19:55, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Eliminate the __thermal_zone_get_trip() usage that adds unnecessary
> >>> overhead (due to pointless bounds checking and copying of trip point
> >>> data) from the power allocator thermal governor and generally make it
> >>> use trip pointers instead of trip indices where applicable.
> >>
> >> Actually the __thermal_zone_get_trip() change was done on purpose to
> >> replace the 'throttle' callback index parameter by the trip pointer and
> >> removing those call to __thermal_zone_get_trip() while the code was
> >> using the trip pointer.
> >>
> >> IMO, the changes should focus on changing the trip_index parameter by
> >> the trip pointer directly in the throttle ops.
> >
> > So you would like .throttle() to take a trip pointer argument instead
> > of an index?
> >
> > The difficulty here is that the user space governor needs to expose
> > the index to user space anyway, so it would need to find it if it gets
> > a trip pointer instead.
> >
> > Not a big deal I suppose, but a bit of extra overhead.
> >
> > Also it is easier to switch the governors over to using trip pointers
> > internally and then change the .throttle() argument on top of that.
> >
> >> The pointer can be
> >> retrieved in the handle_thermal_trip() function and passed around for
> >> the rest of the actions on this trip point
> >
> > Right, except for the user space governor which needs a trip index.
> > And the indices are used for tracing too.
>
> Given the userspace governor is going obsolete and the notifications are
> for the userspace, which is slow, we can retrieve the index from the
> throttling ops

OK

Given that patches [01-05/13] are not controversial, I'll respon the
governor patches into a separate series on top of them.

I would much appreciate it if you could take a look at patch [10/13]
and the remaining ACPi thermal patches in this series [11-13/13].
They don't depend on the governor changes.