On Thu, Feb 09, 2017 at 09:25:22AM +0000, Juri Lelli wrote:
> arm and arm64 share lot of code relative to parsing CPU capacity
> information from DT, using that information for appropriate scaling and
> exposing a sysfs interface for chaging such values at runtime.
>
> Factorize such code in a common place (driver/base/arch_topology.c) in
> preparation for further additions.
>
> Suggested-by: Will Deacon <will.dea...@arm.com>
> Suggested-by: Mark Rutland <mark.rutl...@arm.com>
> Suggested-by: Catalin Marinas <catalin.mari...@arm.com>
> Cc: Russell King <li...@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.mari...@arm.com>
> Cc: Will Deacon <will.dea...@arm.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Signed-off-by: Juri Lelli <juri.le...@arm.com>
> ---
>
> Changes from v1:
> - keep the original GPLv2 header
> ---
> arch/arm/Kconfig | 1 +
> arch/arm/kernel/topology.c | 213 ++------------------------------------
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/topology.c | 219 +--------------------------------------
> drivers/base/Kconfig | 8 ++
> drivers/base/Makefile | 1 +
> drivers/base/arch_topology.c | 237
> +++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 257 insertions(+), 423 deletions(-)
> create mode 100644 drivers/base/arch_topology.c
Ah, so you want _me_ to maintain this, ok, I better review it...
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -339,4 +339,12 @@ config CMA_ALIGNMENT
>
> endif
>
> +config GENERIC_ARCH_TOPOLOGY
> + bool
> + help
> + Enable support for architectures common topology code: e.g., parsing
> + CPU capacity information from DT, usage of such information for
> + appropriate scaling, sysfs interface for changing capacity values at
> + runtime.
Mix of spaces and tabs :(
> +
> endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index f2816f6ff76a..397e5c344e6a 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
> 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-y += test/
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> new file mode 100644
> index 000000000000..c1dd430adad2
> --- /dev/null
> +++ b/drivers/base/arch_topology.c
> @@ -0,0 +1,237 @@
> +/*
> + * driver/base/arch_topology.c - Arch specific cpu topology information
No need to keep the filename in the file, you know what it is called :)
> + *
> + * Copyright (C) 2016, ARM Ltd.
> + * Written by: Juri Lelli, ARM Ltd.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
So, v2 only? Please be specific. Even better yet, use a SPDX header if
you want to, those are always nice.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/topology.h>
> +
> +static DEFINE_MUTEX(cpu_scale_mutex);
> +static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
> +
> +unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
Why do you have sd here? You never use it:
> +{
> + return per_cpu(cpu_scale, cpu);
See? What am I missing?
> +}
> +
> +void set_capacity_scale(unsigned int cpu, unsigned long capacity)
> +{
> + per_cpu(cpu_scale, cpu) = capacity;
> +}
> +
> +static ssize_t cpu_capacity_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct cpu *cpu = container_of(dev, struct cpu, dev);
> +
> + return sprintf(buf, "%lu\n",
> + arch_scale_cpu_capacity(NULL, cpu->dev.id));
> +}
> +
> +static ssize_t cpu_capacity_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct cpu *cpu = container_of(dev, struct cpu, dev);
> + int this_cpu = cpu->dev.id, i;
new line for:
int i;
please.
> + unsigned long new_capacity;
> + ssize_t ret;
> +
> + if (count) {
if (!count)
return 0;
then you can get on with the rest of the logic. Don't indent if you
don't have to.
> + ret = kstrtoul(buf, 0, &new_capacity);
> + if (ret)
> + return ret;
> + if (new_capacity > SCHED_CAPACITY_SCALE)
> + return -EINVAL;
> +
> + mutex_lock(&cpu_scale_mutex);
> + for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> + set_capacity_scale(i, new_capacity);
> + mutex_unlock(&cpu_scale_mutex);
> + }
> +
> + return count;
> +}
No documentation for these sysfs file? Not good :(
> +
> +static DEVICE_ATTR_RW(cpu_capacity);
> +
> +static int register_cpu_capacity_sysctl(void)
> +{
> + int i;
> + struct device *cpu;
> +
> + for_each_possible_cpu(i) {
> + cpu = get_cpu_device(i);
> + if (!cpu) {
> + pr_err("%s: too early to get CPU%d device!\n",
> + __func__, i);
What is this going to help with?
> + continue;
> + }
> + device_create_file(cpu, &dev_attr_cpu_capacity);
You realize you just raced userspace, right? Why do it this way and not
register the files when the CPU device is created/removed?
> + }
> +
> + return 0;
> +}
> +subsys_initcall(register_cpu_capacity_sysctl);
> +
> +u32 capacity_scale;
> +u32 *raw_capacity;
> +bool cap_parsing_failed;
globals? really? That's bold :(
> +
> +void normalize_cpu_capacity(void)
naming is hard, but try to put a good, descriptive, prefix on everything
you are exporting in the same file, the same prefix.
cpu_capacity_normalize()?
cpu_capacity_register_sysctl()?
and so on.
> +{
> + u64 capacity;
> + int cpu;
> +
> + if (!raw_capacity || cap_parsing_failed)
> + return;
> +
> + pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
> + mutex_lock(&cpu_scale_mutex);
> + for_each_possible_cpu(cpu) {
> + pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
> + cpu, raw_capacity[cpu]);
> + capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
> + / capacity_scale;
> + set_capacity_scale(cpu, capacity);
> + pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> + cpu, arch_scale_cpu_capacity(NULL, cpu));
> + }
> + mutex_unlock(&cpu_scale_mutex);
> +}
> +
> +int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
cpu_capacity_parse()?
thanks,
greg k-h
