diff mbox series

cpufreq: fail to start a governor if limits weren't updated correctly

Message ID 20240430143923.40431-1-faresx@amazon.de (mailing list archive)
State Changes Requested, archived
Headers show
Series cpufreq: fail to start a governor if limits weren't updated correctly | expand

Commit Message

Fares Mehanna April 30, 2024, 2:39 p.m. UTC 
Current cpufreq governors are using `__cpufreq_driver_target()` in their
`.limits()` functions to update the frequency. `__cpufreq_driver_target()`
will eventually call `.target()` or `.target_index()` in the cpufreq driver
to update the frequency.

`.target()`, `.target_index()` and `__cpufreq_driver_target()` may fail and
all do return an error code, this error code is dropped by the governor and
not propagated to the core.

This have the downside of accepting a new CPU governor even if it fails to
set the wanted limits. This is misleading to the sysfs user, as setting the
governor will be accepted but the governor itself is not functioning as
expected. Especially with `performance` and `powersave` where they only
target specific frequency during starting of the governor and stays the
same during their lifetime.

This change will cause a failure to start the new governor if `.limits()`
failed, propagating back to userspace if the change is driven by sysfs.

Signed-off-by: Fares Mehanna <faresx@amazon.de>
---
 drivers/cpufreq/cpufreq.c             |  7 +++++--
 drivers/cpufreq/cpufreq_governor.c    |  6 ++++--
 drivers/cpufreq/cpufreq_governor.h    |  2 +-
 drivers/cpufreq/cpufreq_performance.c |  4 ++--
 drivers/cpufreq/cpufreq_powersave.c   |  4 ++--
 drivers/cpufreq/cpufreq_userspace.c   | 16 +++++++++-------
 include/linux/cpufreq.h               | 13 +++++++------
 kernel/sched/cpufreq_schedutil.c      |  6 ++++--
 8 files changed, 34 insertions(+), 24 deletions(-)

Comments

