drivers: Frequency constraint infrastructure
Subsystems (2)
Viresh Kumar
maintainercover drivers: Frequency constraint infrastructure
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
--
viresh
[1] lore.kernel.org/lkml/[email protected]
[2] lore.kernel.org/lkml/[email protected]
Viresh Kumar (3):
drivers: base: Add frequency constraint infrastructure
cpufreq: Implement freq-constraint callback
cpufreq: Implement USER constraint
MAINTAINERS | 8 + drivers/base/Kconfig | 5 + drivers/base/Makefile | 1 + drivers/base/freq_constraint.c | 633 ++++++++++++++++++++++++++++++++++++++++ drivers/cpufreq/Kconfig | 1 + drivers/cpufreq/cpufreq.c | 92 ++++-- include/linux/cpufreq.h | 8 +- include/linux/freq_constraint.h | 45 +++ 8 files changed, 756 insertions(+), 37 deletions(-) create mode 100644 drivers/base/freq_constraint.c create mode 100644 include/linux/freq_constraint.h
Signature · 1 line
2.7.4
Viresh Kumar
maintainerPATCH 1/3 drivers: base: Add frequency constraint infrastructure
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Frameworks like cpufreq and devfreq need to provide a callback, which the freq-constraint core will call on updates to the constraints, with the help of freq_constraint_{set|remove}_dev_callback() OR freq_constraint_{set|remove}_cpumask_callback() helpers.
Individual constraints can be managed by any part of the kernel with the help of freq_constraint_{add|remove|update}() helpers.
Whenever a device constraint is added, removed or updated, the freq-constraint core re-calculates the aggregated constraints on the device and calls the callback if the min-max range has changed.
The current constraints on a device can be read using freq_constraints_get().
--- MAINTAINERS | 8 + drivers/base/Kconfig | 5 + drivers/base/Makefile | 1 + drivers/base/freq_constraint.c | 633 ++++++++++++++++++++++++++++++++++++++++ include/linux/freq_constraint.h | 45 +++ 5 files changed, 692 insertions(+) create mode 100644 drivers/base/freq_constraint.c create mode 100644 include/linux/freq_constraint.h
MAINTAINERS +8 -0
@@ -6176,6 +6176,14 @@ F: Documentation/power/freezing-of-tasks.txt F: include/linux/freezer.h F: kernel/freezer.c +FREQUENCY CONSTRAINTS +M: Viresh Kumar <[email protected]> +L: [email protected] +S: Maintained +T: git git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git +F: drivers/base/freq_constraint.c +F: include/linux/freq_constraint.h + FRONTSWAP API M: Konrad Rzeszutek Wilk <[email protected]> L: [email protected]
drivers/base/Kconfig +5 -0
@@ -26,6 +26,11 @@ config UEVENT_HELPER_PATH via /proc/sys/kernel/hotplug or via /sys/kernel/uevent_helper later at runtime. +config DEVICE_FREQ_CONSTRAINT + bool + help + Enable support for device frequency constraints. + config DEVTMPFS bool "Maintain a devtmpfs filesystem to mount at /dev" help
drivers/base/Makefile +1 -0
@@ -23,6 +23,7 @@ obj-$(CONFIG_PINCTRL) += pinctrl.o obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o +obj-$(CONFIG_DEVICE_FREQ_CONSTRAINT) += freq_constraint.o obj-y += test/
drivers/base/freq_constraint.c new file +633 -0
633 lines not on this page · raw message
include/linux/freq_constraint.h new file +45 -0
45 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure
Hi Viresh,
Thanks for your work on this!
Not a complete review, more a first pass.
Quoted 94 lines · Viresh Kumar
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Frameworks like cpufreq and devfreq need to provide a callback, which the freq-constraint core will call on updates to the constraints, with the help of freq_constraint_{set|remove}_dev_callback() OR freq_constraint_{set|remove}_cpumask_callback() helpers.
Individual constraints can be managed by any part of the kernel with the help of freq_constraint_{add|remove|update}() helpers.
Whenever a device constraint is added, removed or updated, the freq-constraint core re-calculates the aggregated constraints on the device and calls the callback if the min-max range has changed.
The current constraints on a device can be read using
freq_constraints_get().Co-developed-by Matthias Kaehlcke <[email protected]>Signed-off-by Viresh Kumar <[email protected]>--- MAINTAINERS | 8 + drivers/base/Kconfig | 5 + drivers/base/Makefile | 1 + drivers/base/freq_constraint.c | 633 ++++++++++++++++++++++++++++++++++++++++ include/linux/freq_constraint.h | 45 +++ 5 files changed, 692 insertions(+) create mode 100644 drivers/base/freq_constraint.c create mode 100644 include/linux/freq_constraint.hMAINTAINERS +8
-0@@ -6176,6 +6176,14 @@ F: Documentation/power/freezing-of-tasks.txt F: include/linux/freezer.h F: kernel/freezer.c +FREQUENCY CONSTRAINTS +M: Viresh Kumar <[email protected]> +L: [email protected] +S: Maintained +T: git git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git +F: drivers/base/freq_constraint.c +F: include/linux/freq_constraint.h + FRONTSWAP API M: Konrad Rzeszutek Wilk <[email protected]> L: [email protected]drivers/base/Kconfig +5
-0@@ -26,6 +26,11 @@ config UEVENT_HELPER_PATH via /proc/sys/kernel/hotplug or via /sys/kernel/uevent_helper later at runtime. +config DEVICE_FREQ_CONSTRAINT + bool + help + Enable support for device frequency constraints. + config DEVTMPFS bool "Maintain a devtmpfs filesystem to mount at /dev" helpdrivers/base/Makefile +1
-0@@ -23,6 +23,7 @@ obj-$(CONFIG_PINCTRL) += pinctrl.o obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o +obj-$(CONFIG_DEVICE_FREQ_CONSTRAINT) += freq_constraint.o obj-y += test/drivers/base/freq_constraint.c new file +0
-0
drivers/base/freq_constraint.c line 165
Quoted 10 lines
drivers/base/freq_constraint.c new file +8
-0+static void fcs_update (struct freq_constraints *fcs,struct freq_pair *freq, +enum fc_event event) +{ +mutex_lock (&fcs->lock); + +if (_fcs_update (fcs, freq, event)) { +if (fcs->callback) +schedule_work (&fcs->work);
IIUC the constraints aren't applied until the callback is executed. I wonder if a dedicated workqueue should be used instead of the system one, to avoid longer delays from other kernel entities that might 'misbehave'. Especially for thermal constraints we want a quick response.
drivers/base/freq_constraint.c line 378
Quoted 9 lines
drivers/base/freq_constraint.c new file +9
-0+void freq_constraint_remove (struct device *dev, +struct freq_constraint *constraint) +{ +struct freq_constraints *fcs; +struct freq_pair freq = constraint->freq; + + fcs =find_fcs (dev); +if (IS_ERR (fcs)) { +dev_err (dev,"Failed to find freq-constraint\n" );
"freq-constraint: device not registered\n" as in other functions?
drivers/base/freq_constraint.c line 386
Quoted 8 lines
drivers/base/freq_constraint.c new file +8
-0+return ; + } + +free_constraint (fcs, constraint); +fcs_update (fcs, &freq,REMOVE ); + +/* +* Put the reference twice, once for the freed constraint and one for
s/one/once/
Quoted 5 lines
drivers/base/freq_constraint.c new file +5
-0+int freq_constraint_update (struct device *dev, +struct freq_constraint *constraint, +unsigned long min_freq, +unsigned long max_freq) +{
drivers/base/freq_constraint.c line 408
Quoted 10 lines
drivers/base/freq_constraint.c new file +10
-0+struct freq_constraints *fcs; + +if (!max_freq || min_freq > max_freq) { +dev_err (dev,"freq-constraints: Invalid min/max frequency\n" ); +return -EINVAL ; + } + + fcs =find_fcs (dev); +if (IS_ERR (fcs)) { +dev_err (dev,"Failed to find freq-constraint\n" );
same as above
drivers/base/freq_constraint.c line 464
Quoted 9 lines
drivers/base/freq_constraint.c new file +9
-0+int freq_constraint_set_dev_callback (struct device *dev, +void (*callback)(void *param), +void *callback_param) +{ +struct freq_constraints *fcs; +int ret; + +if (WARN_ON (!callback)) +return -ENODEV ;
Wouldn't that be rather -EINVAL?
Quoted 10 lines
drivers/base/freq_constraint.c new file +10
-0+/* Caller must call put_fcs() after using it */ +static struct freq_constraints *remove_callback (struct device *dev) +{ +struct freq_constraints *fcs; + + fcs =find_fcs (dev); +if (IS_ERR (fcs)) { +dev_err (dev,"freq-constraint: device not registered\n" ); +return fcs; + }
drivers/base/freq_constraint.c line 500
Quoted 10 lines
drivers/base/freq_constraint.c new file +10
-0+ +mutex_lock (&fcs->lock); + +cancel_work_sync (&fcs->work); + +if (fcs->callback) { + fcs->callback =NULL ; + fcs->callback_param =NULL ; + }else { +dev_err (dev,"freq-constraint: Call back not registered for device\n" );
s/Call back/callback/ (for consistency with other messages)
or "no callback registered ..."
drivers/base/freq_constraint.c line 516
Quoted 10 lines
drivers/base/freq_constraint.c new file +10
-0+void freq_constraint_remove_dev_callback (struct device *dev) +{ +struct freq_constraints *fcs; + + fcs =remove_callback (dev); +if (IS_ERR (fcs)) +return ; + +/* +* Put the reference twice, once for the callback removal and one for
s/one/once/
drivers/base/freq_constraint.c line 553
Quoted 11 lines
drivers/base/freq_constraint.c new file +11
-0+int freq_constraint_set_cpumask_callback (const struct cpumask *cpumask, +void (*callback)(void *param), +void *callback_param) +{ +struct freq_constraints *fcs =ERR_PTR (-ENODEV ); +struct device *cpu_dev, *first_cpu_dev =NULL ; +struct freq_constraint_dev *fcdev; +int cpu, ret; + +if (WARN_ON (cpumask_empty (cpumask) || !callback)) +return -ENODEV ;
-EINVAL?
drivers/base/freq_constraint.c line 562
Quoted 9 lines
drivers/base/freq_constraint.c new file +9
-0+ +/* Find a CPU for which fcs already exists */ +for_each_cpu (cpu, cpumask) { + cpu_dev =get_cpu_device (cpu); +if (unlikely (!cpu_dev)) +continue ; + +if (unlikely (!first_cpu_dev)) + first_cpu_dev = cpu_dev;
I'd expect setting the callback to be a one time/rare operation. Is there really any gain from cluttering this code with 'unlikely's?
There are other functions where it could be removed if the outcome is that it isn't needed/desirable in code that only runs sporadically.
Quoted 31 lines
drivers/base/freq_constraint.c new file +31
-0+ + fcs =find_fcs (cpu_dev); +if (!IS_ERR (fcs)) +break ; + } + +/* Allocate fcs if it wasn't already present */ +if (IS_ERR (fcs)) { +if (unlikely (!first_cpu_dev)) { +pr_err ("device structure not available for any CPU\n" ); +return -ENODEV ; + } + + fcs =alloc_fcs (first_cpu_dev); +if (IS_ERR (fcs)) +return PTR_ERR (fcs); + } + +for_each_cpu (cpu, cpumask) { + cpu_dev =get_cpu_device (cpu); +if (unlikely (!cpu_dev)) +continue ; + +if (!find_fcdev (cpu_dev, fcs)) { + fcdev =alloc_fcdev (cpu_dev, fcs); +if (IS_ERR (fcdev)) { +remove_cpumask_fcs (fcs, cpumask, cpu); +put_fcs (fcs); +return PTR_ERR (fcdev); + } + }
drivers/base/freq_constraint.c line 603
Quoted 10 lines
drivers/base/freq_constraint.c new file +10
-0+ +kref_get (&fcs->kref); + } + +mutex_lock (&fcs->lock); + ret =set_fcs_callback (first_cpu_dev, fcs, callback, callback_param); +mutex_unlock (&fcs->lock); + +if (ret) +remove_cpumask_fcs (fcs, cpumask, cpu);
I think it would be clearer to pass -1 instead of 'cpu', as in freq_constraint_remove_cpumask_callback(), no need to backtrack and 'confirm' that the above for loop always stops at the last CPU in the cpumask (unless the function returns due to an error).
Cheers
Matthia
Viresh Kumar
maintainerRe: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure
Quoted 15 lines · Matthias Kaehlcke
drivers/base/freq_constraint.c line 165
Quoted 8 lines · Viresh Kumar
drivers/base/freq_constraint.c new file +8
-0+static void fcs_update (struct freq_constraints *fcs,struct freq_pair *freq, +enum fc_event event) +{ +mutex_lock (&fcs->lock); + +if (_fcs_update (fcs, freq, event)) { +if (fcs->callback) +schedule_work (&fcs->work);IIUC the constraints aren't applied until the callback is executed. I wonder if a dedicated workqueue should be used instead of the system one, to avoid longer delays from other kernel entities that might 'misbehave'. Especially for thermal constraints we want a quick response.
I thought the system workqueue should be fast enough, it contains multiple threads which can all run in parallel and service this work.
Quoted 13 lines
drivers/base/freq_constraint.c line 562
Quoted 9 lines
drivers/base/freq_constraint.c new file +9
-0+ +/* Find a CPU for which fcs already exists */ +for_each_cpu (cpu, cpumask) { + cpu_dev =get_cpu_device (cpu); +if (unlikely (!cpu_dev)) +continue ; + +if (unlikely (!first_cpu_dev)) + first_cpu_dev = cpu_dev;I'd expect setting the callback to be a one time/rare operation. Is there really any gain from cluttering this code with 'unlikely's?
Quoted 2 lines
There are other functions where it could be removed if the outcome is that it isn't needed/desirable in code that only runs sporadically.
I was looking to make the code as fast as possible and the use of unlikely doesn't look that bad to me. Lets see what others have to say on such a policy.
Quoted 7 lines
drivers/base/freq_constraint.c line 603
Quoted 2 lines
drivers/base/freq_constraint.c new file +2
-0+if (ret) +remove_cpumask_fcs (fcs, cpumask, cpu);I think it would be clearer to pass -1 instead of 'cpu', as in
freq_constraint_remove_cpumask_callback(), no need to backtrack and 'confirm' that the above for loop always stops at the last CPU in the cpumask (unless the function returns due to an error).
Okay.
Signature · 1 line
viresh
Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure
Quoted 17 lines · Viresh Kumar
Quoted 15 lines · Matthias Kaehlcke
drivers/base/freq_constraint.c line 165
Quoted 8 lines · Viresh Kumar
drivers/base/freq_constraint.c new file +8
-0+static void fcs_update (struct freq_constraints *fcs,struct freq_pair *freq, +enum fc_event event) +{ +mutex_lock (&fcs->lock); + +if (_fcs_update (fcs, freq, event)) { +if (fcs->callback) +schedule_work (&fcs->work);IIUC the constraints aren't applied until the callback is executed. I wonder if a dedicated workqueue should be used instead of the system one, to avoid longer delays from other kernel entities that might 'misbehave'. Especially for thermal constraints we want a quick response.
Quoted 2 lines
I thought the system workqueue should be fast enough, it contains multiple threads which can all run in parallel and service this work.
Ok, I was still stuck at the old one thread per CPU model, where a slow work would block other items in the same workqueue until it finishes execution. After reading a bit through Documentation/core-api/workqueue.rst I agree that a system workqueue is probably fast enough. It might be warranted though to use system_highpri_wq here.
Cheers
Matthias
Viresh Kumar
maintainerRe: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure
Quoted 21 lines · Matthias Kaehlcke
Quoted 17 lines · Viresh Kumar
Quoted 15 lines · Matthias Kaehlcke
drivers/base/freq_constraint.c line 165
Quoted 8 lines · Viresh Kumar
drivers/base/freq_constraint.c new file +8
-0+static void fcs_update (struct freq_constraints *fcs,struct freq_pair *freq, +enum fc_event event) +{ +mutex_lock (&fcs->lock); + +if (_fcs_update (fcs, freq, event)) { +if (fcs->callback) +schedule_work (&fcs->work);IIUC the constraints aren't applied until the callback is executed. I wonder if a dedicated workqueue should be used instead of the system one, to avoid longer delays from other kernel entities that might 'misbehave'. Especially for thermal constraints we want a quick response.
Quoted 2 lines
I thought the system workqueue should be fast enough, it contains multiple threads which can all run in parallel and service this work.
Quoted 6 lines
Ok, I was still stuck at the old one thread per CPU model, where a slow work would block other items in the same workqueue until it finishes execution. After reading a bit through
Documentation/core-api/workqueue.rstI agree that a system workqueue is probably fast enough. It might be warranted though to usesystem_highpri_wqhere.
Is this really that high priority stuff ? I am not sure.
Signature · 1 line
viresh
Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure
Quoted 22 lines · Viresh Kumar
Quoted 21 lines · Matthias Kaehlcke
Quoted 19 lines · Viresh Kumar
Quoted 15 lines · Matthias Kaehlcke
drivers/base/freq_constraint.c line 165
Quoted 8 lines · Viresh Kumar
drivers/base/freq_constraint.c new file +8
-0+static void fcs_update (struct freq_constraints *fcs,struct freq_pair *freq, +enum fc_event event) +{ +mutex_lock (&fcs->lock); + +if (_fcs_update (fcs, freq, event)) { +if (fcs->callback) +schedule_work (&fcs->work);IIUC the constraints aren't applied until the callback is executed. I wonder if a dedicated workqueue should be used instead of the system one, to avoid longer delays from other kernel entities that might 'misbehave'. Especially for thermal constraints we want a quick response.
I thought the system workqueue should be fast enough, it contains multiple threads which can all run in parallel and service this work.
Quoted 8 lines
Quoted 6 lines
Ok, I was still stuck at the old one thread per CPU model, where a slow work would block other items in the same workqueue until it finishes execution. After reading a bit through
Documentation/core-api/workqueue.rstI agree that a system workqueue is probably fast enough. It might be warranted though to usesystem_highpri_wqhere.Is this really that high priority stuff ? I am not sure.
In terms of thermal it could be. But then again, thermal throttling is driven by input from thermal sensors, which often are polled with periods >= 100 ms rather than being interrupt driven, so the type of workqueue wouldn't make a major difference here. I now think it should be fine to use the normal workqueue unless problems are reported.
Viresh Kumar
maintainerPATCH 2/3 cpufreq: Implement freq-constraint callback
In reply to Viresh KumarThis implements the frequency constraint callback and registers it with the freq-constraint framework whenever a policy is created. On policy removal the callback is unregistered.
The constraints are also taken into consideration in cpufreq_set_policy().
No constraints are added until now though.
--- drivers/cpufreq/Kconfig | 1 + drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)
drivers/cpufreq/Kconfig +1 -0
@@ -2,6 +2,7 @@ menu "CPU Frequency scaling" config CPU_FREQ bool "CPU Frequency scaling" + select DEVICE_FREQ_CONSTRAINT select SRCU help CPU Frequency scaling allows you to change the clock speed of
drivers/cpufreq/cpufreq.c +44 -0
80 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [PATCH 2/3] cpufreq: Implement freq-constraint callback
Quoted 8 lines · Viresh Kumar
This implements the frequency constraint callback and registers it with the freq-constraint framework whenever a policy is created. On policy removal the callback is unregistered.
The constraints are also taken into consideration in
cpufreq_set_policy().No constraints are added until now though.
nit: 'for now'?
Quoted 90 lines
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/Kconfig | 1 + drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)drivers/cpufreq/Kconfig +1
-0@@ -2,6 +2,7 @@ menu "CPU Frequency scaling" config CPU_FREQ bool "CPU Frequency scaling" + select DEVICE_FREQ_CONSTRAINT select SRCU help CPU Frequency scaling allows you to change the clock speed ofdrivers/cpufreq/cpufreq.c +30
-0@@ -21,6 +21,7 @@#include <linux/cpufreq.h> #include <linux/delay.h> #include <linux/device.h> +#include <linux/freq_constraint.h> #include <linux/init.h> #include <linux/kernel_stat.h> #include <linux/module.h> @@ -1163,6 +1164,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)per_cpu (cpufreq_cpu_data, cpu) =NULL ;write_unlock_irqrestore (&cpufreq_driver_lock, flags); +freq_constraint_remove_cpumask_callback (policy->related_cpus);cpufreq_policy_put_kobj (policy);free_cpumask_var (policy->real_cpus);free_cpumask_var (policy->related_cpus);@@ -1170,6 +1172,24 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)kfree (policy); } +static void freq_constraint_callback (void *param) +{ +struct cpufreq_policy *policy = param; +struct cpufreq_policy new_policy = *policy; + + new_policy.min = policy->user_policy.min; + new_policy.max = policy->user_policy.max; + +down_write (&policy->rwsem); +if (policy_is_inactive (policy)) + goto unlock; + +cpufreq_set_policy (policy, &new_policy); + +unlock: +up_write (&policy->rwsem); +} +static int cpufreq_online (unsigned int cpu) {struct cpufreq_policy *policy;@@ -1236,6 +1256,14 @@ static int cpufreq_online(unsigned int cpu)per_cpu (cpufreq_cpu_data, j) = policy;add_cpu_dev_symlink (policy, j); } + + ret =freq_constraint_set_cpumask_callback (policy->related_cpus, + freq_constraint_callback, policy); +if (ret) { +pr_err ("Failed to set freq-constraints: %d (%*pbl)\n" , + ret,cpumask_pr_args (policy->cpus)); + goto out_destroy_policy; + } }else { policy->min = policy->user_policy.min; policy->max = policy->user_policy.max;@@ -2198,6 +2226,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, struct cpufreq_policy *new_policy) {struct cpufreq_governor *old_gov; +struct device *cpu_dev =get_cpu_device (policy->cpu); +unsigned long fc_min, fc_max;int ret; pr_debug("setting new policy for CPU %u: %u - %u kHz\n",@@ -2217,6 +2247,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,if (ret)return ret;
drivers/cpufreq/cpufreq.c static int cpufreq_set_policy(struct cpufreq_policy *policy, · line 2258
Quoted 10 lines
drivers/cpufreq/cpufreq.c +9
-0+ ret =freq_constraints_get (cpu_dev, &fc_min, &fc_max); +if (ret) { +dev_err (cpu_dev,"cpufreq: Failed to get freq-constraints\n" ); + }else { +if (fc_min > new_policy->min) + new_policy->min = fc_min; +if (fc_max < new_policy->max) + new_policy->max = fc_max; + }
nit: for if/else constructs with a typical and an 'exception' case IMO it is usually more readable when the normal case is handled in the 'if' branch (first) and the exception in 'else'.
Cheers
Matthias
Matthias Kaehlcke
Reviewed-byRe: [PATCH 2/3] cpufreq: Implement freq-constraint callback
Quoted 103 lines · Matthias Kaehlcke
Quoted 8 lines · Viresh Kumar
This implements the frequency constraint callback and registers it with the freq-constraint framework whenever a policy is created. On policy removal the callback is unregistered.
The constraints are also taken into consideration in
cpufreq_set_policy().No constraints are added until now though.
nit: 'for now'?
Quoted 91 lines
Signed-off-by Viresh Kumar <[email protected]>--- drivers/cpufreq/Kconfig | 1 + drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)drivers/cpufreq/Kconfig +1
-0@@ -2,6 +2,7 @@ menu "CPU Frequency scaling" config CPU_FREQ bool "CPU Frequency scaling" + select DEVICE_FREQ_CONSTRAINT select SRCU help CPU Frequency scaling allows you to change the clock speed ofdrivers/cpufreq/cpufreq.c +30
-0@@ -21,6 +21,7 @@#include <linux/cpufreq.h> #include <linux/delay.h> #include <linux/device.h> +#include <linux/freq_constraint.h> #include <linux/init.h> #include <linux/kernel_stat.h> #include <linux/module.h> @@ -1163,6 +1164,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)per_cpu (cpufreq_cpu_data, cpu) =NULL ;write_unlock_irqrestore (&cpufreq_driver_lock, flags); +freq_constraint_remove_cpumask_callback (policy->related_cpus);cpufreq_policy_put_kobj (policy);free_cpumask_var (policy->real_cpus);free_cpumask_var (policy->related_cpus);@@ -1170,6 +1172,24 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)kfree (policy); } +static void freq_constraint_callback (void *param) +{ +struct cpufreq_policy *policy = param; +struct cpufreq_policy new_policy = *policy; + + new_policy.min = policy->user_policy.min; + new_policy.max = policy->user_policy.max; + +down_write (&policy->rwsem); +if (policy_is_inactive (policy)) + goto unlock; + +cpufreq_set_policy (policy, &new_policy); + +unlock: +up_write (&policy->rwsem); +} +static int cpufreq_online (unsigned int cpu) {struct cpufreq_policy *policy;@@ -1236,6 +1256,14 @@ static int cpufreq_online(unsigned int cpu)per_cpu (cpufreq_cpu_data, j) = policy;add_cpu_dev_symlink (policy, j); } + + ret =freq_constraint_set_cpumask_callback (policy->related_cpus, + freq_constraint_callback, policy); +if (ret) { +pr_err ("Failed to set freq-constraints: %d (%*pbl)\n" , + ret,cpumask_pr_args (policy->cpus)); + goto out_destroy_policy; + } }else { policy->min = policy->user_policy.min; policy->max = policy->user_policy.max;@@ -2198,6 +2226,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, struct cpufreq_policy *new_policy) {struct cpufreq_governor *old_gov; +struct device *cpu_dev =get_cpu_device (policy->cpu); +unsigned long fc_min, fc_max;int ret; pr_debug("setting new policy for CPU %u: %u - %u kHz\n",@@ -2217,6 +2247,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,if (ret)return ret;
Quoted 13 lines
drivers/cpufreq/cpufreq.c static int cpufreq_set_policy(struct cpufreq_policy *policy, · line 2258
Quoted 9 lines
drivers/cpufreq/cpufreq.c +9
-0+ ret =freq_constraints_get (cpu_dev, &fc_min, &fc_max); +if (ret) { +dev_err (cpu_dev,"cpufreq: Failed to get freq-constraints\n" ); + }else { +if (fc_min > new_policy->min) + new_policy->min = fc_min; +if (fc_max < new_policy->max) + new_policy->max = fc_max; + }nit: for if/else constructs with a typical and an 'exception' case IMO it is usually more readable when the normal case is handled in the 'if' branch (first) and the exception in 'else'.
Forgot to add this:
Viresh Kumar
maintainerPATCH 3/3 cpufreq: Implement USER constraint
In reply to Viresh KumarThis implements the FREQ_CONSTRAINT_USER constraint and removes the old style of doing the same. We just need to update the constraint on any modifications to scaling_{min|max}_frequency and the freq-constraint core will call cpufreq's callback which will call cpufreq_set_policy() eventually.
--- drivers/cpufreq/cpufreq.c | 62 ++++++++++++++++++----------------------------- include/linux/cpufreq.h | 8 ++---- 2 files changed, 26 insertions(+), 44 deletions(-)
drivers/cpufreq/cpufreq.c +24 -38
@@ -685,22 +685,15 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, static ssize_t store_##file_name \ (struct cpufreq_policy *policy,const char *buf,size_t count) \ { \ -int ret, temp; \ -struct cpufreq_policy new_policy; \ +unsigned long min = policy->min, max = policy->max; \ +int ret; \ \ -memcpy (&new_policy, policy,sizeof (*policy)); \ - new_policy.min = policy->user_policy.min; \ - new_policy.max = policy->user_policy.max; \ - \ - ret =sscanf (buf,"%u" , &new_policy.object); \ + ret =sscanf (buf,"%lu" , &object); \if (ret !=1 ) \return -EINVAL ; \ \ - temp = new_policy.object; \ - ret =cpufreq_set_policy (policy, &new_policy); \ -if (!ret) \ - policy->user_policy.object = temp; \ - \ + ret =freq_constraint_update (get_cpu_device (policy->cpu), \ + policy->user_fc, min, max); \return ret ? ret : count; \ }
@@ -1164,6 +1157,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)per_cpu (cpufreq_cpu_data, cpu) =NULL ;write_unlock_irqrestore (&cpufreq_driver_lock, flags); +if (!IS_ERR (policy->user_fc)) +freq_constraint_remove (get_cpu_device (policy->cpu), + policy->user_fc);freq_constraint_remove_cpumask_callback (policy->related_cpus);cpufreq_policy_put_kobj (policy);free_cpumask_var (policy->real_cpus);
@@ -1177,9 +1173,6 @@ static void freq_constraint_callback(void *param)struct cpufreq_policy *policy = param;struct cpufreq_policy new_policy = *policy; - new_policy.min = policy->user_policy.min; - new_policy.max = policy->user_policy.max; -down_write (&policy->rwsem);if (policy_is_inactive (policy)) goto unlock;
@@ -1249,9 +1242,6 @@ static int cpufreq_online(unsigned int cpu)cpumask_and (policy->cpus, policy->cpus, cpu_online_mask);if (new_policy) { - policy->user_policy.min = policy->min; - policy->user_policy.max = policy->max; -for_each_cpu (j, policy->related_cpus) {per_cpu (cpufreq_cpu_data, j) = policy;add_cpu_dev_symlink (policy, j);
@@ -1264,9 +1254,15 @@ static int cpufreq_online(unsigned int cpu) ret, cpumask_pr_args(policy->cpus)); goto out_destroy_policy; } - }else { - policy->min = policy->user_policy.min; - policy->max = policy->user_policy.max; + + policy->user_fc =freq_constraint_add (get_cpu_device (cpu), +FREQ_CONSTRAINT_USER , + policy->min, policy->max); +if (IS_ERR (policy->user_fc)) { + ret =PTR_ERR (policy->user_fc); +pr_err ("Failed to add user constraint: %d\n" , ret); + goto out_destroy_policy; + } }if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
@@ -2235,13 +2231,6 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,memcpy (&new_policy->cpuinfo, &policy->cpuinfo,sizeof (policy->cpuinfo)); -/* -* This check works well when we store new min/max freq attributes, -* because new_policy is a copy of policy with one field updated. -*/ -if (new_policy->min > new_policy->max) -return -EINVAL ; -/* verify the cpu speed can be set within this limit */ ret = cpufreq_driver->verify (new_policy);if (ret)
@@ -2251,10 +2240,8 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,if (ret) {dev_err (cpu_dev,"cpufreq: Failed to get freq-constraints\n" ); }else { -if (fc_min > new_policy->min) - new_policy->min = fc_min; -if (fc_max < new_policy->max) - new_policy->max = fc_max; + new_policy->min = fc_min; + new_policy->max = fc_max; }/*
@@ -2356,8 +2343,6 @@ void cpufreq_update_policy(unsigned int cpu)pr_debug ("updating policy for CPU %u\n" , cpu);memcpy (&new_policy, policy,sizeof (*policy)); - new_policy.min = policy->user_policy.min; - new_policy.max = policy->user_policy.max;/* * BIOS might change freq behind our back
@@ -2401,10 +2386,11 @@ static int cpufreq_boost_set_sw(int state)break ; } -down_write (&policy->rwsem); - policy->user_policy.max = policy->max; -cpufreq_governor_limits (policy); -up_write (&policy->rwsem); + ret =freq_constraint_update (get_cpu_device (policy->cpu), + policy->user_fc, policy->min, + policy->max); +if (ret) +break ; }return ret;
include/linux/cpufreq.h +2 -6
@@ -14,6 +14,7 @@#include <linux/clk.h> #include <linux/cpumask.h> #include <linux/completion.h> +#include <linux/freq_constraint.h> #include <linux/kobject.h> #include <linux/notifier.h> #include <linux/spinlock.h>
@@ -57,11 +58,6 @@ struct cpufreq_cpuinfo {unsigned int transition_latency; }; -struct cpufreq_user_policy { -unsigned int min;/* in kHz */ -unsigned int max;/* in kHz */ -}; -struct cpufreq_policy {/* CPUs sharing clock, require sw coordination */ cpumask_var_t cpus;/* Online CPUs only */
@@ -91,7 +87,7 @@ struct cpufreq_policy {struct work_struct update;/* if update_policy() needs to be * called, but you're in IRQ context */ -struct cpufreq_user_policy user_policy; +struct freq_constraint *user_fc;struct cpufreq_frequency_table *freq_table;enum cpufreq_table_sorting freq_table_sorted;
Signature · 1 line
2.7.4
Rafael J. Wysocki
maintainerQuoted 20 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Quoted 6 lines
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Cheers,
Rafael
Quoted 25 lines · Rafael J. Wysocki
Quoted 24 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 8 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Right, had the same impression. :-)
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's util_clamp).
Thanks,
- Juri
Rafael J. Wysocki
maintainerQuoted 36 lines · Juri Lelli
Quoted 25 lines · Rafael J. Wysocki
Quoted 24 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 8 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Quoted 5 lines
Right, had the same impression. :-)
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).
My understanding is that it is orthogonal to them, like adding extra constraints on top of them etc.
Quoted 37 lines · Rafael J. Wysocki
Quoted 36 lines · Juri Lelli
Quoted 33 lines · Rafael J. Wysocki
Quoted 20 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Quoted 6 lines
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Quoted 8 lines
Quoted 5 lines
Right, had the same impression. :-)
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).My understanding is that it is orthogonal to them, like adding extra constraints on top of them etc.
Mmm, ok. But, if that is indeed the case, I now wonder why and how existing (or hopefully to be added soon) interfaces are not sufficient. I'm not against this proposal, just trying to understand if this might create unwanted, hard to manage, overlap.
Rafael J. Wysocki
maintainerQuoted 45 lines · Juri Lelli
Quoted 43 lines · Rafael J. Wysocki
Quoted 41 lines · Juri Lelli
Quoted 25 lines · Rafael J. Wysocki
Quoted 24 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 8 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Right, had the same impression. :-)
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).
Quoted 7 lines
Quoted 2 lines
My understanding is that it is orthogonal to them, like adding extra constraints on top of them etc.
Mmm, ok. But, if that is indeed the case, I now wonder why and how existing (or hopefully to be added soon) interfaces are not sufficient. I'm not against this proposal, just trying to understand if this might create unwanted, hard to manage, overlap.
That is a valid concern IMO. Especially the utilization clamping and the interconnect framework seem to approach the same problem space from different directions.
For cpufreq this work can be regarded as a replacement for notifiers which are a bandaid of sorts and it would be good to get rid of them. They are mostly used for thermal management and I guess that devfreq users also may want to reduce frequency for thermal reasons and I'd rather not add notifiers to that framework for this purpose.
However, as stated previously, this resembles the PM QoS framework quite a bit to me and whatever thermal entity, say, sets these constraints, it should not work against schedutil and similar. In some situations setting a max frequency limit to control thermals is not the most efficient way to go as it effectively turns into throttling and makes performance go south. For example, it may cause things to run at the limit frequency all the time which may be too slow and it may be more efficient to allow higher frequencies to be used, but instead control how much of the time they can be used. So we need to be careful here.
Quoted 58 lines · Rafael J. Wysocki
Quoted 45 lines · Juri Lelli
Quoted 43 lines · Rafael J. Wysocki
Quoted 41 lines · Juri Lelli
Quoted 25 lines · Rafael J. Wysocki
Quoted 24 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 8 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Right, had the same impression. :-)
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).Quoted 7 lines
Quoted 2 lines
My understanding is that it is orthogonal to them, like adding extra constraints on top of them etc.
Mmm, ok. But, if that is indeed the case, I now wonder why and how existing (or hopefully to be added soon) interfaces are not sufficient. I'm not against this proposal, just trying to understand if this might create unwanted, hard to manage, overlap.
That is a valid concern IMO. Especially the utilization clamping and the interconnect framework seem to approach the same problem space from different directions.
Quoted 5 lines
For cpufreq this work can be regarded as a replacement for notifiers which are a bandaid of sorts and it would be good to get rid of them. They are mostly used for thermal management and I guess that devfreq users also may want to reduce frequency for thermal reasons and I'd rather not add notifiers to that framework for this purpose.
FYI: devfreq already reduces frequency for thermal reasons, however they don't use notifiers, but directly disable OPPs in the cooling driver:
https://elixir.bootlin.com/linux/v4.20.3/source/drivers/thermal/devfreq_cooling.c#L78
The idea to have a frequency constraint framework came up in the context of the throttler series
(https://lore.kernel.org/patchwork/project/lkml/list/?series=357937)
for non-thermal throttling. My initial approach was to copy the notifier bandaid ...
Quoted 10 lines
However, as stated previously, this resembles the PM QoS framework quite a bit to me and whatever thermal entity, say, sets these constraints, it should not work against schedutil and similar. In some situations setting a max frequency limit to control thermals is not the most efficient way to go as it effectively turns into throttling and makes performance go south. For example, it may cause things to run at the limit frequency all the time which may be too slow and it may be more efficient to allow higher frequencies to be used, but instead control how much of the time they can be used. So we need to be careful here.
A message that is not in the archive (sent off-list or not yet received): CA+mqd+7EqERei8eekAsVxa_bJUYETyO3T76L8Q_sV=C9rwiy3g@mail.gmail.com
Quoted 46 lines · Juri Lelli
Quoted 45 lines · Rafael J. Wysocki
Quoted 43 lines · Juri Lelli
Quoted 41 lines · Rafael J. Wysocki
Quoted 25 lines · Viresh Kumar
Quoted 24 lines
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 8 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Right, had the same impression. :-)
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
My understanding is that it is orthogonal to them, like adding extra constraints on top of them etc.
Mmm, ok. But, if that is indeed the case, I now wonder why and how existing (or hopefully to be added soon) interfaces are not sufficient. I'm not against this proposal, just trying to understand if this might create unwanted, hard to manage, overlap.
I echo these concerns as well.
Quoted 11 lines
That is a valid concern IMO. Especially the utilization clamping and the interconnect framework seem to approach the same problem space from different directions.
For cpufreq this work can be regarded as a replacement for notifiers which are a bandaid of sorts and it would be good to get rid of them. They are mostly used for thermal management and I guess that devfreq users also may want to reduce frequency for thermal reasons and I'd rather not add notifiers to that framework for this purpose.
Quoted 3 lines
However, as stated previously, this resembles the PM QoS framework quite a bit to me and whatever thermal entity, say, sets these constraints, it should not work against schedutil and similar. In
But we have no way to enforce this, no? I'm thinking if frequency can be constrained in PM QoS framework, then we will end up with some drivers that think it's a good idea to use it and potentially end up breaking this "should not work against schedutil and similar".
Or did I miss something?
My point is that if we introduce something too generic we might end up encouraging more users and end up with a complex set of rules/interactions and lose some determinism. But I could be reading too much into it :-)
Cheers
--
Qais Yousef
Quoted 7 lines
some situations setting a max frequency limit to control thermals is not the most efficient way to go as it effectively turns into throttling and makes performance go south. For example, it may cause things to run at the limit frequency all the time which may be too slow and it may be more efficient to allow higher frequencies to be used, but instead control how much of the time they can be used. So we need to be careful here.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Viresh Kumar
maintainerQuoted 10 lines · Qais Yousef
But we have no way to enforce this, no? I'm thinking if frequency can be constrained in PM QoS framework, then we will end up with some drivers that think it's a good idea to use it and potentially end up breaking this "should not work against schedutil and similar".
Or did I miss something?
My point is that if we introduce something too generic we might end up encouraging more users and end up with a complex set of rules/interactions and lose some determinism. But I could be reading too much into it :-)
People are free to use notifiers today as well and there is nobody stopping them. A new framework/layer may actually make them more accountable as we can easily record which all entities have requested to impose a freq-limit on CPUs.
Signature · 1 line
viresh
Viresh Kumar
maintainerQuoted 3 lines · Juri Lelli
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).
I am not very sure about what constraints are imposed by schedutil or util-clamp stuff that may not work well with this stuff.
As you are already aware of it, this series isn't doing anything new as we already have thermal/user constraints available in kernel. I am just trying to implement a better way to present those to the cpufreq core.
Signature · 1 line
viresh
Viresh Kumar
maintainerQuoted 5 lines · Viresh Kumar
Quoted 3 lines · Juri Lelli
I was also wondering how this new framework is dealing with constraints/request imposed/generated by the scheduler and related interfaces (thinking about schedutil and Patrick's
util_clamp).
Quoted 7 lines
I am not very sure about what constraints are imposed by schedutil or util-clamp stuff that may not work well with this stuff.
As you are already aware of it, this series isn't doing anything new as we already have thermal/user constraints available in kernel. I am just trying to implement a better way to present those to the cpufreq core.
@Juri: Ping.
Signature · 1 line
viresh
Viresh Kumar
maintainerQuoted 21 lines · Rafael J. Wysocki
Quoted 20 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Quoted 8 lines
Quoted 6 lines
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
@Rafael: You are going to provide some more feedback here, right ?
Quoted 3 lines
One immediate observation is that it seems to do quite a bit of what is done in the PM QoS framework, so maybe there is an opportunity for some consolidation in there.
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerQuoted 27 lines · Viresh Kumar
Quoted 25 lines · Rafael J. Wysocki
Quoted 24 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
@Rafael: You are going to provide some more feedback here, right ?
I think I've said something already.
TLDR: I'm not convinced.
PM QoS does similar things in a similar way. Why does it have to be a different framework?
Using freq constraints for thermal reasons without coordinating it with scheduling is questionable in general, because thermal control may work against scheduling then.
AFAICS, the original reason for notifiers in cpufreq was to avoid drawing too much power (and frequency was a proxy of power then) and they allowed the firmware to set the additional limit when going from AC to battery, for example. That appears to be a different goal, though.
Viresh Kumar
maintainerQuoted 33 lines · Rafael J. Wysocki
Quoted 32 lines · Viresh Kumar
Quoted 29 lines · Rafael J. Wysocki
Quoted 20 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Quoted 6 lines
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
Quoted 8 lines
Quoted 1 line
@Rafael: You are going to provide some more feedback here, right ?
I think I've said something already.
TLDR: I'm not convinced.
PM QoS does similar things in a similar way. Why does it have to be a different framework?
I did it because no one objected to the initial proposal [1]. But you weren't directly cc'd (but only PM list) so can't blame you either :)
Maybe we can make it work with PM QoS as well, I will see that aspect.
Quoted 3 lines
Using freq constraints for thermal reasons without coordinating it with scheduling is questionable in general, because thermal control may work against scheduling then.
Sure, I agree but all we are talking about here is the mechanism with which the constraints should be put and it doesn't make things bad than they already are. With the notifiers in place currently, thermal core doesn't talk to scheduler.
I think a different set of people are trying to fix that by issuing some calls to scheduler from thermal core, or something like that but I still consider that work orthogonal to the way constraints should be added instead of notifiers.
Will implementing it the QoS way would be fine from thermal-scheduler standpoint ?
Quoted 5 lines
AFAICS, the original reason for notifiers in cpufreq was to avoid drawing too much power (and frequency was a proxy of power then) and they allowed the firmware to set the additional limit when going from AC to battery, for example. That appears to be a different goal, though.
Signature · 3 lines
viresh [1] https://marc.info/?l=linux-pm&m=153851798809709
Rafael J. Wysocki
maintainerQuoted 41 lines · Viresh Kumar
Quoted 39 lines · Rafael J. Wysocki
Quoted 27 lines · Viresh Kumar
Quoted 25 lines · Rafael J. Wysocki
Quoted 24 lines · Viresh Kumar
Hi,
This commit introduces the frequency constraint infrastructure, which provides a generic interface for parts of the kernel to constraint the working frequency range of a device.
The primary users of this are the cpufreq and devfreq frameworks. The cpufreq framework already implements such constraints with help of notifier chains (for thermal and other constraints) and some local code (for user-space constraints). The devfreq framework developers have also shown interest [1] in such a framework, which may use it at a later point of time.
The idea here is to provide a generic interface and get rid of the notifier based mechanism.
Only one constraint is added for now for the cpufreq framework and the rest will follow after this stuff is merged.
Matthias Kaehlcke was involved in the preparation of the first draft of this work and so I have added him as Co-author to the first patch. Thanks Matthias.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
FWIW, This doesn't have anything to do with the boot-constraints framework [2] I was trying to upstream earlier :)
This is quite a bit of code to review, so it will take some time.
@Rafael: You are going to provide some more feedback here, right ?
I think I've said something already.
TLDR: I'm not convinced.
Quoted 7 lines
Quoted 2 lines
PM QoS does similar things in a similar way. Why does it have to be a different framework?
I did it because no one objected to the initial proposal [1]. But you weren't directly cc'd (but only PM list) so can't blame you either :)
Maybe we can make it work with PM QoS as well, I will see that aspect.
At least some of the underlying mechanics seem to be very similar. You have priority lists, addition and removal of requests etc.
Arguably, PM QoS may be regarded as a bit overly complicated, but maybe they both can use a common library underneath?
Quoted 9 lines
Quoted 3 lines
Using freq constraints for thermal reasons without coordinating it with scheduling is questionable in general, because thermal control may work against scheduling then.
Sure, I agree but all we are talking about here is the mechanism with which the constraints should be put and it doesn't make things bad than they already are. With the notifiers in place currently, thermal core doesn't talk to scheduler.
Quoted 7 lines
I think a different set of people are trying to fix that by issuing some calls to scheduler from thermal core, or something like that but I still consider that work orthogonal to the way constraints should be added instead of notifiers.
Will implementing it the QoS way would be fine from thermal-scheduler standpoint ?
As I said I like the idea of replacing cpufreq notifiers with something nicer, so if you can avoid doing almost-the-same-ting in two different frameworks, it would be fine by me.
Viresh Kumar
maintainerQuoted 9 lines · Rafael J. Wysocki
At least some of the underlying mechanics seem to be very similar. You have priority lists, addition and removal of requests etc.
Arguably, PM QoS may be regarded as a bit overly complicated, but maybe they both can use a common library underneath?
As I said I like the idea of replacing cpufreq notifiers with something nicer, so if you can avoid doing almost-the-same-ting in two different frameworks, it would be fine by me.
Ok, will try to move to PM QoS. Thanks.
Signature · 1 line
viresh