Re: [PATCH v1 13/16] thermal: gov_fair_share: Eliminate unnecessary integer divisions

Lukasz Luba
2024-04-19T18:45:56Z

On 4/10/24 18:00, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The computations carried out by fair_share_throttle() for each trip
> point include at least one redundant integer division which introduces
> superfluous rounding errors. Also the multiplications by 100 in it are
> not really necessary and can be eliminated.
>
> Rearrange fair_share_throttle() to carry out only one integer division per
> trip and only as many integer multiplications as necessary and rename one
> variable in it (while at it).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/thermal/gov_fair_share.c | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> Index: linux-pm/drivers/thermal/gov_fair_share.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_fair_share.c
> +++ linux-pm/drivers/thermal/gov_fair_share.c
> @@ -41,12 +41,6 @@ static int get_trip_level(struct thermal
> return trip_level;
> }
>
> -static long get_target_state(struct thermal_zone_device *tz,
> - struct thermal_cooling_device *cdev, int percentage, int level)
> -{
> - return (long)(percentage * level * cdev->max_state) / (100 * tz->num_trips);
> -}
> -
> /**
> * fair_share_throttle - throttles devices associated with the given zone
> * @tz: thermal_zone_device
> @@ -58,7 +52,7 @@ static long get_target_state(struct ther
> *
> * Parameters used for Throttling:
> * P1. max_state: Maximum throttle state exposed by the cooling device.
> - * P2. percentage[i]/100:
> + * P2. weight[i]/total_weight:
> * How 'effective' the 'i'th device is, in cooling the given zone.
> * P3. trip_level/max_no_of_trips:
> * This describes the extent to which the devices should be throttled.
> @@ -72,30 +66,34 @@ static void fair_share_throttle(struct t
> {
> struct thermal_instance *instance;
> int total_weight = 0;
> - int total_instance = 0;
> + int nr_instances = 0;
>
> list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> if (instance->trip != trip)
> continue;
>
> total_weight += instance->weight;
> - total_instance++;
> + nr_instances++;
> }
>
> list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
> - int percentage;
> struct thermal_cooling_device *cdev = instance->cdev;
> + u64 dividend;
> + u32 divisor;
>
> if (instance->trip != trip)
> continue;
>
> - if (!total_weight)
> - percentage = 100 / total_instance;
> - else
> - percentage = (instance->weight * 100) / total_weight;
> -
> - instance->target = get_target_state(tz, cdev, percentage,
> - trip_level);
> + dividend = trip_level;
> + dividend *= cdev->max_state;
> + divisor = tz->num_trips;
> + if (total_weight) {
> + dividend *= instance->weight;
> + divisor *= total_weight;
> + } else {
> + divisor *= nr_instances;
> + }
> + instance->target = div_u64(dividend, divisor);
>
> mutex_lock(&cdev->lock);
> __thermal_cdev_update(cdev);
>
>
>

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