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
--
2.7.4
This 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.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/cpufreq/cpufreq.c | 62 ++++++++++++++++++-----------------------------
include/linux/cpufreq.h | 8 ++----
2 files changed, 26 insertions(+), 44 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 63028612d011..6f66e1261b65 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index c86d6d8bdfed..62bf33aafc6c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -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;
--
2.7.4
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.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/cpufreq/Kconfig | 1 +
drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 608af20a3494..2c2842cf2734 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -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
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a8fa684f5f90..63028612d011 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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;
+ 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;
+ }
+
+ /*
+ * The notifier-chain shall be removed once all the users of
+ * CPUFREQ_ADJUST are moved to use the freq-constraints.
+ */
/* adjust if necessary - all reasons */
blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
CPUFREQ_ADJUST, new_policy);
--
2.7.4
On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
>
> 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 :)
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
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.h
diff --git a/MAINTAINERS b/MAINTAINERS
index f6fc1b9dc00b..5b0ad4956d31 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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]
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 3e63a900b330..d53eb18ab732 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -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
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 157452080f3d..7530cbfd3cf8 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -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/
diff --git a/drivers/base/freq_constraint.c b/drivers/base/freq_constraint.c
new file mode 100644
index 000000000000..91356bae1af8
--- /dev/null
+++ b/drivers/base/freq_constraint.c
@@ -0,0 +1,633 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This manages frequency constraints on devices.
+ *
+ * Copyright (C) 2019 Linaro.
+ * Viresh Kumar <[email protected]>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/freq_constraint.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+struct freq_constraint_dev {
+ struct list_head node;
+ struct device *dev;
+};
+
+struct freq_pair {
+ unsigned long min;
+ unsigned long max;
+};
+
+struct freq_constraint {
+ struct list_head node;
+ enum freq_constraint_type type;
+ struct freq_pair freq;
+};
+
+struct freq_constraints {
+ struct list_head node;
+ struct list_head devices;
+ struct list_head constraints;
+ void (*callback)(void *param);
+ void *callback_param;
+ struct kref kref;
+ struct mutex lock;
+ struct work_struct work;
+
+ /* Aggregated constraint values */
+ struct freq_pair freq;
+};
+
+enum fc_event {
+ ADD,
+ REMOVE,
+ UPDATE
+};
+
+/* List of all frequency constraints */
+static LIST_HEAD(fcs_list);
+static DEFINE_MUTEX(fc_mutex);
+
+/* Return true if aggregated constraints are updated, else false */
+static bool fcs_reevaluate(struct freq_constraints *fcs)
+{
+ struct freq_pair limits[FREQ_CONSTRAINT_MAX] = {
+ [0 ... FREQ_CONSTRAINT_MAX - 1] = {0, ULONG_MAX} };
+ struct freq_constraint *constraint;
+ unsigned long min = 0, max = ULONG_MAX;
+ bool updated = false;
+ int i;
+
+ /* Find min/max freq under each constraint type */
+ list_for_each_entry(constraint, &fcs->constraints, node) {
+ if (constraint->freq.min > limits[constraint->type].min)
+ limits[constraint->type].min = constraint->freq.min;
+
+ if (constraint->freq.max < limits[constraint->type].max)
+ limits[constraint->type].max = constraint->freq.max;
+ }
+
+ /*
+ * Resolve possible 'internal' conflicts for each constraint type,
+ * the max limit wins over the min.
+ */
+ for (i = 0; i < FREQ_CONSTRAINT_MAX; i++) {
+ if (limits[i].min > limits[i].max)
+ limits[i].min = limits[i].max;
+ }
+
+ /*
+ * Thermal constraints are always honored, adjust conflicting other
+ * constraints.
+ */
+ if (limits[FREQ_CONSTRAINT_USER].min > limits[FREQ_CONSTRAINT_THERMAL].max)
+ limits[FREQ_CONSTRAINT_USER].min = 0;
+
+ if (limits[FREQ_CONSTRAINT_USER].max < limits[FREQ_CONSTRAINT_THERMAL].min)
+ limits[FREQ_CONSTRAINT_USER].max = ULONG_MAX;
+
+ for (i = 0; i < FREQ_CONSTRAINT_MAX; i++) {
+ min = max(min, limits[i].min);
+ max = min(max, limits[i].max);
+ }
+
+ WARN_ON(min > max);
+
+ if (fcs->freq.min != min) {
+ fcs->freq.min = min;
+ updated = true;
+ }
+
+ if (fcs->freq.max != max) {
+ fcs->freq.max = max;
+ updated = true;
+ }
+
+ return updated;
+}
+
+/* Return true if aggregated constraints are updated, else false */
+static bool _fcs_update(struct freq_constraints *fcs, struct freq_pair *freq,
+ enum fc_event event)
+{
+ bool updated = false;
+
+ switch (event) {
+ case ADD:
+ if (freq->min > fcs->freq.max || freq->max < fcs->freq.min)
+ return fcs_reevaluate(fcs);
+
+ if (freq->min > fcs->freq.min) {
+ fcs->freq.min = freq->min;
+ updated = true;
+ }
+
+ if (freq->max < fcs->freq.max) {
+ fcs->freq.max = freq->max;
+ updated = true;
+ }
+
+ return updated;
+
+ case REMOVE:
+ if (freq->min == fcs->freq.min || freq->max == fcs->freq.max)
+ return fcs_reevaluate(fcs);
+
+ return false;
+
+ case UPDATE:
+ return fcs_reevaluate(fcs);
+
+ default:
+ WARN_ON(1);
+ return false;
+ }
+}
+
+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);
+ }
+
+ mutex_unlock(&fcs->lock);
+}
+
+static void fcs_work_handler(struct work_struct *work)
+{
+ struct freq_constraints *fcs = container_of(work,
+ struct freq_constraints, work);
+
+ fcs->callback(fcs->callback_param);
+}
+
+static void free_fcdev(struct freq_constraint_dev *fcdev,
+ struct freq_constraints *fcs)
+{
+ mutex_lock(&fcs->lock);
+ list_del(&fcdev->node);
+ mutex_unlock(&fcs->lock);
+
+ kfree(fcdev);
+}
+
+static struct freq_constraint_dev *alloc_fcdev(struct device *dev,
+ struct freq_constraints *fcs)
+{
+ struct freq_constraint_dev *fcdev;
+
+ fcdev = kzalloc(sizeof(*fcdev), GFP_KERNEL);
+ if (!fcdev)
+ return ERR_PTR(-ENOMEM);
+
+ fcdev->dev = dev;
+
+ mutex_lock(&fcs->lock);
+ list_add(&fcdev->node, &fcs->devices);
+ mutex_unlock(&fcs->lock);
+
+ return fcdev;
+}
+
+static struct freq_constraint_dev *find_fcdev(struct device *dev,
+ struct freq_constraints *fcs)
+{
+ struct freq_constraint_dev *fcdev;
+
+ mutex_lock(&fcs->lock);
+ list_for_each_entry(fcdev, &fcs->devices, node) {
+ if (fcdev->dev == dev) {
+ mutex_unlock(&fcs->lock);
+ return fcdev;
+ }
+ }
+ mutex_unlock(&fcs->lock);
+
+ return NULL;
+}
+
+static void free_constraint(struct freq_constraints *fcs,
+ struct freq_constraint *constraint)
+{
+ mutex_lock(&fcs->lock);
+ list_del(&constraint->node);
+ mutex_unlock(&fcs->lock);
+
+ kfree(constraint);
+}
+
+static struct freq_constraint *alloc_constraint(struct freq_constraints *fcs,
+ enum freq_constraint_type type,
+ unsigned long min_freq,
+ unsigned long max_freq)
+{
+ struct freq_constraint *constraint;
+
+ constraint = kzalloc(sizeof(*constraint), GFP_KERNEL);
+ if (!constraint)
+ return ERR_PTR(-ENOMEM);
+
+ constraint->type = type;
+ constraint->freq.min = min_freq;
+ constraint->freq.max = max_freq;
+
+ mutex_lock(&fcs->lock);
+ list_add(&constraint->node, &fcs->constraints);
+ mutex_unlock(&fcs->lock);
+
+ return constraint;
+}
+
+static void free_fcs(struct freq_constraints *fcs)
+{
+ list_del(&fcs->node);
+ mutex_destroy(&fcs->lock);
+ kfree(fcs);
+}
+
+static void fcs_kref_release(struct kref *kref)
+{
+ struct freq_constraints *fcs = container_of(kref, struct freq_constraints, kref);
+ struct freq_constraint_dev *fcdev, *temp;
+
+ WARN_ON(!list_empty(&fcs->constraints));
+
+ list_for_each_entry_safe(fcdev, temp, &fcs->devices, node)
+ free_fcdev(fcdev, fcs);
+
+ free_fcs(fcs);
+ mutex_unlock(&fc_mutex);
+}
+
+static void put_fcs(struct freq_constraints *fcs)
+{
+ kref_put_mutex(&fcs->kref, fcs_kref_release, &fc_mutex);
+}
+
+static struct freq_constraints *alloc_fcs(struct device *dev)
+{
+ struct freq_constraints *fcs;
+ struct freq_constraint_dev *fcdev;
+
+ fcs = kzalloc(sizeof(*fcs), GFP_KERNEL);
+ if (!fcs)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_init(&fcs->lock);
+ INIT_LIST_HEAD(&fcs->devices);
+ INIT_LIST_HEAD(&fcs->constraints);
+ INIT_WORK(&fcs->work, fcs_work_handler);
+ kref_init(&fcs->kref);
+
+ fcs->freq.min = 0;
+ fcs->freq.max = ULONG_MAX;
+
+ fcdev = alloc_fcdev(dev, fcs);
+ if (IS_ERR(fcdev)) {
+ free_fcs(fcs);
+ return ERR_CAST(fcdev);
+ }
+
+ mutex_lock(&fc_mutex);
+ list_add(&fcs->node, &fcs_list);
+ mutex_unlock(&fc_mutex);
+
+ return fcs;
+}
+
+static struct freq_constraints *find_fcs(struct device *dev)
+{
+ struct freq_constraints *fcs;
+
+ mutex_lock(&fc_mutex);
+ list_for_each_entry(fcs, &fcs_list, node) {
+ if (find_fcdev(dev, fcs)) {
+ kref_get(&fcs->kref);
+ mutex_unlock(&fc_mutex);
+ return fcs;
+ }
+ }
+ mutex_unlock(&fc_mutex);
+
+ return ERR_PTR(-ENODEV);
+}
+
+static struct freq_constraints *get_fcs(struct device *dev)
+{
+ struct freq_constraints *fcs;
+
+ fcs = find_fcs(dev);
+ if (!IS_ERR(fcs))
+ return fcs;
+
+ return alloc_fcs(dev);
+}
+
+struct freq_constraint *freq_constraint_add(struct device *dev,
+ enum freq_constraint_type type,
+ unsigned long min_freq,
+ unsigned long max_freq)
+{
+ struct freq_constraints *fcs;
+ struct freq_constraint *constraint;
+
+ if (!max_freq || min_freq > max_freq) {
+ dev_err(dev, "freq-constraints: Invalid min/max frequency\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ fcs = get_fcs(dev);
+ if (IS_ERR(fcs))
+ return ERR_CAST(fcs);
+
+ constraint = alloc_constraint(fcs, type, min_freq, max_freq);
+ if (IS_ERR(constraint)) {
+ put_fcs(fcs);
+ return constraint;
+ }
+
+ fcs_update(fcs, &constraint->freq, ADD);
+
+ return constraint;
+}
+EXPORT_SYMBOL_GPL(freq_constraint_add);
+
+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");
+ return;
+ }
+
+ free_constraint(fcs, constraint);
+ fcs_update(fcs, &freq, REMOVE);
+
+ /*
+ * Put the reference twice, once for the freed constraint and one for
+ * the above call to find_fcs().
+ */
+ put_fcs(fcs);
+ put_fcs(fcs);
+}
+EXPORT_SYMBOL_GPL(freq_constraint_remove);
+
+int freq_constraint_update(struct device *dev,
+ struct freq_constraint *constraint,
+ unsigned long min_freq,
+ unsigned long max_freq)
+{
+ 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");
+ return -ENODEV;
+ }
+
+ mutex_lock(&fcs->lock);
+ constraint->freq.min = min_freq;
+ constraint->freq.max = max_freq;
+ mutex_unlock(&fcs->lock);
+
+ fcs_update(fcs, &constraint->freq, UPDATE);
+
+ put_fcs(fcs);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(freq_constraint_update);
+
+int freq_constraints_get(struct device *dev, unsigned long *min_freq,
+ unsigned long *max_freq)
+{
+ struct freq_constraints *fcs;
+
+ fcs = find_fcs(dev);
+ if (IS_ERR(fcs))
+ return -ENODEV;
+
+ mutex_lock(&fcs->lock);
+ *min_freq = fcs->freq.min;
+ *max_freq = fcs->freq.max;
+ mutex_unlock(&fcs->lock);
+
+ put_fcs(fcs);
+ return 0;
+}
+
+static int set_fcs_callback(struct device *dev, struct freq_constraints *fcs,
+ void (*callback)(void *param), void *callback_param)
+{
+ if (unlikely(fcs->callback)) {
+ dev_err(dev, "freq-constraint: callback already registered\n");
+ return -EBUSY;
+ }
+
+ fcs->callback = callback;
+ fcs->callback_param = callback_param;
+ return 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;
+
+ fcs = get_fcs(dev);
+ if (IS_ERR(fcs))
+ return PTR_ERR(fcs);
+
+ mutex_lock(&fcs->lock);
+ ret = set_fcs_callback(dev, fcs, callback, callback_param);
+ mutex_unlock(&fcs->lock);
+
+ if (ret)
+ put_fcs(fcs);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(freq_constraint_set_dev_callback);
+
+/* 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;
+ }
+
+ 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");
+ }
+ mutex_unlock(&fcs->lock);
+
+ return fcs;
+}
+
+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
+ * the above call to remove_callback().
+ */
+ put_fcs(fcs);
+ put_fcs(fcs);
+}
+EXPORT_SYMBOL_GPL(freq_constraint_remove_dev_callback);
+
+#ifdef CONFIG_CPU_FREQ
+static void remove_cpumask_fcs(struct freq_constraints *fcs,
+ const struct cpumask *cpumask, int stop_cpu)
+{
+ struct device *cpu_dev;
+ int cpu;
+
+ for_each_cpu(cpu, cpumask) {
+ if (unlikely(cpu == stop_cpu))
+ return;
+
+ cpu_dev = get_cpu_device(cpu);
+ if (unlikely(!cpu_dev))
+ continue;
+
+ put_fcs(fcs);
+ }
+}
+
+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;
+
+ /* 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;
+
+ 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);
+ }
+ }
+
+ 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);
+
+ put_fcs(fcs);
+
+ return ret;
+}
+
+void freq_constraint_remove_cpumask_callback(const struct cpumask *cpumask)
+{
+ struct freq_constraints *fcs;
+ struct device *cpu_dev = NULL;
+ int cpu;
+
+ for_each_cpu(cpu, cpumask) {
+ cpu_dev = get_cpu_device(cpu);
+ if (likely(cpu_dev))
+ break;
+ }
+
+ if (!cpu_dev)
+ return;
+
+ fcs = remove_callback(cpu_dev);
+ if (IS_ERR(fcs))
+ return;
+
+ remove_cpumask_fcs(fcs, cpumask, -1);
+
+ put_fcs(fcs);
+}
+#endif /* CONFIG_CPU_FREQ */
diff --git a/include/linux/freq_constraint.h b/include/linux/freq_constraint.h
new file mode 100644
index 000000000000..628dca3ef646
--- /dev/null
+++ b/include/linux/freq_constraint.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Frequency constraints header.
+ *
+ * Copyright (C) 2019 Linaro.
+ * Viresh Kumar <[email protected]>
+ */
+#ifndef _LINUX_FREQ_CONSTRAINT_H
+#define _LINUX_FREQ_CONSTRAINT_H
+
+struct device;
+struct freq_constraint;
+
+enum freq_constraint_type {
+ FREQ_CONSTRAINT_THERMAL,
+ FREQ_CONSTRAINT_USER,
+ FREQ_CONSTRAINT_MAX
+};
+
+struct freq_constraint *freq_constraint_add(struct device *dev,
+ enum freq_constraint_type type,
+ unsigned long min_freq,
+ unsigned long max_freq);
+void freq_constraint_remove(struct device *dev,
+ struct freq_constraint *constraint);
+int freq_constraint_update(struct device *dev,
+ struct freq_constraint *constraint,
+ unsigned long min_freq,
+ unsigned long max_freq);
+
+int freq_constraint_set_dev_callback(struct device *dev,
+ void (*callback)(void *param),
+ void *callback_param);
+void freq_constraint_remove_dev_callback(struct device *dev);
+int freq_constraints_get(struct device *dev, unsigned long *min_freq,
+ unsigned long *max_freq);
+
+#ifdef CONFIG_CPU_FREQ
+int freq_constraint_set_cpumask_callback(const struct cpumask *cpumask,
+ void (*callback)(void *param),
+ void *callback_param);
+void freq_constraint_remove_cpumask_callback(const struct cpumask *cpumask);
+#endif /* CONFIG_CPU_FREQ */
+
+#endif /* _LINUX_FREQ_CONSTRAINT_H */
--
2.7.4
On 11/01/19 10:47, Rafael J. Wysocki wrote:
> On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> >
> > 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 :)
>
> 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
On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <[email protected]> wrote:
>
> On 11/01/19 10:47, Rafael J. Wysocki wrote:
> > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > >
> > > 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 :)
> >
> > 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).
My understanding is that it is orthogonal to them, like adding extra
constraints on top of them etc.
Hi Viresh,
Thanks for your work on this!
Not a complete review, more a first pass.
On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote:
> 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.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f6fc1b9dc00b..5b0ad4956d31 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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]
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 3e63a900b330..d53eb18ab732 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -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
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 157452080f3d..7530cbfd3cf8 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -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/
>
> diff --git a/drivers/base/freq_constraint.c b/drivers/base/freq_constraint.c
> new file mode 100644
> index 000000000000..91356bae1af8
> --- /dev/null
> +++ b/drivers/base/freq_constraint.c
>
> ...
>
> +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.
> +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?
> + 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/
> +int freq_constraint_update(struct device *dev,
> + struct freq_constraint *constraint,
> + unsigned long min_freq,
> + unsigned long max_freq)
> +{
> + 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
> +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?
> +/* 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;
> + }
> +
> + 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 ..."
> +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/
> +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?
> +
> + /* 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.
> +
> + 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);
> + }
> + }
> +
> + 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
On Thu, Jan 17, 2019 at 05:46:32PM -0800, Matthias Kaehlcke wrote:
> On Fri, Jan 11, 2019 at 02:48:35PM +0530, Viresh Kumar wrote:
> > 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'?
>
> > Signed-off-by: Viresh Kumar <[email protected]>
> > ---
> > drivers/cpufreq/Kconfig | 1 +
> > drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index 608af20a3494..2c2842cf2734 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -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
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index a8fa684f5f90..63028612d011 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -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;
> >
> > + 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:
Reviewed-by: Matthias Kaehlcke <[email protected]>
On Fri, Jan 11, 2019 at 02:48:35PM +0530, Viresh Kumar wrote:
> 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'?
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/cpufreq/Kconfig | 1 +
> drivers/cpufreq/cpufreq.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index 608af20a3494..2c2842cf2734 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -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
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index a8fa684f5f90..63028612d011 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -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;
>
> + 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
On 17-01-19, 17:03, Matthias Kaehlcke wrote:
> On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote:
> > +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.
> > +
> > + /* 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.
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.
> > + 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.
--
viresh
On 17/01/19 15:55, Rafael J. Wysocki wrote:
> On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <[email protected]> wrote:
> >
> > On 11/01/19 10:47, Rafael J. Wysocki wrote:
> > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > > >
> > > > 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 :)
> > >
> > > 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).
>
> 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.
On Fri, Jan 18, 2019 at 03:32:34PM +0530, Viresh Kumar wrote:
> On 17-01-19, 17:03, Matthias Kaehlcke wrote:
> > On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote:
> > > +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.
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
On Fri, Jan 18, 2019 at 1:39 PM Juri Lelli <[email protected]> wrote:
>
> On 17/01/19 15:55, Rafael J. Wysocki wrote:
> > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <[email protected]> wrote:
> > >
> > > On 11/01/19 10:47, Rafael J. Wysocki wrote:
> > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > > > >
> > > > > 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 :)
> > > >
> > > > 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).
> >
> > 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.
On 18-01-19, 14:45, Matthias Kaehlcke wrote:
> On Fri, Jan 18, 2019 at 03:32:34PM +0530, Viresh Kumar wrote:
> > On 17-01-19, 17:03, Matthias Kaehlcke wrote:
> > > On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote:
> > > > +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.
>
> 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.
Is this really that high priority stuff ? I am not sure.
--
viresh
On Tue, Jan 22, 2019 at 12:39:36PM +0530, Viresh Kumar wrote:
> On 18-01-19, 14:45, Matthias Kaehlcke wrote:
> > On Fri, Jan 18, 2019 at 03:32:34PM +0530, Viresh Kumar wrote:
> > > On 17-01-19, 17:03, Matthias Kaehlcke wrote:
> > > > On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote:
> > > > > +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.
> >
> > 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.
>
> 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.
On Mon, Jan 21, 2019 at 12:10:55PM +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 18, 2019 at 1:39 PM Juri Lelli <[email protected]> wrote:
> >
> > On 17/01/19 15:55, Rafael J. Wysocki wrote:
> > > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <[email protected]> wrote:
> > > >
> > > > On 11/01/19 10:47, Rafael J. Wysocki wrote:
> > > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > > > > >
> > > > > > 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 :)
> > > > >
> > > > > 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).
> > >
> > > 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.
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 ...
> 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.
> On Fri, Jan 18, 2019 at 1:39 PM Juri Lelli <[email protected]> wrote:
> >
> > On 17/01/19 15:55, Rafael J. Wysocki wrote:
> > > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <[email protected]> wrote:
> > > >
> > > > On 11/01/19 10:47, Rafael J. Wysocki wrote:
> > > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > > > > >
> > > > > > 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 :)
> > > > >
> > > > > 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).
> > >
> > > 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.
>
> 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
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
> 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.
On 17-01-19, 14:16, Juri Lelli wrote:
> 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.
--
viresh
On 28-01-19, 14:04, Qais Yousef wrote:
> 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.
--
viresh
On 30-01-19, 10:55, Viresh Kumar wrote:
> On 17-01-19, 14:16, Juri Lelli wrote:
> > 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.
@Juri: Ping.
--
viresh
On 11-01-19, 10:47, Rafael J. Wysocki wrote:
> On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> >
> > 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 :)
>
> 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 ?
> 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.
--
viresh
On Fri, Feb 8, 2019 at 10:09 AM Viresh Kumar <[email protected]> wrote:
>
> On 11-01-19, 10:47, Rafael J. Wysocki wrote:
> > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > >
> > > 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 :)
> >
> > 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.
On 08-02-19, 10:53, Rafael J. Wysocki wrote:
> On Fri, Feb 8, 2019 at 10:09 AM Viresh Kumar <[email protected]> wrote:
> >
> > On 11-01-19, 10:47, Rafael J. Wysocki wrote:
> > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > > >
> > > > 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 :)
> > >
> > > 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?
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.
> 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 ?
> 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
[1] https://marc.info/?l=linux-pm&m=153851798809709
On Fri, Feb 8, 2019 at 11:23 AM Viresh Kumar <[email protected]> wrote:
>
> On 08-02-19, 10:53, Rafael J. Wysocki wrote:
> > On Fri, Feb 8, 2019 at 10:09 AM Viresh Kumar <[email protected]> wrote:
> > >
> > > On 11-01-19, 10:47, Rafael J. Wysocki wrote:
> > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <[email protected]> wrote:
> > > > >
> > > > > 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 :)
> > > >
> > > > 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?
>
> 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?
> > 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 ?
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.
On 08-02-19, 11:35, Rafael J. Wysocki wrote:
> 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.
--
viresh