diff mbox series

[v1,14/16] thermal: gov_user_space: Use .trip_crossed() instead of .throttle()

Message ID 15186663.tv2OnDr8pf@kreacher
State New
Headers show
Series thermal: core: Redesign the governor interface | expand

Commit Message

Rafael J. Wysocki April 10, 2024, 5:03 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notifying user space about trip points that have not been crossed is
not particuarly useful, so modity the User Space governor to use the
.trip_crossed() callback, which is only invoked for trips that have been
crossed, instead of .throttle() that is invoked for all trips in a
thermal zone every time the zone is updated.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Note: I am not actually sure if there is user space depending on the
current behavior that can be broken by this change.

I can easily imagine trying to implement a complicated governor in user
space that will look at all of the trips in the thermal zone regardless
of whether or not they are crossed, which can be kind of helped by the
current behavior of the user space governor.

However, the total overhead caused by it is considerable and quite
arguably it may not be acceptable at least in some cases.

---
 drivers/thermal/gov_user_space.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Lukasz Luba April 19, 2024, 6:16 p.m. UTC | #1
On 4/10/24 18:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notifying user space about trip points that have not been crossed is
> not particuarly useful, so modity the User Space governor to use the

s/particuarly/particularly/

s/modity/modify/

> .trip_crossed() callback, which is only invoked for trips that have been
> crossed, instead of .throttle() that is invoked for all trips in a
> thermal zone every time the zone is updated.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Note: I am not actually sure if there is user space depending on the
> current behavior that can be broken by this change.
> 
> I can easily imagine trying to implement a complicated governor in user
> space that will look at all of the trips in the thermal zone regardless
> of whether or not they are crossed, which can be kind of helped by the
> current behavior of the user space governor.
> 
> However, the total overhead caused by it is considerable and quite
> arguably it may not be acceptable at least in some cases.

If there is a such user space governor - it should not rely on
notifications from each trip. It should rather read from sysfs
the list of all trips during setup and just act on those which
have been crossed in runtime, IMO.

Therefore, I agree with the proposed change.

