[0/3] drivers: Frequency constraint infrastructure

Message ID cover.1547197612.git.viresh.kumar@linaro.org
Headers show
Series
  • drivers: Frequency constraint infrastructure
Related show

Message

Viresh Kumar Jan. 11, 2019, 9:18 a.m.
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

Comments

Juri Lelli Jan. 17, 2019, 1:16 p.m. | #1
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
Rafael J. Wysocki Jan. 17, 2019, 2:55 p.m. | #2
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.
Matthias Kaehlcke Jan. 18, 2019, 1:03 a.m. | #3
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
Juri Lelli Jan. 18, 2019, 12:39 p.m. | #4
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.
Rafael J. Wysocki Jan. 21, 2019, 11:10 a.m. | #5
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.
Viresh Kumar Jan. 22, 2019, 7:09 a.m. | #6
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
Matthias Kaehlcke Jan. 22, 2019, 5:50 p.m. | #7
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.
Matthias Kaehlcke Jan. 22, 2019, 7:30 p.m. | #8
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.
Qais Yousef Jan. 28, 2019, 2:04 p.m. | #9
> 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.
Viresh Kumar Jan. 30, 2019, 5:25 a.m. | #10
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
Viresh Kumar Jan. 30, 2019, 5:27 a.m. | #11
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
Viresh Kumar Feb. 8, 2019, 9:08 a.m. | #12
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
Viresh Kumar Feb. 8, 2019, 9:09 a.m. | #13
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
Rafael J. Wysocki Feb. 8, 2019, 9:53 a.m. | #14
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.
Viresh Kumar Feb. 8, 2019, 10:23 a.m. | #15
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
Rafael J. Wysocki Feb. 8, 2019, 10:35 a.m. | #16
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.
Viresh Kumar Feb. 11, 2019, 5:43 a.m. | #17
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