Message-ID: <20190118010305.GX261387@google.com>
Date:   Thu, 17 Jan 2019 17:03:05 -0800
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Viresh Kumar <vireshk@...nel.org>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] drivers: base: Add frequency constraint
 infrastructure

Hi Viresh,

Thanks for your work on this!

Not a complete review, more a first pass.

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@...omium.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.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@...nel.org>
> +L:	linux-pm@...r.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@...cle.com>
>  L:	linux-kernel@...r.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

Powered by blists - more mailing lists