| Message ID | 1389554441-27335-4-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 12, 2014 at 07:20:40PM +0000, Mark Brown wrote: [...] > @@ -89,7 +113,7 @@ static void __init parse_cluster(struct device_node *cluster, int depth) > bool leaf = true; > bool has_cores = false; > struct device_node *c; > - static int cluster_id = 0; > + static int cluster_id; It has to be __initdata, and the line change above does not belong in this patch but patch 1. [...] > static void __init parse_dt_topology(void) > { > + const struct cpu_efficiency *cpu_eff; > struct device_node *cn; > + unsigned long min_capacity = ULONG_MAX; > + unsigned long max_capacity = 0; > + unsigned long capacity = 0; > + int cpu; > + > + __cpu_capacity = kcalloc(nr_cpu_ids, sizeof(*__cpu_capacity), > + GFP_NOWAIT); > > cn = of_find_node_by_path("/cpus"); > if (!cn) { > @@ -155,10 +219,84 @@ static void __init parse_dt_topology(void) > if (!cn) > return; > parse_cluster(cn, 0); > + > + for_each_possible_cpu(cpu) { > + const u32 *rate; > + int len; > + > + /* Too early to use cpu->of_node */ > + cn = of_get_cpu_node(cpu, NULL); > + if (!cn) { > + pr_err("Missing device node for CPU %d\n", cpu); > + continue; > + } > + > + for (cpu_eff = table_efficiency; cpu_eff->compatible; cpu_eff++) > + if (of_device_is_compatible(cn, cpu_eff->compatible)) > + break; > + > + if (cpu_eff->compatible == NULL) { > + pr_warn("%s: Unknown CPU type\n", cn->full_name); > + continue; > + } > + > + rate = of_get_property(cn, "clock-frequency", &len); > + if (!rate || len != 4) { > + pr_err("%s: Missing clock-frequency property\n", > + cn->full_name); > + continue; > + } I am wondering why we spit an error for a property that in practice is optional. Either we make it required, or we drop the error output. Actually this is not defined anywhere apart from the ePAPR, which defines this property as required, but following your attempt to standardize it for ARM, I gather it should be considered optional. If it is optional, should we really print an error ? (I know it is the same on arm32, I am questioning that code too). Lorenzo
On Mon, Jan 13, 2014 at 04:40:21PM +0000, Lorenzo Pieralisi wrote: > On Sun, Jan 12, 2014 at 07:20:40PM +0000, Mark Brown wrote: > > @@ -89,7 +113,7 @@ static void __init parse_cluster(struct device_node *cluster, int depth) > > bool leaf = true; > > bool has_cores = false; > > struct device_node *c; > > - static int cluster_id = 0; > > + static int cluster_id; > It has to be __initdata, and the line change above does not belong in > this patch but patch 1. I would really have expected static data from a function marked init to end up marked appropriately, but whatever. > > + rate = of_get_property(cn, "clock-frequency", &len); > > + if (!rate || len != 4) { > > + pr_err("%s: Missing clock-frequency property\n", > > + cn->full_name); > > + continue; > > + } > I am wondering why we spit an error for a property that in practice is > optional. Either we make it required, or we drop the error output. > Actually this is not defined anywhere apart from the ePAPR, which > defines this property as required, but following your attempt to > standardize it for ARM, I gather it should be considered optional. It's already standard in the spec we claim to be following... > If it is optional, should we really print an error ? (I know it is the > same on arm32, I am questioning that code too). For big.LITTLE systems with the current implementation this information is required in order to scale the relative performances of the cores - such a system the maximum frequencies of the cores may vary as well as their type (or indeed theoretically even only their maximum frequency). We could at some future time end up evolving the code so that this information is acquired from cpufreq or somewhere but that's something that should probably happen kernel wide as part of the scheduler work rather than going off and doing something custom.
On Mon, Jan 13, 2014 at 05:01:56PM +0000, Mark Brown wrote: > On Mon, Jan 13, 2014 at 04:40:21PM +0000, Lorenzo Pieralisi wrote: > > On Sun, Jan 12, 2014 at 07:20:40PM +0000, Mark Brown wrote: > > > > @@ -89,7 +113,7 @@ static void __init parse_cluster(struct device_node *cluster, int depth) > > > bool leaf = true; > > > bool has_cores = false; > > > struct device_node *c; > > > - static int cluster_id = 0; > > > + static int cluster_id; > > > It has to be __initdata, and the line change above does not belong in > > this patch but patch 1. > > I would really have expected static data from a function marked init to > end up marked appropriately, but whatever. I would not expect that. > > > + rate = of_get_property(cn, "clock-frequency", &len); > > > + if (!rate || len != 4) { > > > + pr_err("%s: Missing clock-frequency property\n", > > > + cn->full_name); > > > + continue; > > > + } > > > I am wondering why we spit an error for a property that in practice is > > optional. Either we make it required, or we drop the error output. > > > Actually this is not defined anywhere apart from the ePAPR, which > > defines this property as required, but following your attempt to > > standardize it for ARM, I gather it should be considered optional. > > It's already standard in the spec we claim to be following... So is it required ? > > If it is optional, should we really print an error ? (I know it is the > > same on arm32, I am questioning that code too). > > For big.LITTLE systems with the current implementation this information > is required in order to scale the relative performances of the cores - > such a system the maximum frequencies of the cores may vary as well as > their type (or indeed theoretically even only their maximum frequency). > We could at some future time end up evolving the code so that this > information is acquired from cpufreq or somewhere but that's something > that should probably happen kernel wide as part of the scheduler work > rather than going off and doing something custom. I was just referring to this thread, whose outcome is unclear to me. http://archive.arm.linux.org.uk/lurker/message/20131206.115707.24b095f4.en.html I am not questioning why it is needed, I am just asking whether it is optional or not. If it is, getting error messages in the kernel log does not seem correct. Lorenzo
On Tue, Jan 14, 2014 at 10:12:36AM +0000, Lorenzo Pieralisi wrote: > On Mon, Jan 13, 2014 at 05:01:56PM +0000, Mark Brown wrote: > > I would really have expected static data from a function marked init to > > end up marked appropriately, but whatever. > I would not expect that. Really? If something is local to a function marked init it seems like the __init ought to carry over to it. > > > > + rate = of_get_property(cn, "clock-frequency", &len); > > It's already standard in the spec we claim to be following... > So is it required ? That's what ePAPR says. If that's good decision making on the part of ePAPR or not is a separate question. > I was just referring to this thread, whose outcome is unclear to me. > http://archive.arm.linux.org.uk/lurker/message/20131206.115707.24b095f4.en.html > I am not questioning why it is needed, I am just asking whether it is > optional or not. If it is, getting error messages in the kernel log does not > seem correct. At present we don't really have a better way to get the information so we're relying on it; until the scheduler is able to talk to cpufreq not providing this information means we won't be able to provide a relative performance estimate to the scheduler. This means that we probably ought to be telling the user if we couldn't figure out the top frequency for the core.
On Tue, Jan 14, 2014 at 12:13:43PM +0000, Mark Brown wrote: > On Tue, Jan 14, 2014 at 10:12:36AM +0000, Lorenzo Pieralisi wrote: > > On Mon, Jan 13, 2014 at 05:01:56PM +0000, Mark Brown wrote: > > > > I would really have expected static data from a function marked init to > > > end up marked appropriately, but whatever. > > > I would not expect that. > > Really? If something is local to a function marked init it seems like > the __init ought to carry over to it. Yes, for the same reason as static variables declared in a function do not end up in the .text section. You want the variable to be in the .init.data section and the compiler initialize it to 0 for you. If it was not declared as __initdata it would be added to the .bss section and zeroed by the kernel. > > > > > > + rate = of_get_property(cn, "clock-frequency", &len); > > > > It's already standard in the spec we claim to be following... > > > So is it required ? > > That's what ePAPR says. If that's good decision making on the part of > ePAPR or not is a separate question. > > > I was just referring to this thread, whose outcome is unclear to me. > > > http://archive.arm.linux.org.uk/lurker/message/20131206.115707.24b095f4.en.html > > > I am not questioning why it is needed, I am just asking whether it is > > optional or not. If it is, getting error messages in the kernel log does not > > seem correct. > > At present we don't really have a better way to get the information so > we're relying on it; until the scheduler is able to talk to cpufreq not > providing this information means we won't be able to provide a relative > performance estimate to the scheduler. This means that we probably > ought to be telling the user if we couldn't figure out the top frequency > for the core. It is not the top frequency, it is the current frequency. Leaving the log is fine by me, but actually implies that the ePAPR must be followed, ie the property is "required". Lorenzo
On Tue, Jan 14, 2014 at 01:23:19PM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 14, 2014 at 12:13:43PM +0000, Mark Brown wrote: > > On Tue, Jan 14, 2014 at 10:12:36AM +0000, Lorenzo Pieralisi wrote: > > > On Mon, Jan 13, 2014 at 05:01:56PM +0000, Mark Brown wrote: > > > > I would really have expected static data from a function marked init to > > > > end up marked appropriately, but whatever. > > > I would not expect that. > > Really? If something is local to a function marked init it seems like > > the __init ought to carry over to it. > Yes, for the same reason as static variables declared in a function do > not end up in the .text section. You want the variable to be in the > .init.data section and the compiler initialize it to 0 for you. If it was not > declared as __initdata it would be added to the .bss section and zeroed > by the kernel. I understand why it might happen from an implementation point of view but still not what I would expect to happen - I'd have expected that annotations applied to a function would be able to automatically do the right thing with their data. > It is not the top frequency, it is the current frequency. Leaving the > log is fine by me, but actually implies that the ePAPR must be followed, > ie the property is "required". Tweaking the semantics there was half the point of my patch (since having the current frequency makes no sense in the context of FDT or anything else without a running firmware), the other bit was just making this more discoverable since while we say we're following ePAPR I don't think anyone actually looks at it.
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index 7ef0d783ffff..2748b252d4e7 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -19,9 +19,33 @@ #include <linux/nodemask.h> #include <linux/of.h> #include <linux/sched.h> +#include <linux/slab.h> #include <asm/topology.h> +/* + * cpu power table + * This per cpu data structure describes the relative capacity of each core. + * On a heteregenous system, cores don't have the same computation capacity + * and we reflect that difference in the cpu_power field so the scheduler can + * take this difference into account during load balance. A per cpu structure + * is preferred because each CPU updates its own cpu_power field during the + * load balance except for idle cores. One idle core is selected to run the + * rebalance_domains for all idle cores and the cpu_power can be updated + * during this sequence. + */ +static DEFINE_PER_CPU(unsigned long, cpu_scale); + +unsigned long arch_scale_freq_power(struct sched_domain *sd, int cpu) +{ + return per_cpu(cpu_scale, cpu); +} + +static void set_power_scale(unsigned int cpu, unsigned long power) +{ + per_cpu(cpu_scale, cpu) = power; +} + #ifdef CONFIG_OF static int __init get_cpu_for_node(struct device_node *node) { @@ -89,7 +113,7 @@ static void __init parse_cluster(struct device_node *cluster, int depth) bool leaf = true; bool has_cores = false; struct device_node *c; - static int cluster_id = 0; + static int cluster_id; int core_id = 0; int i; @@ -137,9 +161,49 @@ static void __init parse_cluster(struct device_node *cluster, int depth) cluster_id++; } +struct cpu_efficiency { + const char *compatible; + unsigned long efficiency; +}; + +/* + * Table of relative efficiency of each processors + * The efficiency value must fit in 20bit and the final + * cpu_scale value must be in the range + * 0 < cpu_scale < 3*SCHED_POWER_SCALE/2 + * in order to return at most 1 when DIV_ROUND_CLOSEST + * is used to compute the capacity of a CPU. + * Processors that are not defined in the table, + * use the default SCHED_POWER_SCALE value for cpu_scale. + */ +static const struct cpu_efficiency table_efficiency[] = { + { NULL, }, +}; + +static unsigned long *__cpu_capacity; +#define cpu_capacity(cpu) __cpu_capacity[cpu] + +static unsigned long middle_capacity = 1; + +/* + * Iterate all CPUs' descriptor in DT and compute the efficiency + * (as per table_efficiency). Also calculate a middle efficiency + * as close as possible to (max{eff_i} - min{eff_i}) / 2 + * This is later used to scale the cpu_power field such that an + * 'average' CPU is of middle power. Also see the comments near + * table_efficiency[] and update_cpu_power(). + */ static void __init parse_dt_topology(void) { + const struct cpu_efficiency *cpu_eff; struct device_node *cn; + unsigned long min_capacity = ULONG_MAX; + unsigned long max_capacity = 0; + unsigned long capacity = 0; + int cpu; + + __cpu_capacity = kcalloc(nr_cpu_ids, sizeof(*__cpu_capacity), + GFP_NOWAIT); cn = of_find_node_by_path("/cpus"); if (!cn) { @@ -155,10 +219,84 @@ static void __init parse_dt_topology(void) if (!cn) return; parse_cluster(cn, 0); + + for_each_possible_cpu(cpu) { + const u32 *rate; + int len; + + /* Too early to use cpu->of_node */ + cn = of_get_cpu_node(cpu, NULL); + if (!cn) { + pr_err("Missing device node for CPU %d\n", cpu); + continue; + } + + for (cpu_eff = table_efficiency; cpu_eff->compatible; cpu_eff++) + if (of_device_is_compatible(cn, cpu_eff->compatible)) + break; + + if (cpu_eff->compatible == NULL) { + pr_warn("%s: Unknown CPU type\n", cn->full_name); + continue; + } + + rate = of_get_property(cn, "clock-frequency", &len); + if (!rate || len != 4) { + pr_err("%s: Missing clock-frequency property\n", + cn->full_name); + continue; + } + + capacity = ((be32_to_cpup(rate)) >> 20) * cpu_eff->efficiency; + + /* Save min capacity of the system */ + if (capacity < min_capacity) + min_capacity = capacity; + + /* Save max capacity of the system */ + if (capacity > max_capacity) + max_capacity = capacity; + + cpu_capacity(cpu) = capacity; + } + + /* If min and max capacities are equal we bypass the update of the + * cpu_scale because all CPUs have the same capacity. Otherwise, we + * compute a middle_capacity factor that will ensure that the capacity + * of an 'average' CPU of the system will be as close as possible to + * SCHED_POWER_SCALE, which is the default value, but with the + * constraint explained near table_efficiency[]. + */ + if (min_capacity == max_capacity) + return; + else if (4 * max_capacity < (3 * (max_capacity + min_capacity))) + middle_capacity = (min_capacity + max_capacity) + >> (SCHED_POWER_SHIFT+1); + else + middle_capacity = ((max_capacity / 3) + >> (SCHED_POWER_SHIFT-1)) + 1; + +} + +/* + * Look for a customed capacity of a CPU in the cpu_topo_data table during the + * boot. The update of all CPUs is in O(n^2) for heteregeneous system but the + * function returns directly for SMP system. + */ +static void update_cpu_power(unsigned int cpu) +{ + if (!cpu_capacity(cpu)) + return; + + set_power_scale(cpu, cpu_capacity(cpu) / middle_capacity); + + pr_info("CPU%u: update cpu_power %lu\n", + cpu, arch_scale_freq_power(NULL, cpu)); } #else static inline void parse_dt_topology(void) {} +static inline void update_cpu_power(unsigned int cpuid) {} #endif /* @@ -207,6 +345,8 @@ void store_cpu_topology(unsigned int cpuid) pr_info("CPU%u: No topology information configured\n", cpuid); else update_siblings_masks(cpuid); + + update_cpu_power(cpuid); } /* @@ -226,6 +366,8 @@ void __init init_cpu_topology(void) cpu_topo->socket_id = -1; cpumask_clear(&cpu_topo->core_sibling); cpumask_clear(&cpu_topo->thread_sibling); + + set_power_scale(cpu, SCHED_POWER_SCALE); } parse_dt_topology();