* [PATCH 0/3] drivers: Frequency constraint infrastructure @ 2019-01-11 9:18 Viresh Kumar 2019-01-11 9:18 ` [PATCH 1/3] drivers: base: Add frequency " Viresh Kumar ` (3 more replies) 0 siblings, 4 replies; 26+ messages in thread From: Viresh Kumar @ 2019-01-11 9:18 UTC (permalink / raw) To: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar Cc: Viresh Kumar, linux-pm, Vincent Guittot, mka, linux-kernel 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/20181002220625.GJ22824@google.com [2] lore.kernel.org/lkml/cover.1519380923.git.viresh.kumar@linaro.org 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] drivers: base: Add frequency constraint infrastructure 2019-01-11 9:18 [PATCH 0/3] drivers: Frequency constraint infrastructure Viresh Kumar @ 2019-01-11 9:18 ` Viresh Kumar 2019-01-18 1:03 ` Matthias Kaehlcke 2019-01-11 9:18 ` [PATCH 2/3] cpufreq: Implement freq-constraint callback Viresh Kumar ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-01-11 9:18 UTC (permalink / raw) To: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar Cc: Viresh Kumar, linux-pm, Vincent Guittot, mka, linux-kernel 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 <mka@chromium.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- 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 <vireshk@kernel.org> +L: linux-pm@vger.kernel.org +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 <konrad.wilk@oracle.com> L: linux-kernel@vger.kernel.org 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 <viresh.kumar@linaro.org> + */ + +#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 <viresh.kumar@linaro.org> + */ +#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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure 2019-01-11 9:18 ` [PATCH 1/3] drivers: base: Add frequency " Viresh Kumar @ 2019-01-18 1:03 ` Matthias Kaehlcke 2019-01-18 10:02 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Matthias Kaehlcke @ 2019-01-18 1:03 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel 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 <mka@chromium.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > 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 <vireshk@kernel.org> > +L: linux-pm@vger.kernel.org > +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 <konrad.wilk@oracle.com> > L: linux-kernel@vger.kernel.org > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure 2019-01-18 1:03 ` Matthias Kaehlcke @ 2019-01-18 10:02 ` Viresh Kumar 2019-01-18 22:45 ` Matthias Kaehlcke 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-01-18 10:02 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure 2019-01-18 10:02 ` Viresh Kumar @ 2019-01-18 22:45 ` Matthias Kaehlcke 2019-01-22 7:09 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Matthias Kaehlcke @ 2019-01-18 22:45 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure 2019-01-18 22:45 ` Matthias Kaehlcke @ 2019-01-22 7:09 ` Viresh Kumar 2019-01-22 17:50 ` Matthias Kaehlcke 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-01-22 7:09 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] drivers: base: Add frequency constraint infrastructure 2019-01-22 7:09 ` Viresh Kumar @ 2019-01-22 17:50 ` Matthias Kaehlcke 0 siblings, 0 replies; 26+ messages in thread From: Matthias Kaehlcke @ 2019-01-22 17:50 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] cpufreq: Implement freq-constraint callback 2019-01-11 9:18 [PATCH 0/3] drivers: Frequency constraint infrastructure Viresh Kumar 2019-01-11 9:18 ` [PATCH 1/3] drivers: base: Add frequency " Viresh Kumar @ 2019-01-11 9:18 ` Viresh Kumar 2019-01-18 1:46 ` Matthias Kaehlcke 2019-01-11 9:18 ` [PATCH 3/3] cpufreq: Implement USER constraint Viresh Kumar 2019-01-11 9:47 ` [PATCH 0/3] drivers: Frequency constraint infrastructure Rafael J. Wysocki 3 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-01-11 9:18 UTC (permalink / raw) To: Rafael Wysocki, Greg Kroah-Hartman Cc: Viresh Kumar, linux-pm, Vincent Guittot, mka, linux-kernel 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 <viresh.kumar@linaro.org> --- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] cpufreq: Implement freq-constraint callback 2019-01-11 9:18 ` [PATCH 2/3] cpufreq: Implement freq-constraint callback Viresh Kumar @ 2019-01-18 1:46 ` Matthias Kaehlcke 2019-01-18 1:49 ` Matthias Kaehlcke 0 siblings, 1 reply; 26+ messages in thread From: Matthias Kaehlcke @ 2019-01-18 1:46 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael Wysocki, Greg Kroah-Hartman, linux-pm, Vincent Guittot, linux-kernel 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 <viresh.kumar@linaro.org> > --- > 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] cpufreq: Implement freq-constraint callback 2019-01-18 1:46 ` Matthias Kaehlcke @ 2019-01-18 1:49 ` Matthias Kaehlcke 0 siblings, 0 replies; 26+ messages in thread From: Matthias Kaehlcke @ 2019-01-18 1:49 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael Wysocki, Greg Kroah-Hartman, linux-pm, Vincent Guittot, linux-kernel 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 <viresh.kumar@linaro.org> > > --- > > 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 <mka@chromium.org> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] cpufreq: Implement USER constraint 2019-01-11 9:18 [PATCH 0/3] drivers: Frequency constraint infrastructure Viresh Kumar 2019-01-11 9:18 ` [PATCH 1/3] drivers: base: Add frequency " Viresh Kumar 2019-01-11 9:18 ` [PATCH 2/3] cpufreq: Implement freq-constraint callback Viresh Kumar @ 2019-01-11 9:18 ` Viresh Kumar 2019-01-11 9:47 ` [PATCH 0/3] drivers: Frequency constraint infrastructure Rafael J. Wysocki 3 siblings, 0 replies; 26+ messages in thread From: Viresh Kumar @ 2019-01-11 9:18 UTC (permalink / raw) To: Rafael Wysocki, Greg Kroah-Hartman Cc: Viresh Kumar, linux-pm, Vincent Guittot, mka, linux-kernel 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 <viresh.kumar@linaro.org> --- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-11 9:18 [PATCH 0/3] drivers: Frequency constraint infrastructure Viresh Kumar ` (2 preceding siblings ...) 2019-01-11 9:18 ` [PATCH 3/3] cpufreq: Implement USER constraint Viresh Kumar @ 2019-01-11 9:47 ` Rafael J. Wysocki 2019-01-17 13:16 ` Juri Lelli 2019-02-08 9:09 ` Viresh Kumar 3 siblings, 2 replies; 26+ messages in thread From: Rafael J. Wysocki @ 2019-01-11 9:47 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael Wysocki, Greg Kroah-Hartman, Rafael J. Wysocki, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-11 9:47 ` [PATCH 0/3] drivers: Frequency constraint infrastructure Rafael J. Wysocki @ 2019-01-17 13:16 ` Juri Lelli 2019-01-17 14:55 ` Rafael J. Wysocki 2019-01-30 5:25 ` Viresh Kumar 2019-02-08 9:09 ` Viresh Kumar 1 sibling, 2 replies; 26+ messages in thread From: Juri Lelli @ 2019-01-17 13:16 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Viresh Kumar, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On 11/01/19 10:47, Rafael J. Wysocki wrote: > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-17 13:16 ` Juri Lelli @ 2019-01-17 14:55 ` Rafael J. Wysocki 2019-01-18 12:39 ` Juri Lelli 2019-01-30 5:25 ` Viresh Kumar 1 sibling, 1 reply; 26+ messages in thread From: Rafael J. Wysocki @ 2019-01-17 14:55 UTC (permalink / raw) To: Juri Lelli Cc: Rafael J. Wysocki, Viresh Kumar, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > On 11/01/19 10:47, Rafael J. Wysocki wrote: > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-17 14:55 ` Rafael J. Wysocki @ 2019-01-18 12:39 ` Juri Lelli 2019-01-21 11:10 ` Rafael J. Wysocki 0 siblings, 1 reply; 26+ messages in thread From: Juri Lelli @ 2019-01-18 12:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Viresh Kumar, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On 17/01/19 15:55, Rafael J. Wysocki wrote: > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > > > On 11/01/19 10:47, Rafael J. Wysocki wrote: > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-18 12:39 ` Juri Lelli @ 2019-01-21 11:10 ` Rafael J. Wysocki 2019-01-22 19:30 ` Matthias Kaehlcke [not found] ` <CA+mqd+7EqERei8eekAsVxa_bJUYETyO3T76L8Q_sV=C9rwiy3g@mail.gmail.com> 0 siblings, 2 replies; 26+ messages in thread From: Rafael J. Wysocki @ 2019-01-21 11:10 UTC (permalink / raw) To: Juri Lelli Cc: Rafael J. Wysocki, Viresh Kumar, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On Fri, Jan 18, 2019 at 1:39 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > On 17/01/19 15:55, Rafael J. Wysocki wrote: > > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > > > > > On 11/01/19 10:47, Rafael J. Wysocki wrote: > > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-21 11:10 ` Rafael J. Wysocki @ 2019-01-22 19:30 ` Matthias Kaehlcke [not found] ` <CA+mqd+7EqERei8eekAsVxa_bJUYETyO3T76L8Q_sV=C9rwiy3g@mail.gmail.com> 1 sibling, 0 replies; 26+ messages in thread From: Matthias Kaehlcke @ 2019-01-22 19:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Juri Lelli, Viresh Kumar, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Linux Kernel Mailing List 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 <juri.lelli@gmail.com> wrote: > > > > On 17/01/19 15:55, Rafael J. Wysocki wrote: > > > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > > > > > > > On 11/01/19 10:47, Rafael J. Wysocki wrote: > > > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <CA+mqd+7EqERei8eekAsVxa_bJUYETyO3T76L8Q_sV=C9rwiy3g@mail.gmail.com>]
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure [not found] ` <CA+mqd+7EqERei8eekAsVxa_bJUYETyO3T76L8Q_sV=C9rwiy3g@mail.gmail.com> @ 2019-01-28 14:04 ` Qais Yousef 2019-01-30 5:27 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Qais Yousef @ 2019-01-28 14:04 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Juri Lelli, Viresh Kumar, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List > On Fri, Jan 18, 2019 at 1:39 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > > > On 17/01/19 15:55, Rafael J. Wysocki wrote: > > > On Thu, Jan 17, 2019 at 2:16 PM Juri Lelli <juri.lelli@gmail.com> wrote: > > > > > > > > On 11/01/19 10:47, Rafael J. Wysocki wrote: > > > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-28 14:04 ` Qais Yousef @ 2019-01-30 5:27 ` Viresh Kumar 0 siblings, 0 replies; 26+ messages in thread From: Viresh Kumar @ 2019-01-30 5:27 UTC (permalink / raw) To: CAJZ5v0gtjq9Nh0WHbMzY+3Z3o5NWiWhP=QEXxzjVe3ta32z7=Q Cc: Rafael J. Wysocki, Juri Lelli, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-17 13:16 ` Juri Lelli 2019-01-17 14:55 ` Rafael J. Wysocki @ 2019-01-30 5:25 ` Viresh Kumar 2019-02-08 9:08 ` Viresh Kumar 1 sibling, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-01-30 5:25 UTC (permalink / raw) To: Juri Lelli Cc: Rafael J. Wysocki, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-30 5:25 ` Viresh Kumar @ 2019-02-08 9:08 ` Viresh Kumar 0 siblings, 0 replies; 26+ messages in thread From: Viresh Kumar @ 2019-02-08 9:08 UTC (permalink / raw) To: Juri Lelli Cc: Rafael J. Wysocki, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-01-11 9:47 ` [PATCH 0/3] drivers: Frequency constraint infrastructure Rafael J. Wysocki 2019-01-17 13:16 ` Juri Lelli @ 2019-02-08 9:09 ` Viresh Kumar 2019-02-08 9:53 ` Rafael J. Wysocki 1 sibling, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-02-08 9:09 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On 11-01-19, 10:47, Rafael J. Wysocki wrote: > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-02-08 9:09 ` Viresh Kumar @ 2019-02-08 9:53 ` Rafael J. Wysocki 2019-02-08 10:23 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Rafael J. Wysocki @ 2019-02-08 9:53 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael J. Wysocki, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On Fri, Feb 8, 2019 at 10:09 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 11-01-19, 10:47, Rafael J. Wysocki wrote: > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-02-08 9:53 ` Rafael J. Wysocki @ 2019-02-08 10:23 ` Viresh Kumar 2019-02-08 10:35 ` Rafael J. Wysocki 0 siblings, 1 reply; 26+ messages in thread From: Viresh Kumar @ 2019-02-08 10:23 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On 08-02-19, 10:53, Rafael J. Wysocki wrote: > On Fri, Feb 8, 2019 at 10:09 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 11-01-19, 10:47, Rafael J. Wysocki wrote: > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-02-08 10:23 ` Viresh Kumar @ 2019-02-08 10:35 ` Rafael J. Wysocki 2019-02-11 5:43 ` Viresh Kumar 0 siblings, 1 reply; 26+ messages in thread From: Rafael J. Wysocki @ 2019-02-08 10:35 UTC (permalink / raw) To: Viresh Kumar Cc: Rafael J. Wysocki, Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List On Fri, Feb 8, 2019 at 11:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 08-02-19, 10:53, Rafael J. Wysocki wrote: > > On Fri, Feb 8, 2019 at 10:09 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 11-01-19, 10:47, Rafael J. Wysocki wrote: > > > > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <viresh.kumar@linaro.org> 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] drivers: Frequency constraint infrastructure 2019-02-08 10:35 ` Rafael J. Wysocki @ 2019-02-11 5:43 ` Viresh Kumar 0 siblings, 0 replies; 26+ messages in thread From: Viresh Kumar @ 2019-02-11 5:43 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael Wysocki, Greg Kroah-Hartman, Viresh Kumar, Linux PM, Vincent Guittot, Matthias Kaehlcke, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-02-11 5:43 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-11 9:18 [PATCH 0/3] drivers: Frequency constraint infrastructure Viresh Kumar 2019-01-11 9:18 ` [PATCH 1/3] drivers: base: Add frequency " Viresh Kumar 2019-01-18 1:03 ` Matthias Kaehlcke 2019-01-18 10:02 ` Viresh Kumar 2019-01-18 22:45 ` Matthias Kaehlcke 2019-01-22 7:09 ` Viresh Kumar 2019-01-22 17:50 ` Matthias Kaehlcke 2019-01-11 9:18 ` [PATCH 2/3] cpufreq: Implement freq-constraint callback Viresh Kumar 2019-01-18 1:46 ` Matthias Kaehlcke 2019-01-18 1:49 ` Matthias Kaehlcke 2019-01-11 9:18 ` [PATCH 3/3] cpufreq: Implement USER constraint Viresh Kumar 2019-01-11 9:47 ` [PATCH 0/3] drivers: Frequency constraint infrastructure Rafael J. Wysocki 2019-01-17 13:16 ` Juri Lelli 2019-01-17 14:55 ` Rafael J. Wysocki 2019-01-18 12:39 ` Juri Lelli 2019-01-21 11:10 ` Rafael J. Wysocki 2019-01-22 19:30 ` Matthias Kaehlcke [not found] ` <CA+mqd+7EqERei8eekAsVxa_bJUYETyO3T76L8Q_sV=C9rwiy3g@mail.gmail.com> 2019-01-28 14:04 ` Qais Yousef 2019-01-30 5:27 ` Viresh Kumar 2019-01-30 5:25 ` Viresh Kumar 2019-02-08 9:08 ` Viresh Kumar 2019-02-08 9:09 ` Viresh Kumar 2019-02-08 9:53 ` Rafael J. Wysocki 2019-02-08 10:23 ` Viresh Kumar 2019-02-08 10:35 ` Rafael J. Wysocki 2019-02-11 5:43 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).