| Message ID | cover.1547197612.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | drivers: Frequency constraint infrastructure | expand |
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
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.
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
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.
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.
On 18-01-19, 14:45, Matthias Kaehlcke wrote: > On Fri, Jan 18, 2019 at 03:32:34PM +0530, Viresh Kumar wrote: > > On 17-01-19, 17:03, Matthias Kaehlcke wrote: > > > On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote: > > > > +static void fcs_update(struct freq_constraints *fcs, struct freq_pair *freq, > > > > + enum fc_event event) > > > > +{ > > > > + mutex_lock(&fcs->lock); > > > > + > > > > + if (_fcs_update(fcs, freq, event)) { > > > > + if (fcs->callback) > > > > + schedule_work(&fcs->work); > > > > > > IIUC the constraints aren't applied until the callback is executed. I > > > wonder if a dedicated workqueue should be used instead of the system > > > one, to avoid longer delays from other kernel entities that might > > > 'misbehave'. Especially for thermal constraints we want a quick > > > response. > > > > I thought the system workqueue should be fast enough, it contains > > multiple threads which can all run in parallel and service this work. > > Ok, I was still stuck at the old one thread per CPU model, where a > slow work would block other items in the same workqueue until it > finishes execution. After reading a bit through > Documentation/core-api/workqueue.rst I agree that a system workqueue > is probably fast enough. It might be warranted though to use > system_highpri_wq here. Is this really that high priority stuff ? I am not sure. -- viresh
On Tue, Jan 22, 2019 at 12:39:36PM +0530, Viresh Kumar wrote: > On 18-01-19, 14:45, Matthias Kaehlcke wrote: > > On Fri, Jan 18, 2019 at 03:32:34PM +0530, Viresh Kumar wrote: > > > On 17-01-19, 17:03, Matthias Kaehlcke wrote: > > > > On Fri, Jan 11, 2019 at 02:48:34PM +0530, Viresh Kumar wrote: > > > > > +static void fcs_update(struct freq_constraints *fcs, struct freq_pair *freq, > > > > > + enum fc_event event) > > > > > +{ > > > > > + mutex_lock(&fcs->lock); > > > > > + > > > > > + if (_fcs_update(fcs, freq, event)) { > > > > > + if (fcs->callback) > > > > > + schedule_work(&fcs->work); > > > > > > > > IIUC the constraints aren't applied until the callback is executed. I > > > > wonder if a dedicated workqueue should be used instead of the system > > > > one, to avoid longer delays from other kernel entities that might > > > > 'misbehave'. Especially for thermal constraints we want a quick > > > > response. > > > > > > I thought the system workqueue should be fast enough, it contains > > > multiple threads which can all run in parallel and service this work. > > > > Ok, I was still stuck at the old one thread per CPU model, where a > > slow work would block other items in the same workqueue until it > > finishes execution. After reading a bit through > > Documentation/core-api/workqueue.rst I agree that a system workqueue > > is probably fast enough. It might be warranted though to use > > system_highpri_wq here. > > Is this really that high priority stuff ? I am not sure. In terms of thermal it could be. But then again, thermal throttling is driven by input from thermal sensors, which often are polled with periods >= 100 ms rather than being interrupt driven, so the type of workqueue wouldn't make a major difference here. I now think it should be fine to use the normal workqueue unless problems are reported.
On Mon, Jan 21, 2019 at 12:10:55PM +0100, Rafael J. Wysocki wrote: > On Fri, Jan 18, 2019 at 1:39 PM Juri Lelli <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.
> 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.
On 17-01-19, 14:16, Juri Lelli wrote: > I was also wondering how this new framework is dealing with > constraints/request imposed/generated by the scheduler and related > interfaces (thinking about schedutil and Patrick's util_clamp). I am not very sure about what constraints are imposed by schedutil or util-clamp stuff that may not work well with this stuff. As you are already aware of it, this series isn't doing anything new as we already have thermal/user constraints available in kernel. I am just trying to implement a better way to present those to the cpufreq core. -- viresh
On 28-01-19, 14:04, Qais Yousef wrote: > But we have no way to enforce this, no? I'm thinking if frequency can be > constrained in PM QoS framework, then we will end up with some drivers that > think it's a good idea to use it and potentially end up breaking this "should > not work against schedutil and similar". > > Or did I miss something? > > My point is that if we introduce something too generic we might end up > encouraging more users and end up with a complex set of rules/interactions and > lose some determinism. But I could be reading too much into it :-) People are free to use notifiers today as well and there is nobody stopping them. A new framework/layer may actually make them more accountable as we can easily record which all entities have requested to impose a freq-limit on CPUs. -- viresh
On 30-01-19, 10:55, Viresh Kumar wrote: > On 17-01-19, 14:16, Juri Lelli wrote: > > I was also wondering how this new framework is dealing with > > constraints/request imposed/generated by the scheduler and related > > interfaces (thinking about schedutil and Patrick's util_clamp). > > I am not very sure about what constraints are imposed by schedutil or > util-clamp stuff that may not work well with this stuff. > > As you are already aware of it, this series isn't doing anything new > as we already have thermal/user constraints available in kernel. I am > just trying to implement a better way to present those to the cpufreq > core. @Juri: Ping. -- viresh
On 11-01-19, 10:47, Rafael J. Wysocki wrote: > On Fri, Jan 11, 2019 at 10:18 AM Viresh Kumar <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
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.
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
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.
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