diff mbox series

[v1,06/13] thermal: gov_fair_share: Rearrange get_trip_level()

Message ID 1882755.CQOukoFCf9@kreacher (mailing list archive)
State Changes Requested, archived
Headers show
Series thermal: ACPI: More ACPI thermal improvements and modification of thermal instances | expand

Commit Message

Rafael J. Wysocki Sept. 21, 2023, 5:54 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make get_trip_level() access the thermal zone's trip table directly
instead of using __thermal_zone_get_trip() which adds overhead related
to the unnecessary bounds checking and copying the trip point data.

Also rearrange the code in it to make it somewhat easier to follow.

The general functionality is not expected to be changed.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/gov_fair_share.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Daniel Lezcano Sept. 27, 2023, 3 p.m. UTC | #1 
On 21/09/2023 19:54, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make get_trip_level() access the thermal zone's trip table directly
> instead of using __thermal_zone_get_trip() which adds overhead related
> to the unnecessary bounds checking and copying the trip point data.
> 
> Also rearrange the code in it to make it somewhat easier to follow.
> 
> The general functionality is not expected to be changed.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/gov_fair_share.c |   22 ++++++++++------------
>   1 file changed, 10 insertions(+), 12 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
> @@ -21,23 +21,21 @@
>    */
>   static int get_trip_level(struct thermal_zone_device *tz)
>   {
> -	struct thermal_trip trip;
> -	int count;
> +	const struct thermal_trip *trip = tz->trips;
> +	int i;
>   
> -	for (count = 0; count < tz->num_trips; count++) {
> -		__thermal_zone_get_trip(tz, count, &trip);
> -		if (tz->temperature < trip.temperature)
> +	if (tz->temperature < trip->temperature)
> +		return 0;
> +
> +	for (i = 0; i < tz->num_trips - 1; i++) {
> +		trip++;
> +		if (tz->temperature < trip->temperature)
>   			break;
>   	}

Is it possible to use for_each_thermal_trip() instead ? That would make 
the code more self-encapsulate

> -	/*
> -	 * count > 0 only if temperature is greater than first trip
> -	 * point, in which case, trip_point = count - 1
> -	 */
> -	if (count > 0)
> -		trace_thermal_zone_trip(tz, count - 1, trip.type);
> +	trace_thermal_zone_trip(tz, i, tz->trips[i].type);
>   
> -	return count;
> +	return i;
>   }
>   
>   static long get_target_state(struct thermal_zone_device *tz,
> 
> 
>
Rafael J. Wysocki Sept. 27, 2023, 3:06 p.m. UTC | #2 
On Wed, Sep 27, 2023 at 5:00 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 21/09/2023 19:54, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Make get_trip_level() access the thermal zone's trip table directly
> > instead of using __thermal_zone_get_trip() which adds overhead related
> > to the unnecessary bounds checking and copying the trip point data.
> >
> > Also rearrange the code in it to make it somewhat easier to follow.
> >
> > The general functionality is not expected to be changed.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/gov_fair_share.c |   22 ++++++++++------------
> >   1 file changed, 10 insertions(+), 12 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
> > @@ -21,23 +21,21 @@
> >    */
> >   static int get_trip_level(struct thermal_zone_device *tz)
> >   {
> > -     struct thermal_trip trip;
> > -     int count;
> > +     const struct thermal_trip *trip = tz->trips;
> > +     int i;
> >
> > -     for (count = 0; count < tz->num_trips; count++) {
> > -             __thermal_zone_get_trip(tz, count, &trip);
> > -             if (tz->temperature < trip.temperature)
> > +     if (tz->temperature < trip->temperature)
> > +             return 0;
> > +
> > +     for (i = 0; i < tz->num_trips - 1; i++) {
> > +             trip++;
> > +             if (tz->temperature < trip->temperature)
> >                       break;
> >       }
>
> Is it possible to use for_each_thermal_trip() instead ? That would make
> the code more self-encapsulate

It is possible in principle, but this is a governor which is regarded
as part of the core, isn't it?

So is an extra overhead related to using a callback (which may be
subject to retpolines and such) really justified in this case?

>
> > -     /*
> > -      * count > 0 only if temperature is greater than first trip
> > -      * point, in which case, trip_point = count - 1
> > -      */
> > -     if (count > 0)
> > -             trace_thermal_zone_trip(tz, count - 1, trip.type);
> > +     trace_thermal_zone_trip(tz, i, tz->trips[i].type);
> >
> > -     return count;
> > +     return i;
> >   }
> >
> >   static long get_target_state(struct thermal_zone_device *tz,
> >
> >
> >
>
> --
Daniel Lezcano Sept. 27, 2023, 3:37 p.m. UTC | #3 
On 27/09/2023 17:06, Rafael J. Wysocki wrote:
> On Wed, Sep 27, 2023 at 5:00 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 21/09/2023 19:54, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Make get_trip_level() access the thermal zone's trip table directly
>>> instead of using __thermal_zone_get_trip() which adds overhead related
>>> to the unnecessary bounds checking and copying the trip point data.
>>>
>>> Also rearrange the code in it to make it somewhat easier to follow.
>>>
>>> The general functionality is not expected to be changed.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>    drivers/thermal/gov_fair_share.c |   22 ++++++++++------------
>>>    1 file changed, 10 insertions(+), 12 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
>>> @@ -21,23 +21,21 @@
>>>     */
>>>    static int get_trip_level(struct thermal_zone_device *tz)
>>>    {
>>> -     struct thermal_trip trip;
>>> -     int count;
>>> +     const struct thermal_trip *trip = tz->trips;
>>> +     int i;
>>>
>>> -     for (count = 0; count < tz->num_trips; count++) {
>>> -             __thermal_zone_get_trip(tz, count, &trip);
>>> -             if (tz->temperature < trip.temperature)
>>> +     if (tz->temperature < trip->temperature)
>>> +             return 0;
>>> +
>>> +     for (i = 0; i < tz->num_trips - 1; i++) {
>>> +             trip++;
>>> +             if (tz->temperature < trip->temperature)
>>>                        break;
>>>        }
>>
>> Is it possible to use for_each_thermal_trip() instead ? That would make
>> the code more self-encapsulate
> 
> It is possible in principle, but this is a governor which is regarded
> as part of the core, isn't it?
> 
> So is an extra overhead related to using a callback (which may be
> subject to retpolines and such) really justified in this case?

 From my POV, all trip points browsing should be replaced by 
for_each_thermal_trip() so any change in the future in how we go through 
the existing thermal trips will impact one place.

If the routine needs to be optimized, that is something we can do also 
(may be an inline the callback?)
Rafael J. Wysocki Sept. 27, 2023, 4:09 p.m. UTC | #4 
On Wed, Sep 27, 2023 at 5:37 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 27/09/2023 17:06, Rafael J. Wysocki wrote:
> > On Wed, Sep 27, 2023 at 5:00 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 21/09/2023 19:54, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Make get_trip_level() access the thermal zone's trip table directly
> >>> instead of using __thermal_zone_get_trip() which adds overhead related
> >>> to the unnecessary bounds checking and copying the trip point data.
> >>>
> >>> Also rearrange the code in it to make it somewhat easier to follow.
> >>>
> >>> The general functionality is not expected to be changed.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>>    drivers/thermal/gov_fair_share.c |   22 ++++++++++------------
> >>>    1 file changed, 10 insertions(+), 12 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
> >>> @@ -21,23 +21,21 @@
> >>>     */
> >>>    static int get_trip_level(struct thermal_zone_device *tz)
> >>>    {
> >>> -     struct thermal_trip trip;
> >>> -     int count;
> >>> +     const struct thermal_trip *trip = tz->trips;
> >>> +     int i;
> >>>
> >>> -     for (count = 0; count < tz->num_trips; count++) {
> >>> -             __thermal_zone_get_trip(tz, count, &trip);
> >>> -             if (tz->temperature < trip.temperature)
> >>> +     if (tz->temperature < trip->temperature)
> >>> +             return 0;
> >>> +
> >>> +     for (i = 0; i < tz->num_trips - 1; i++) {
> >>> +             trip++;
> >>> +             if (tz->temperature < trip->temperature)
> >>>                        break;
> >>>        }
> >>
> >> Is it possible to use for_each_thermal_trip() instead ? That would make
> >> the code more self-encapsulate
> >
> > It is possible in principle, but this is a governor which is regarded
> > as part of the core, isn't it?
> >
> > So is an extra overhead related to using a callback (which may be
> > subject to retpolines and such) really justified in this case?
>
>  From my POV, all trip points browsing should be replaced by
> for_each_thermal_trip() so any change in the future in how we go through
> the existing thermal trips will impact one place.
>
> If the routine needs to be optimized, that is something we can do also
> (may be an inline the callback?)

OK
diff mbox series

Patch

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
@@ -21,23 +21,21 @@ 
  */
 static int get_trip_level(struct thermal_zone_device *tz)
 {
-	struct thermal_trip trip;
-	int count;
+	const struct thermal_trip *trip = tz->trips;
+	int i;
 
-	for (count = 0; count < tz->num_trips; count++) {
-		__thermal_zone_get_trip(tz, count, &trip);
-		if (tz->temperature < trip.temperature)
+	if (tz->temperature < trip->temperature)
+		return 0;
+
+	for (i = 0; i < tz->num_trips - 1; i++) {
+		trip++;
+		if (tz->temperature < trip->temperature)
 			break;
 	}
 
-	/*
-	 * count > 0 only if temperature is greater than first trip
-	 * point, in which case, trip_point = count - 1
-	 */
-	if (count > 0)
-		trace_thermal_zone_trip(tz, count - 1, trip.type);
+	trace_thermal_zone_trip(tz, i, tz->trips[i].type);
 
-	return count;
+	return i;
 }
 
 static long get_target_state(struct thermal_zone_device *tz,