Viresh Kumar May 20, 2024, 7:11 a.m. UTC | #1 
On 30-04-24, 14:39, Fares Mehanna wrote:
> Current cpufreq governors are using `__cpufreq_driver_target()` in their
> `.limits()` functions to update the frequency. `__cpufreq_driver_target()`
> will eventually call `.target()` or `.target_index()` in the cpufreq driver
> to update the frequency.
> 
> `.target()`, `.target_index()` and `__cpufreq_driver_target()` may fail and
> all do return an error code, this error code is dropped by the governor and
> not propagated to the core.
> 
> This have the downside of accepting a new CPU governor even if it fails to
> set the wanted limits. This is misleading to the sysfs user, as setting the
> governor will be accepted but the governor itself is not functioning as
> expected. Especially with `performance` and `powersave` where they only
> target specific frequency during starting of the governor and stays the
> same during their lifetime.
> 
> This change will cause a failure to start the new governor if `.limits()`
> failed, propagating back to userspace if the change is driven by sysfs.
> 
> Signed-off-by: Fares Mehanna <faresx@amazon.de>
> ---
>  drivers/cpufreq/cpufreq.c             |  7 +++++--
>  drivers/cpufreq/cpufreq_governor.c    |  6 ++++--
>  drivers/cpufreq/cpufreq_governor.h    |  2 +-
>  drivers/cpufreq/cpufreq_performance.c |  4 ++--
>  drivers/cpufreq/cpufreq_powersave.c   |  4 ++--
>  drivers/cpufreq/cpufreq_userspace.c   | 16 +++++++++-------
>  include/linux/cpufreq.h               | 13 +++++++------
>  kernel/sched/cpufreq_schedutil.c      |  6 ++++--
>  8 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 66e10a19d76a..5ac44a44d319 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2474,8 +2474,11 @@ int cpufreq_start_governor(struct cpufreq_policy *policy)
>  			return ret;
>  	}
>  
> -	if (policy->governor->limits)
> -		policy->governor->limits(policy);
> +	if (policy->governor->limits) {
> +		ret = policy->governor->limits(policy);
> +		if (ret)
> +			return ret;

You need to stop the governor here on failure as this function started it
successfully.
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 66e10a19d76a..5ac44a44d319 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2474,8 +2474,11 @@  int cpufreq_start_governor(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	if (policy->governor->limits)
-		policy->governor->limits(policy);
+	if (policy->governor->limits) {
+		ret = policy->governor->limits(policy);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index af44ee6a6430..d4e5d433cf68 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -560,9 +560,10 @@  void cpufreq_dbs_governor_stop(struct cpufreq_policy *policy)
 }
 EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_stop);
 
-void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
+int cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
 {
 	struct policy_dbs_info *policy_dbs;
+	int rc = 0;
 
 	/* Protect gov->gdbs_data against cpufreq_dbs_governor_exit() */
 	mutex_lock(&gov_dbs_data_mutex);
@@ -571,11 +572,12 @@  void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
 		goto out;
 
 	mutex_lock(&policy_dbs->update_mutex);
-	cpufreq_policy_apply_limits(policy);
+	rc = cpufreq_policy_apply_limits(policy);
 	gov_update_sample_delay(policy_dbs, 0);
 	mutex_unlock(&policy_dbs->update_mutex);
 
 out:
 	mutex_unlock(&gov_dbs_data_mutex);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_limits);
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 168c23fd7fca..551c8e7f1df9 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -150,7 +150,7 @@  int cpufreq_dbs_governor_init(struct cpufreq_policy *policy);
 void cpufreq_dbs_governor_exit(struct cpufreq_policy *policy);
 int cpufreq_dbs_governor_start(struct cpufreq_policy *policy);
 void cpufreq_dbs_governor_stop(struct cpufreq_policy *policy);
-void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy);
+int cpufreq_dbs_governor_limits(struct cpufreq_policy *policy);
 
 #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)			\
 	{								\
diff --git a/drivers/cpufreq/cpufreq_performance.c b/drivers/cpufreq/cpufreq_performance.c
index addd93f2a420..3e02896a155b 100644
--- a/drivers/cpufreq/cpufreq_performance.c
+++ b/drivers/cpufreq/cpufreq_performance.c
@@ -11,10 +11,10 @@ 
 #include <linux/init.h>
 #include <linux/module.h>
 
-static void cpufreq_gov_performance_limits(struct cpufreq_policy *policy)
+static int cpufreq_gov_performance_limits(struct cpufreq_policy *policy)
 {
 	pr_debug("setting to %u kHz\n", policy->max);
-	__cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H);
+	return __cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H);
 }
 
 static struct cpufreq_governor cpufreq_gov_performance = {
diff --git a/drivers/cpufreq/cpufreq_powersave.c b/drivers/cpufreq/cpufreq_powersave.c
index 8d830d860e91..68eebfcae742 100644
--- a/drivers/cpufreq/cpufreq_powersave.c
+++ b/drivers/cpufreq/cpufreq_powersave.c
@@ -11,10 +11,10 @@ 
 #include <linux/init.h>
 #include <linux/module.h>
 
-static void cpufreq_gov_powersave_limits(struct cpufreq_policy *policy)
+static int cpufreq_gov_powersave_limits(struct cpufreq_policy *policy)
 {
 	pr_debug("setting to %u kHz\n", policy->min);
-	__cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
+	return __cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
 }
 
 static struct cpufreq_governor cpufreq_gov_powersave = {
diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c
index 2c42fee76daa..fb6a9d955189 100644
--- a/drivers/cpufreq/cpufreq_userspace.c
+++ b/drivers/cpufreq/cpufreq_userspace.c
@@ -102,8 +102,9 @@  static void cpufreq_userspace_policy_stop(struct cpufreq_policy *policy)
 	mutex_unlock(&userspace->mutex);
 }
 
-static void cpufreq_userspace_policy_limits(struct cpufreq_policy *policy)
+static int cpufreq_userspace_policy_limits(struct cpufreq_policy *policy)
 {
+	int rc;
 	struct userspace_policy *userspace = policy->governor_data;
 
 	mutex_lock(&userspace->mutex);
@@ -112,16 +113,17 @@  static void cpufreq_userspace_policy_limits(struct cpufreq_policy *policy)
 		 policy->cpu, policy->min, policy->max, policy->cur, userspace->setspeed);
 
 	if (policy->max < userspace->setspeed)
-		__cpufreq_driver_target(policy, policy->max,
-					CPUFREQ_RELATION_H);
+		rc = __cpufreq_driver_target(policy, policy->max,
+					     CPUFREQ_RELATION_H);
 	else if (policy->min > userspace->setspeed)
-		__cpufreq_driver_target(policy, policy->min,
-					CPUFREQ_RELATION_L);
+		rc = __cpufreq_driver_target(policy, policy->min,
+					     CPUFREQ_RELATION_L);
 	else
-		__cpufreq_driver_target(policy, userspace->setspeed,
-					CPUFREQ_RELATION_L);
+		rc = __cpufreq_driver_target(policy, userspace->setspeed,
+					     CPUFREQ_RELATION_L);
 
 	mutex_unlock(&userspace->mutex);
+	return rc;
 }
 
 static struct cpufreq_governor cpufreq_gov_userspace = {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 9956afb9acc2..f5c2bf659701 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -579,7 +579,7 @@  struct cpufreq_governor {
 	void	(*exit)(struct cpufreq_policy *policy);
 	int	(*start)(struct cpufreq_policy *policy);
 	void	(*stop)(struct cpufreq_policy *policy);
-	void	(*limits)(struct cpufreq_policy *policy);
+	int	(*limits)(struct cpufreq_policy *policy);
 	ssize_t	(*show_setspeed)	(struct cpufreq_policy *policy,
 					 char *buf);
 	int	(*store_setspeed)	(struct cpufreq_policy *policy,
@@ -637,14 +637,15 @@  module_exit(__governor##_exit)
 struct cpufreq_governor *cpufreq_default_governor(void);
 struct cpufreq_governor *cpufreq_fallback_governor(void);
 
-static inline void cpufreq_policy_apply_limits(struct cpufreq_policy *policy)
+static inline int cpufreq_policy_apply_limits(struct cpufreq_policy *policy)
 {
 	if (policy->max < policy->cur)
-		__cpufreq_driver_target(policy, policy->max,
-					CPUFREQ_RELATION_HE);
+		return __cpufreq_driver_target(policy, policy->max,
+					       CPUFREQ_RELATION_HE);
 	else if (policy->min > policy->cur)
-		__cpufreq_driver_target(policy, policy->min,
-					CPUFREQ_RELATION_LE);
+		return __cpufreq_driver_target(policy, policy->min,
+					       CPUFREQ_RELATION_LE);
+	return 0;
 }
 
 /* Governor attribute set */
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index eece6244f9d2..9c1e3dbe9657 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -871,17 +871,19 @@  static void sugov_stop(struct cpufreq_policy *policy)
 	}
 }
 
-static void sugov_limits(struct cpufreq_policy *policy)
+static int sugov_limits(struct cpufreq_policy *policy)
 {
 	struct sugov_policy *sg_policy = policy->governor_data;
+	int rc = 0;
 
 	if (!policy->fast_switch_enabled) {
 		mutex_lock(&sg_policy->work_lock);
-		cpufreq_policy_apply_limits(policy);
+		rc = cpufreq_policy_apply_limits(policy);
 		mutex_unlock(&sg_policy->work_lock);
 	}
 
 	sg_policy->limits_changed = true;
+	return rc;
 }
 
 struct cpufreq_governor schedutil_gov = {