> 
> ---
>   drivers/thermal/gov_user_space.c |   10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/thermal/gov_user_space.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_user_space.c
> +++ linux-pm/drivers/thermal/gov_user_space.c
> @@ -26,11 +26,13 @@ static int user_space_bind(struct therma
>    * notify_user_space - Notifies user space about thermal events
>    * @tz: thermal_zone_device
>    * @trip: trip point
> + * @crossed_up: whether or not the trip has been crossed on the way up
>    *
>    * This function notifies the user space through UEvents.
>    */
> -static int notify_user_space(struct thermal_zone_device *tz,
> -			     const struct thermal_trip *trip)
> +static void notify_user_space(struct thermal_zone_device *tz,
> +			      const struct thermal_trip *trip,
> +			      bool crossed_up)
>   {
>   	char *thermal_prop[5];
>   	int i;
> @@ -46,13 +48,11 @@ static int notify_user_space(struct ther
>   	kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
>   	for (i = 0; i < 4; ++i)
>   		kfree(thermal_prop[i]);
> -
> -	return 0;
>   }
>   
>   static struct thermal_governor thermal_gov_user_space = {
>   	.name		= "user_space",
> -	.throttle	= notify_user_space,
> +	.trip_crossed	= notify_user_space,
>   	.bind_to_tz	= user_space_bind,
>   };
>   THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);
> 
> 
> 

LGTM w/ the spelling fixes applied

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Daniel Lezcano April 24, 2024, 9:14 a.m. UTC | #2
On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notifying user space about trip points that have not been crossed is
> not particuarly useful, so modity the User Space governor to use the
> .trip_crossed() callback, which is only invoked for trips that have been
> crossed, instead of .throttle() that is invoked for all trips in a
> thermal zone every time the zone is updated.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I would also consider removing this governor which is pointless now that we
have the netlink notification mechanism
Srinivas Pandruvada April 24, 2024, 11:32 a.m. UTC | #3
On 4/24/24 02:14, Daniel Lezcano wrote:
> On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Notifying user space about trip points that have not been crossed is
>> not particuarly useful, so modity the User Space governor to use the
>> .trip_crossed() callback, which is only invoked for trips that have been
>> crossed, instead of .throttle() that is invoked for all trips in a
>> thermal zone every time the zone is updated.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> I would also consider removing this governor which is pointless now that we
> have the netlink notification mechanism

That is a good goal, But, not there yet to deprecate.

Thanks,

Srinivas

>
Daniel Lezcano April 24, 2024, 11:34 a.m. UTC | #4
On 24/04/2024 13:32, Srinivas Pandruvada wrote:
> 
> On 4/24/24 02:14, Daniel Lezcano wrote:
>> On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Notifying user space about trip points that have not been crossed is
>>> not particuarly useful, so modity the User Space governor to use the
>>> .trip_crossed() callback, which is only invoked for trips that have been
>>> crossed, instead of .throttle() that is invoked for all trips in a
>>> thermal zone every time the zone is updated.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> I would also consider removing this governor which is pointless now 
>> that we
>> have the netlink notification mechanism
> 
> That is a good goal, But, not there yet to deprecate.

What can be done to deprecate it ?
Srinivas Pandruvada April 24, 2024, 11:44 a.m. UTC | #5
On 4/24/24 04:34, Daniel Lezcano wrote:
> On 24/04/2024 13:32, Srinivas Pandruvada wrote:
>>
>> On 4/24/24 02:14, Daniel Lezcano wrote:
>>> On Wed, Apr 10, 2024 at 07:03:10PM +0200, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> Notifying user space about trip points that have not been crossed is
>>>> not particuarly useful, so modity the User Space governor to use the
>>>> .trip_crossed() callback, which is only invoked for trips that have 
>>>> been
>>>> crossed, instead of .throttle() that is invoked for all trips in a
>>>> thermal zone every time the zone is updated.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> I would also consider removing this governor which is pointless now 
>>> that we
>>> have the netlink notification mechanism
>>
>> That is a good goal, But, not there yet to deprecate.
>
> What can be done to deprecate it ?
>
>

First we need to migrate all the existing usage to netlink. We need to 
add some more netlink notifications. That is relatively easy.

The problem is user space changes are much slower to push than kernel. 
Kernels changes gets deployed at faster pace in some distributions.

Thanks,

Srinivas
diff mbox series

Patch

Index: linux-pm/drivers/thermal/gov_user_space.c
===================================================================
--- linux-pm.orig/drivers/thermal/gov_user_space.c
+++ linux-pm/drivers/thermal/gov_user_space.c
@@ -26,11 +26,13 @@  static int user_space_bind(struct therma
  * notify_user_space - Notifies user space about thermal events
  * @tz: thermal_zone_device
  * @trip: trip point
+ * @crossed_up: whether or not the trip has been crossed on the way up
  *
  * This function notifies the user space through UEvents.
  */
-static int notify_user_space(struct thermal_zone_device *tz,
-			     const struct thermal_trip *trip)
+static void notify_user_space(struct thermal_zone_device *tz,
+			      const struct thermal_trip *trip,
+			      bool crossed_up)
 {
 	char *thermal_prop[5];
 	int i;
@@ -46,13 +48,11 @@  static int notify_user_space(struct ther
 	kobject_uevent_env(&tz->device.kobj, KOBJ_CHANGE, thermal_prop);
 	for (i = 0; i < 4; ++i)
 		kfree(thermal_prop[i]);
-
-	return 0;
 }
 
 static struct thermal_governor thermal_gov_user_space = {
 	.name		= "user_space",
-	.throttle	= notify_user_space,
+	.trip_crossed	= notify_user_space,
 	.bind_to_tz	= user_space_bind,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);