lkml.org 
[lkml]   [2019]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only
From
Date

On 3/27/2019 5:27 PM, Lingutla Chandrasekhar wrote:
> If user updates any cpu's cpu_capacity, then the new value is going to
> be applied to all its online sibling cpus. But this need not to be correct
> always, as sibling cpus (in ARM, same micro architecture cpus) would have
> different cpu_capacity with different performance characteristics.
> So updating the user supplied cpu_capacity to all cpu siblings
s/So/So,
> is not correct.
>
> And another problem is, current code assumes that 'all cpus in a cluster
> or with same package_id (core_siblings), would have same cpu_capacity'.
> But with commit '5bdd2b3f0f8 ("arm64: topology: add support to remove
> cpu topology sibling masks")', when a cpu hotplugged out, the cpu
> information gets cleared in its sibling cpus. So user supplied
--
> cpu_capacity would be applied to only online sibling cpus at the time.
> After that, if any cpu hot plugged in, it would have different cpu_capacity

s/hot plugged/hotplugged

> than its siblings, which breaks the above assumption.
>
> So instead of mucking around the core sibling mask for user supplied
--
> value, use device-tree to set cpu capacity. And make the cpu_capacity
> node as read-only to know the assymetry between cpus in the system.
s/assymetry/asymmetry
> While at it, remove cpu_scale_mutex usage, which used for sysfs write
> protection.
>
> Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Tested-by: Quentin Perret <quentin.perret@arm.com>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Reviewed-by: Quentin Perret <quentin.perret@arm.com>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
Please fix the commit text minor comments.
otherwise , looks good.

Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
-Mukesh


>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index edfcf8d982e4..1739d7e1952a 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -7,7 +7,6 @@
> */
>
> #include <linux/acpi.h>
> -#include <linux/arch_topology.h>
> #include <linux/cpu.h>
> #include <linux/cpufreq.h>
> #include <linux/device.h>
> @@ -31,7 +30,6 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> per_cpu(freq_scale, i) = scale;
> }
>
> -static DEFINE_MUTEX(cpu_scale_mutex);
> DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
>
> void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
> @@ -51,37 +49,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
> static void update_topology_flags_workfn(struct work_struct *work);
> static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
>
> -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;
> - int i;
> - unsigned long new_capacity;
> - ssize_t ret;
> -
> - if (!count)
> - return 0;
> -
> - 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)
> - topology_set_cpu_scale(i, new_capacity);
> - mutex_unlock(&cpu_scale_mutex);
> -
> - schedule_work(&update_topology_flags_work);
> -
> - return count;
> -}
> -
> -static DEVICE_ATTR_RW(cpu_capacity);
> +static DEVICE_ATTR_RO(cpu_capacity);
>
> static int register_cpu_capacity_sysctl(void)
> {
> @@ -141,7 +109,6 @@ void topology_normalize_cpu_scale(void)
> 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]);
> @@ -151,7 +118,6 @@ void topology_normalize_cpu_scale(void)
> pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> cpu, topology_get_cpu_scale(NULL, cpu));
> }
> - mutex_unlock(&cpu_scale_mutex);
> }
>
> bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)

\
 
 \ /
  Last update: 2019-03-27 17:16    [W:0.039 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site