* [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster @ 2019-02-28 11:53 Lingutla Chandrasekhar 2019-02-28 12:19 ` Sudeep Holla 0 siblings, 1 reply; 35+ messages in thread From: Lingutla Chandrasekhar @ 2019-02-28 11:53 UTC (permalink / raw) To: will.deacon, sudeep.holla, catalin.marinas, dietmar.eggemann Cc: Lingutla Chandrasekhar, linux-arm-kernel With commit '5bdd2b3f0f8 ("arm64: topology: add support to remove cpu topology sibling masks")', when cpu hotplugged out, it resets the cpu information in its sibling CPUs. If user changes capacity of any cpu, then the new capacity applied to all online cpus in the cluster. If any hot plugged out cpu in the same cluster comes back to online, then that would have different/stale capacity value. Fix it by applying user supplied capacity to all possible cpus in the cluster. Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index edfcf8d..dadc5d8 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -61,6 +61,7 @@ static ssize_t cpu_capacity_store(struct device *dev, int i; unsigned long new_capacity; ssize_t ret; + struct cpu_topology *icpu_topo, *this_topo = &cpu_topology[this_cpu]; if (!count) return 0; @@ -72,8 +73,15 @@ static ssize_t cpu_capacity_store(struct device *dev, return -EINVAL; mutex_lock(&cpu_scale_mutex); - for_each_cpu(i, &cpu_topology[this_cpu].core_sibling) + + for_each_possible_cpu(i) { + icpu_topo = &cpu_topology[i]; + + if (icpu_topo->package_id != this_topo->package_id) + continue; topology_set_cpu_scale(i, new_capacity); + } + mutex_unlock(&cpu_scale_mutex); schedule_work(&update_topology_flags_work); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-02-28 11:53 [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster Lingutla Chandrasekhar @ 2019-02-28 12:19 ` Sudeep Holla 2019-02-28 14:38 ` Chandra Sekhar Lingutla 0 siblings, 1 reply; 35+ messages in thread From: Sudeep Holla @ 2019-02-28 12:19 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: catalin.marinas, will.deacon, dietmar.eggemann, linux-arm-kernel, Sudeep Holla On Thu, Feb 28, 2019 at 05:23:58PM +0530, Lingutla Chandrasekhar wrote: > With commit '5bdd2b3f0f8 ("arm64: topology: add support to remove cpu > topology sibling masks")', when cpu hotplugged out, it resets the cpu > information in its sibling CPUs. If user changes capacity of any cpu, > then the new capacity applied to all online cpus in the cluster. > Correct but you are now changing to apply the same to all the CPUs in the package which is wrong. > If any hot plugged out cpu in the same cluster comes back to online, > then that would have different/stale capacity value. > Why not save the value ? > Fix it by applying user supplied capacity to all possible cpus in the > cluster. > NACK for the change. It changes for all the CPUs in the package/socket. Though DT platforms have cluster ids as package ids, that's wrong and must be fixed. So you need to fix this issuw without depending on the package id. I have removed all the wrong users of the same and this is also a wrong usage. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-02-28 12:19 ` Sudeep Holla @ 2019-02-28 14:38 ` Chandra Sekhar Lingutla 2019-02-28 15:25 ` Sudeep Holla 0 siblings, 1 reply; 35+ messages in thread From: Chandra Sekhar Lingutla @ 2019-02-28 14:38 UTC (permalink / raw) To: Sudeep Holla Cc: catalin.marinas, will.deacon, dietmar.eggemann, linux-arm-kernel Hi Sudeep, On 2/28/2019 5:49 PM, Sudeep Holla wrote: > On Thu, Feb 28, 2019 at 05:23:58PM +0530, Lingutla Chandrasekhar wrote: >> With commit '5bdd2b3f0f8 ("arm64: topology: add support to remove cpu >> topology sibling masks")', when cpu hotplugged out, it resets the cpu >> information in its sibling CPUs. If user changes capacity of any cpu, >> then the new capacity applied to all online cpus in the cluster. >> > Correct but you are now changing to apply the same to all the CPUs > in the package which is wrong. > >> If any hot plugged out cpu in the same cluster comes back to online, >> then that would have different/stale capacity value. >> > Why not save the value ? Sorry, didn't get you, you mean save user supplied value ? > >> Fix it by applying user supplied capacity to all possible cpus in the >> cluster. >> > NACK for the change. It changes for all the CPUs in the package/socket. > Though DT platforms have cluster ids as package ids, that's wrong and > must be fixed. So you need to fix this issuw without depending on the > package id. I have removed all the wrong users of the same and this is > also a wrong usage. I presumed all cores with same package-id have same cpu capacity, so depended on it. I think, we can update the capacity of newly online cpu by reading its core_sibling cpu capacity. Let me know your opinion on this option ? > -- > Regards, > Sudeep -- Chandrasekhar L, QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-02-28 14:38 ` Chandra Sekhar Lingutla @ 2019-02-28 15:25 ` Sudeep Holla 2019-03-02 13:30 ` Chandra Sekhar Lingutla 0 siblings, 1 reply; 35+ messages in thread From: Sudeep Holla @ 2019-02-28 15:25 UTC (permalink / raw) To: Chandra Sekhar Lingutla Cc: catalin.marinas, will.deacon, dietmar.eggemann, linux-arm-kernel On Thu, Feb 28, 2019 at 08:08:13PM +0530, Chandra Sekhar Lingutla wrote: > Hi Sudeep, > > On 2/28/2019 5:49 PM, Sudeep Holla wrote: > > On Thu, Feb 28, 2019 at 05:23:58PM +0530, Lingutla Chandrasekhar wrote: > >> With commit '5bdd2b3f0f8 ("arm64: topology: add support to remove cpu > >> topology sibling masks")', when cpu hotplugged out, it resets the cpu > >> information in its sibling CPUs. If user changes capacity of any cpu, > >> then the new capacity applied to all online cpus in the cluster. > >> > > Correct but you are now changing to apply the same to all the CPUs > > in the package which is wrong. > > > >> If any hot plugged out cpu in the same cluster comes back to online, > >> then that would have different/stale capacity value. > >> > > Why not save the value ? > Sorry, didn't get you, you mean save user supplied value ? I meant save the last user set value and reset when CPU comes online. > > > >> Fix it by applying user supplied capacity to all possible cpus in the > >> cluster. > >> > > NACK for the change. It changes for all the CPUs in the package/socket. > > Though DT platforms have cluster ids as package ids, that's wrong and > > must be fixed. So you need to fix this issuw without depending on the > > package id. I have removed all the wrong users of the same and this is > > also a wrong usage. > > I presumed all cores with same package-id have same cpu capacity, so > depended on it. No, 1. Package is not cluster, it's the physical socket which on typical mobile systems will be the whole SoC. So it includes all the CPUs in the system. 2. How about DSU systems where CPUs can have different capacity within cluster ? > I think, we can update the capacity of newly online cpu by reading its > core_sibling cpu capacity. Will that survive scenario where all the CPUs in the so-called cluster is hot-plugged out and back in. > Let me know your opinion on this option ? > I see only solution is to save the value last set from the user somewhere if not already done and restore the same. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-02-28 15:25 ` Sudeep Holla @ 2019-03-02 13:30 ` Chandra Sekhar Lingutla 2019-03-04 18:21 ` Sudeep Holla 0 siblings, 1 reply; 35+ messages in thread From: Chandra Sekhar Lingutla @ 2019-03-02 13:30 UTC (permalink / raw) To: Sudeep Holla Cc: catalin.marinas, will.deacon, dietmar.eggemann, linux-arm-kernel On 2/28/2019 8:55 PM, Sudeep Holla wrote: > On Thu, Feb 28, 2019 at 08:08:13PM +0530, Chandra Sekhar Lingutla wrote: >> Hi Sudeep, >> >> On 2/28/2019 5:49 PM, Sudeep Holla wrote: >>> On Thu, Feb 28, 2019 at 05:23:58PM +0530, Lingutla Chandrasekhar wrote: >>>> With commit '5bdd2b3f0f8 ("arm64: topology: add support to remove cpu >>>> topology sibling masks")', when cpu hotplugged out, it resets the cpu >>>> information in its sibling CPUs. If user changes capacity of any cpu, >>>> then the new capacity applied to all online cpus in the cluster. >>>> >>> Correct but you are now changing to apply the same to all the CPUs >>> in the package which is wrong. >>> >>>> If any hot plugged out cpu in the same cluster comes back to online, >>>> then that would have different/stale capacity value. >>>> >>> Why not save the value ? >> Sorry, didn't get you, you mean save user supplied value ? > I meant save the last user set value and reset when CPU comes online. But 'cpu_capacity' for hot plugging cpu is not touched, so it would retain same value. The actual problem is, cpu_capacity_store() tries to change the capacity for all its 'sibling cpus'. Now the commit '5bdd2b3f0f8' keeps only online cpus in the sibling mask, so user supplied cpu_capacity would be applied only online sibling cpus at the time. After that, if any cpu hot plugged in, it would have different cpu_capacity than its siblings. >>>> Fix it by applying user supplied capacity to all possible cpus in the >>>> cluster. >>>> >>> NACK for the change. It changes for all the CPUs in the package/socket. >>> Though DT platforms have cluster ids as package ids, that's wrong and >>> must be fixed. So you need to fix this issuw without depending on the >>> package id. I have removed all the wrong users of the same and this is >>> also a wrong usage. >> I presumed all cores with same package-id have same cpu capacity, so >> depended on it. > No, > 1. Package is not cluster, it's the physical socket which on typical > mobile systems will be the whole SoC. So it includes all the CPUs > in the system. > > 2. How about DSU systems where CPUs can have different capacity within > cluster ? So cpus in cpu_topology->core_sibling mask would not need to have same capacity_cpu? Then i think, we should update the cpu_capacity for only requested cpu right? ex: 'echo 910 > sys/devices/system/cpu/cpu5/cpu_capacity' should be applied only to cpu5. >> I think, we can update the capacity of newly online cpu by reading its >> core_sibling cpu capacity. > Will that survive scenario where all the CPUs in the so-called cluster > is hot-plugged out and back in. > >> Let me know your opinion on this option ? >> > I see only solution is to save the value last set from the user somewhere > if not already done and restore the same. > > -- > Regards, > Sudeep -- Chandrasekhar L, -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-02 13:30 ` Chandra Sekhar Lingutla @ 2019-03-04 18:21 ` Sudeep Holla 2019-03-05 9:23 ` Quentin Perret 0 siblings, 1 reply; 35+ messages in thread From: Sudeep Holla @ 2019-03-04 18:21 UTC (permalink / raw) To: Chandra Sekhar Lingutla Cc: catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, Quentin Perret, dietmar.eggemann, linux-arm-kernel On Sat, Mar 02, 2019 at 07:00:43PM +0530, Chandra Sekhar Lingutla wrote: > > On 2/28/2019 8:55 PM, Sudeep Holla wrote: > > On Thu, Feb 28, 2019 at 08:08:13PM +0530, Chandra Sekhar Lingutla wrote: > >> Hi Sudeep, > >> > >> On 2/28/2019 5:49 PM, Sudeep Holla wrote: > >>> On Thu, Feb 28, 2019 at 05:23:58PM +0530, Lingutla Chandrasekhar wrote: > >>>> With commit '5bdd2b3f0f8 ("arm64: topology: add support to remove cpu > >>>> topology sibling masks")', when cpu hotplugged out, it resets the cpu > >>>> information in its sibling CPUs. If user changes capacity of any cpu, > >>>> then the new capacity applied to all online cpus in the cluster. > >>>> > >>> Correct but you are now changing to apply the same to all the CPUs > >>> in the package which is wrong. > >>> > >>>> If any hot plugged out cpu in the same cluster comes back to online, > >>>> then that would have different/stale capacity value. > >>>> > >>> Why not save the value ? > >> Sorry, didn't get you, you mean save user supplied value ? > > I meant save the last user set value and reset when CPU comes online. > > But 'cpu_capacity' for hot plugging cpu is not touched, so it would retain > same value. The actual problem is, cpu_capacity_store() tries to change the > capacity for all its 'sibling cpus'. Now the commit '5bdd2b3f0f8' keeps only > online cpus in the sibling mask, so user supplied cpu_capacity would be > applied only online sibling cpus at the time. After that, if any cpu hot > plugged in, it would have different cpu_capacity than its siblings. > OK, I understand the problem. What I was saying is using sibling cpus to update cpu_capacity needs to be changed. That is no longer correct with 2 scenarios I mentioned: DSU and ACPI sockets/physical package. We need to fix physical package id for DT systems but that shouldn't block this issue IMO. > >>>> Fix it by applying user supplied capacity to all possible cpus in the > >>>> cluster. > >>>> > >>> NACK for the change. It changes for all the CPUs in the package/socket. > >>> Though DT platforms have cluster ids as package ids, that's wrong and > >>> must be fixed. So you need to fix this issuw without depending on the > >>> package id. I have removed all the wrong users of the same and this is > >>> also a wrong usage. > >> I presumed all cores with same package-id have same cpu capacity, so > >> depended on it. > > No, > > 1. Package is not cluster, it's the physical socket which on typical > > mobile systems will be the whole SoC. So it includes all the CPUs > > in the system. > > > > 2. How about DSU systems where CPUs can have different capacity within > > cluster ? > So cpus in cpu_topology->core_sibling mask would not need to have same > capacity_cpu ? > Yes, it need not. DSU is simple example. Even normal heterogeneous multi-cluster single socket systems will have all the cpus in the die present in core_siblings. > Then i think, we should update the cpu_capacity for only requested cpu > right? One possible solution and a simpler one. But I am open to any better alternative if it exists/possible. > > ex: 'echo 910 > sys/devices/system/cpu/cpu5/cpu_capacity' should be applied > only to cpu5. Yes something similar. > > >> I think, we can update the capacity of newly online cpu by reading its > >> core_sibling cpu capacity. > > Will that survive scenario where all the CPUs in the so-called cluster > > is hot-plugged out and back in. > > > >> Let me know your opinion on this option ? > >> I was always under the impression that this was in debugfs and will be removed. I did mention this in one of the thread couple of months back. I was wrong and do understand the need for this on system where firmware doesn't provide this capacity value. If possible I want to drop the write capability for the sysfs. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-04 18:21 ` Sudeep Holla @ 2019-03-05 9:23 ` Quentin Perret 2019-03-05 11:13 ` Sudeep Holla 0 siblings, 1 reply; 35+ messages in thread From: Quentin Perret @ 2019-03-05 9:23 UTC (permalink / raw) To: Sudeep Holla Cc: Chandra Sekhar Lingutla, catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, dietmar.eggemann, linux-arm-kernel On Monday 04 Mar 2019 at 18:21:38 (+0000), Sudeep Holla wrote: > On Sat, Mar 02, 2019 at 07:00:43PM +0530, Chandra Sekhar Lingutla wrote: > > So cpus in cpu_topology->core_sibling mask would not need to have same > > capacity_cpu ? > > Yes, it need not. DSU is simple example. Even normal heterogeneous > multi-cluster single socket systems will have all the cpus in the die > present in core_siblings. > > > Then i think, we should update the cpu_capacity for only requested cpu > > right? > > One possible solution and a simpler one. But I am open to any better > alternative if it exists/possible. How about we update the capacity for the related_cpus of the CPUFreq policy ? This is what we're interested in here, I think, and is orthogonal to the topology stuff. And that should map fairly well to the core_sibling_mask for legacy platforms. FWIW, we already mandate something similar for EAS for example (see [1]), and I'm not sure we want to support having different uarchs in the same freq domain here either, even though strictly speaking DynamIQ doesn't forbid it. [1] https://elixir.bootlin.com/linux/latest/source/kernel/power/energy_model.c#L170 [...] > I was always under the impression that this was in debugfs and will be > removed. I did mention this in one of the thread couple of months back. > I was wrong and do understand the need for this on system where firmware > doesn't provide this capacity value. > > If possible I want to drop the write capability for the sysfs. But yes, that is even better, if at all possible. Thanks, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 9:23 ` Quentin Perret @ 2019-03-05 11:13 ` Sudeep Holla 2019-03-05 11:29 ` Quentin Perret 0 siblings, 1 reply; 35+ messages in thread From: Sudeep Holla @ 2019-03-05 11:13 UTC (permalink / raw) To: Quentin Perret Cc: Chandra Sekhar Lingutla, catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, Sudeep Holla, dietmar.eggemann, linux-arm-kernel On Tue, Mar 05, 2019 at 09:23:25AM +0000, Quentin Perret wrote: > On Monday 04 Mar 2019 at 18:21:38 (+0000), Sudeep Holla wrote: > > On Sat, Mar 02, 2019 at 07:00:43PM +0530, Chandra Sekhar Lingutla wrote: > > > So cpus in cpu_topology->core_sibling mask would not need to have same > > > capacity_cpu ? > > > > Yes, it need not. DSU is simple example. Even normal heterogeneous > > multi-cluster single socket systems will have all the cpus in the die > > present in core_siblings. > > > > > Then i think, we should update the cpu_capacity for only requested cpu > > > right? > > > > One possible solution and a simpler one. But I am open to any better > > alternative if it exists/possible. > > How about we update the capacity for the related_cpus of the CPUFreq > policy ? This is what we're interested in here, I think, and is > orthogonal to the topology stuff. And that should map fairly well to the > core_sibling_mask for legacy platforms. > While I like the idea, I am afraid that linking this to cpufreq policy may not be good. How will we deal with it on systems without CPUfreq ? > FWIW, we already mandate something similar for EAS for example > (see [1]), and I'm not sure we want to support having different uarchs > in the same freq domain here either, even though strictly speaking > DynamIQ doesn't forbid it. > Yes that dependency is other way around and topology is not optional, so it works out well. The reverse may not be that simple. > [1] https://elixir.bootlin.com/linux/latest/source/kernel/power/energy_model.c#L170 > > [...] > > > I was always under the impression that this was in debugfs and will be > > removed. I did mention this in one of the thread couple of months back. > > I was wrong and do understand the need for this on system where firmware > > doesn't provide this capacity value. > > > > If possible I want to drop the write capability for the sysfs. > > But yes, that is even better, if at all possible. > I think if there are no valid users of this, we *must* remove it. As I have pointed out in past, giving user such access will need platform knowledge. Though it's debatable topic, firmware providing this information is the only correct solution IMO. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 11:13 ` Sudeep Holla @ 2019-03-05 11:29 ` Quentin Perret 2019-03-05 11:36 ` Sudeep Holla 2019-03-06 9:48 ` [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster Dietmar Eggemann 0 siblings, 2 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-05 11:29 UTC (permalink / raw) To: Sudeep Holla Cc: Chandra Sekhar Lingutla, catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, dietmar.eggemann, linux-arm-kernel On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: [...] > While I like the idea, I am afraid that linking this to cpufreq policy > may not be good. How will we deal with it on systems without CPUfreq ? Maybe something like this ? policy = cpufreq_cpu_get(cpu); if (policy) { for_each_cpu(i, policy->related_cpus) { /* Update capacity for @i*/ } cpufreq_cpu_put(policy); } else { /* Update capacity for @cpu*/ } I think it's OK to assume per-cpu capacities w/o CPUFreq. The only case where it makes sense to 'bind' the capacity of several CPUs together is when they're part of the same perf domain, I think. If you don't know what the perf domains are, then there's nothing sensible you can do. And for the dependency, a large part of the arch_topology driver is already dependent on CPUFreq -- it registers a CPUFreq notifier on boot to re-scale the CPU capacities depending on the max freq of the various policies and so on. So the dependency is already there somehow. [...] > I think if there are no valid users of this, we *must* remove it. As I > have pointed out in past, giving user such access will need platform > knowledge. Though it's debatable topic, firmware providing this > information is the only correct solution IMO. Yeah, if nobody is using it then maybe we can just remove it. Or at least we can give it a go and if somebody complains then we can 'fix' it with something like my snippet above :-) Thanks, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 11:29 ` Quentin Perret @ 2019-03-05 11:36 ` Sudeep Holla 2019-03-05 15:53 ` Chandra Sekhar Lingutla 2019-03-06 9:48 ` [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster Dietmar Eggemann 1 sibling, 1 reply; 35+ messages in thread From: Sudeep Holla @ 2019-03-05 11:36 UTC (permalink / raw) To: Quentin Perret Cc: Chandra Sekhar Lingutla, catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, dietmar.eggemann, linux-arm-kernel On Tue, Mar 05, 2019 at 11:29:55AM +0000, Quentin Perret wrote: > On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: > [...] > > While I like the idea, I am afraid that linking this to cpufreq policy > > may not be good. How will we deal with it on systems without CPUfreq ? > > Maybe something like this ? > > policy = cpufreq_cpu_get(cpu); > if (policy) { > for_each_cpu(i, policy->related_cpus) { > /* Update capacity for @i*/ > } > cpufreq_cpu_put(policy); > } else { > /* Update capacity for @cpu*/ > } > > I think it's OK to assume per-cpu capacities w/o CPUFreq. The only > case where it makes sense to 'bind' the capacity of several CPUs > together is when they're part of the same perf domain, I think. If you > don't know what the perf domains are, then there's nothing sensible you > can do. > Makes sense. > And for the dependency, a large part of the arch_topology driver is > already dependent on CPUFreq -- it registers a CPUFreq notifier on boot > to re-scale the CPU capacities depending on the max freq of the various > policies and so on. So the dependency is already there somehow. > Sorry when I mentioned dependency, I meant absence of it needs to be dealt with. Your suggestion looks good. > [...] > > > I think if there are no valid users of this, we *must* remove it. As I > > have pointed out in past, giving user such access will need platform > > knowledge. Though it's debatable topic, firmware providing this > > information is the only correct solution IMO. > > Yeah, if nobody is using it then maybe we can just remove it. Or at > least we can give it a go and if somebody complains then we can 'fix' it > with something like my snippet above :-) > Happy to Ack code removing it ;). The argument that it can't be provided by firmware is no longer valid. We already have some dependency on DVFS data from the firmware for this to be functional correctly. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 11:36 ` Sudeep Holla @ 2019-03-05 15:53 ` Chandra Sekhar Lingutla 2019-03-05 16:12 ` Quentin Perret 2019-03-05 16:54 ` Sudeep Holla 0 siblings, 2 replies; 35+ messages in thread From: Chandra Sekhar Lingutla @ 2019-03-05 15:53 UTC (permalink / raw) To: Sudeep Holla, Quentin Perret Cc: catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, dietmar.eggemann, linux-arm-kernel On 3/5/2019 5:06 PM, Sudeep Holla wrote: > On Tue, Mar 05, 2019 at 11:29:55AM +0000, Quentin Perret wrote: >> On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: >> [...] >>> While I like the idea, I am afraid that linking this to cpufreq policy >>> may not be good. How will we deal with it on systems without CPUfreq ? >> >> Maybe something like this ? >> >> policy = cpufreq_cpu_get(cpu); >> if (policy) { >> for_each_cpu(i, policy->related_cpus) { >> /* Update capacity for @i*/ >> } >> cpufreq_cpu_put(policy); >> } else { >> /* Update capacity for @cpu*/ >> } >> >> I think it's OK to assume per-cpu capacities w/o CPUFreq. The only >> case where it makes sense to 'bind' the capacity of several CPUs >> together is when they're part of the same perf domain, I think. If you >> don't know what the perf domains are, then there's nothing sensible you >> can do. >> > > Makes sense. > >> And for the dependency, a large part of the arch_topology driver is >> already dependent on CPUFreq -- it registers a CPUFreq notifier on boot >> to re-scale the CPU capacities depending on the max freq of the various >> policies and so on. So the dependency is already there somehow. >> > > Sorry when I mentioned dependency, I meant absence of it needs to be > dealt with. Your suggestion looks good. > >> [...] >> >>> I think if there are no valid users of this, we *must* remove it. As I >>> have pointed out in past, giving user such access will need platform >>> knowledge. Though it's debatable topic, firmware providing this >>> information is the only correct solution IMO. >> >> Yeah, if nobody is using it then maybe we can just remove it. Or at >> least we can give it a go and if somebody complains then we can 'fix' it >> with something like my snippet above :-) >> > > Happy to Ack code removing it ;). The argument that it can't be provided > by firmware is no longer valid. We already have some dependency on DVFS > data from the firmware for this to be functional correctly. > If at all nobody uses it, making it as read-only (under debugfs) would be good ? or else we are ok to update patch with related_cpus ? > -- > Regards, > Sudeep > -- Chandrasekhar L, -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 15:53 ` Chandra Sekhar Lingutla @ 2019-03-05 16:12 ` Quentin Perret 2019-03-05 16:54 ` Sudeep Holla 1 sibling, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-05 16:12 UTC (permalink / raw) To: Chandra Sekhar Lingutla Cc: catalin.marinas, will.deacon, Jeremy Linton, dietmar.eggemann, Sudeep Holla, Morten Rasmussen, linux-arm-kernel On Tuesday 05 Mar 2019 at 21:23:24 (+0530), Chandra Sekhar Lingutla wrote: > > > On 3/5/2019 5:06 PM, Sudeep Holla wrote: > > On Tue, Mar 05, 2019 at 11:29:55AM +0000, Quentin Perret wrote: > >> On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: > >> [...] > >>> While I like the idea, I am afraid that linking this to cpufreq policy > >>> may not be good. How will we deal with it on systems without CPUfreq ? > >> > >> Maybe something like this ? > >> > >> policy = cpufreq_cpu_get(cpu); > >> if (policy) { > >> for_each_cpu(i, policy->related_cpus) { > >> /* Update capacity for @i*/ > >> } > >> cpufreq_cpu_put(policy); > >> } else { > >> /* Update capacity for @cpu*/ > >> } > >> > >> I think it's OK to assume per-cpu capacities w/o CPUFreq. The only > >> case where it makes sense to 'bind' the capacity of several CPUs > >> together is when they're part of the same perf domain, I think. If you > >> don't know what the perf domains are, then there's nothing sensible you > >> can do. > >> > > > > Makes sense. > > > >> And for the dependency, a large part of the arch_topology driver is > >> already dependent on CPUFreq -- it registers a CPUFreq notifier on boot > >> to re-scale the CPU capacities depending on the max freq of the various > >> policies and so on. So the dependency is already there somehow. > >> > > > > Sorry when I mentioned dependency, I meant absence of it needs to be > > dealt with. Your suggestion looks good. > > > >> [...] > >> > >>> I think if there are no valid users of this, we *must* remove it. As I > >>> have pointed out in past, giving user such access will need platform > >>> knowledge. Though it's debatable topic, firmware providing this > >>> information is the only correct solution IMO. > >> > >> Yeah, if nobody is using it then maybe we can just remove it. Or at > >> least we can give it a go and if somebody complains then we can 'fix' it > >> with something like my snippet above :-) > >> > > > > Happy to Ack code removing it ;). The argument that it can't be provided > > by firmware is no longer valid. We already have some dependency on DVFS > > data from the firmware for this to be functional correctly. > > > If at all nobody uses it, making it as read-only (under debugfs) would be good ? I'd say keep the sysfs, but make it RO. I'm aware of a few tools reading from that sysfs node so let's not break them. (None of these tools _write_ in there, though.) > or else we are ok to update patch with related_cpus ? And that we can keep as a backup solution if the above fails. Please also don't forget to CC Greg KH who maintains that file. Thanks, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 15:53 ` Chandra Sekhar Lingutla 2019-03-05 16:12 ` Quentin Perret @ 2019-03-05 16:54 ` Sudeep Holla 2019-03-06 15:22 ` Morten Rasmussen 1 sibling, 1 reply; 35+ messages in thread From: Sudeep Holla @ 2019-03-05 16:54 UTC (permalink / raw) To: Chandra Sekhar Lingutla Cc: catalin.marinas, will.deacon, Jeremy Linton, dietmar.eggemann, Quentin Perret, Morten Rasmussen, linux-arm-kernel On Tue, Mar 05, 2019 at 09:23:24PM +0530, Chandra Sekhar Lingutla wrote: > > > On 3/5/2019 5:06 PM, Sudeep Holla wrote: > > On Tue, Mar 05, 2019 at 11:29:55AM +0000, Quentin Perret wrote: > >> On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: > >> [...] > >>> While I like the idea, I am afraid that linking this to cpufreq policy > >>> may not be good. How will we deal with it on systems without CPUfreq ? > >> > >> Maybe something like this ? > >> > >> policy = cpufreq_cpu_get(cpu); > >> if (policy) { > >> for_each_cpu(i, policy->related_cpus) { > >> /* Update capacity for @i*/ > >> } > >> cpufreq_cpu_put(policy); > >> } else { > >> /* Update capacity for @cpu*/ > >> } > >> > >> I think it's OK to assume per-cpu capacities w/o CPUFreq. The only > >> case where it makes sense to 'bind' the capacity of several CPUs > >> together is when they're part of the same perf domain, I think. If you > >> don't know what the perf domains are, then there's nothing sensible you > >> can do. > >> > > > > Makes sense. > > > >> And for the dependency, a large part of the arch_topology driver is > >> already dependent on CPUFreq -- it registers a CPUFreq notifier on boot > >> to re-scale the CPU capacities depending on the max freq of the various > >> policies and so on. So the dependency is already there somehow. > >> > > > > Sorry when I mentioned dependency, I meant absence of it needs to be > > dealt with. Your suggestion looks good. > > > >> [...] > >> > >>> I think if there are no valid users of this, we *must* remove it. As I > >>> have pointed out in past, giving user such access will need platform > >>> knowledge. Though it's debatable topic, firmware providing this > >>> information is the only correct solution IMO. > >> > >> Yeah, if nobody is using it then maybe we can just remove it. Or at > >> least we can give it a go and if somebody complains then we can 'fix' it > >> with something like my snippet above :-) > >> > > > > Happy to Ack code removing it ;). The argument that it can't be provided > > by firmware is no longer valid. We already have some dependency on DVFS > > data from the firmware for this to be functional correctly. > > > If at all nobody uses it, making it as read-only (under debugfs) would be > good ? Yes, but under sysfs as it is now. Just remove write capability and make it read-only. > or else we are ok to update patch with related_cpus ? As Quentin, mentions only if anyone has objections to the above and provide valid use-case with details on how kernel can validate the data provided from the user which is the most difficult part IMO. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 16:54 ` Sudeep Holla @ 2019-03-06 15:22 ` Morten Rasmussen 2019-03-06 15:27 ` Lingutla Chandrasekhar 0 siblings, 1 reply; 35+ messages in thread From: Morten Rasmussen @ 2019-03-06 15:22 UTC (permalink / raw) To: Sudeep Holla Cc: Chandra Sekhar Lingutla, catalin.marinas, will.deacon, Jeremy Linton, Quentin Perret, dietmar.eggemann, linux-arm-kernel On Tue, Mar 05, 2019 at 04:54:31PM +0000, Sudeep Holla wrote: > On Tue, Mar 05, 2019 at 09:23:24PM +0530, Chandra Sekhar Lingutla wrote: > > > > > > On 3/5/2019 5:06 PM, Sudeep Holla wrote: > > > On Tue, Mar 05, 2019 at 11:29:55AM +0000, Quentin Perret wrote: > > >> On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: > > >> [...] > > >>> While I like the idea, I am afraid that linking this to cpufreq policy > > >>> may not be good. How will we deal with it on systems without CPUfreq ? > > >> > > >> Maybe something like this ? > > >> > > >> policy = cpufreq_cpu_get(cpu); > > >> if (policy) { > > >> for_each_cpu(i, policy->related_cpus) { > > >> /* Update capacity for @i*/ > > >> } > > >> cpufreq_cpu_put(policy); > > >> } else { > > >> /* Update capacity for @cpu*/ > > >> } > > >> > > >> I think it's OK to assume per-cpu capacities w/o CPUFreq. The only > > >> case where it makes sense to 'bind' the capacity of several CPUs > > >> together is when they're part of the same perf domain, I think. If you > > >> don't know what the perf domains are, then there's nothing sensible you > > >> can do. Moving away from using the topology masks is certainly good thing if we can't kill the write ability through sysfs. My only concern with the per-cpu approach for non-cpufreq systems is that we have to be sure that the rebuild of the sched_domain hierarchy doesn't go wrong when you have different capacities in the same group. I don't think it will but it is worth checking. > > >> > > > > > > Makes sense. > > > > > >> And for the dependency, a large part of the arch_topology driver is > > >> already dependent on CPUFreq -- it registers a CPUFreq notifier on boot > > >> to re-scale the CPU capacities depending on the max freq of the various > > >> policies and so on. So the dependency is already there somehow. > > >> > > > > > > Sorry when I mentioned dependency, I meant absence of it needs to be > > > dealt with. Your suggestion looks good. > > > > > >> [...] > > >> > > >>> I think if there are no valid users of this, we *must* remove it. As I > > >>> have pointed out in past, giving user such access will need platform > > >>> knowledge. Though it's debatable topic, firmware providing this > > >>> information is the only correct solution IMO. > > >> > > >> Yeah, if nobody is using it then maybe we can just remove it. Or at > > >> least we can give it a go and if somebody complains then we can 'fix' it > > >> with something like my snippet above :-) > > >> > > > > > > Happy to Ack code removing it ;). The argument that it can't be provided > > > by firmware is no longer valid. We already have some dependency on DVFS > > > data from the firmware for this to be functional correctly. > > > > > If at all nobody uses it, making it as read-only (under debugfs) would be > > good ? > > Yes, but under sysfs as it is now. Just remove write capability and make > it read-only. > > > or else we are ok to update patch with related_cpus ? > > As Quentin, mentions only if anyone has objections to the above and > provide valid use-case with details on how kernel can validate the data > provided from the user which is the most difficult part IMO. +1 Morten _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-06 15:22 ` Morten Rasmussen @ 2019-03-06 15:27 ` Lingutla Chandrasekhar 0 siblings, 0 replies; 35+ messages in thread From: Lingutla Chandrasekhar @ 2019-03-06 15:27 UTC (permalink / raw) To: quentin.perret, sudeep.holla, dietmar.eggemann, gregkh Cc: will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel, Lingutla Chandrasekhar 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 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 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. Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> --- drivers/base/arch_topology.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index edfcf8d..d455897 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> @@ -51,37 +50,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) { -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-06 15:27 ` Lingutla Chandrasekhar 0 siblings, 0 replies; 35+ messages in thread From: Lingutla Chandrasekhar @ 2019-03-06 15:27 UTC (permalink / raw) To: quentin.perret, sudeep.holla, dietmar.eggemann, gregkh Cc: Lingutla Chandrasekhar, catalin.marinas, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, linux-arm-kernel 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 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 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. Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> --- drivers/base/arch_topology.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index edfcf8d..d455897 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> @@ -51,37 +50,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) { -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-06 15:27 ` Lingutla Chandrasekhar @ 2019-03-07 7:28 ` Juri Lelli -1 siblings, 0 replies; 35+ messages in thread From: Juri Lelli @ 2019-03-07 7:28 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: quentin.perret, sudeep.holla, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel Hi, On 06/03/19 20:57, 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 > 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 > 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. > > Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> > --- > drivers/base/arch_topology.c | 33 +-------------------------------- > 1 file changed, 1 insertion(+), 32 deletions(-) > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index edfcf8d..d455897 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> > @@ -51,37 +50,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); There are cases in which this needs to be RW, as recently discussed https://lore.kernel.org/lkml/20181123135807.GA14964@e107155-lin/ IMHO, if the core_sibling assumption doesn't work in all cases, one should be looking into fixing it, rather than making this RO. Best, - Juri ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-07 7:28 ` Juri Lelli 0 siblings, 0 replies; 35+ messages in thread From: Juri Lelli @ 2019-03-07 7:28 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: catalin.marinas, sudeep.holla, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, quentin.perret, gregkh, dietmar.eggemann, linux-arm-kernel Hi, On 06/03/19 20:57, 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 > 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 > 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. > > Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> > --- > drivers/base/arch_topology.c | 33 +-------------------------------- > 1 file changed, 1 insertion(+), 32 deletions(-) > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index edfcf8d..d455897 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> > @@ -51,37 +50,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); There are cases in which this needs to be RW, as recently discussed https://lore.kernel.org/lkml/20181123135807.GA14964@e107155-lin/ IMHO, if the core_sibling assumption doesn't work in all cases, one should be looking into fixing it, rather than making this RO. Best, - Juri _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-07 7:28 ` Juri Lelli @ 2019-03-07 9:31 ` Quentin Perret -1 siblings, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-07 9:31 UTC (permalink / raw) To: Juri Lelli Cc: Lingutla Chandrasekhar, sudeep.holla, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel Hi Juri, On Thursday 07 Mar 2019 at 08:28:56 (+0100), Juri Lelli wrote: > There are cases in which this needs to be RW, as recently discussed > https://lore.kernel.org/lkml/20181123135807.GA14964@e107155-lin/ Yeah there's that problem when you can't fix your DT ... But I guess this is a problem for _all_ values in the DT, not just capacities right ? But these other values, I'd expected they just can't be fixed from userspace most of the time, you just have to live with sub-optimal values. So I don't find it unreasonable to do that for capacities too. > IMHO, if the core_sibling assumption doesn't work in all cases, one > should be looking into fixing it, rather than making this RO. It's just that this thing keeps causing more harm than it helps IMO. It's quite severely broken ATM, and it prevents us from assuming 'stable' capacity values in places were we'd like to do so (e.g. EAS). And I'm not aware of a single platform where this is used. So, I'm personally all for removing the write capability if we can. Thanks, Quentin ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-07 9:31 ` Quentin Perret 0 siblings, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-07 9:31 UTC (permalink / raw) To: Juri Lelli Cc: Lingutla Chandrasekhar, gregkh, sudeep.holla, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, catalin.marinas, dietmar.eggemann, linux-arm-kernel Hi Juri, On Thursday 07 Mar 2019 at 08:28:56 (+0100), Juri Lelli wrote: > There are cases in which this needs to be RW, as recently discussed > https://lore.kernel.org/lkml/20181123135807.GA14964@e107155-lin/ Yeah there's that problem when you can't fix your DT ... But I guess this is a problem for _all_ values in the DT, not just capacities right ? But these other values, I'd expected they just can't be fixed from userspace most of the time, you just have to live with sub-optimal values. So I don't find it unreasonable to do that for capacities too. > IMHO, if the core_sibling assumption doesn't work in all cases, one > should be looking into fixing it, rather than making this RO. It's just that this thing keeps causing more harm than it helps IMO. It's quite severely broken ATM, and it prevents us from assuming 'stable' capacity values in places were we'd like to do so (e.g. EAS). And I'm not aware of a single platform where this is used. So, I'm personally all for removing the write capability if we can. Thanks, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-07 9:31 ` Quentin Perret @ 2019-03-07 9:57 ` Juri Lelli -1 siblings, 0 replies; 35+ messages in thread From: Juri Lelli @ 2019-03-07 9:57 UTC (permalink / raw) To: Quentin Perret Cc: Lingutla Chandrasekhar, sudeep.holla, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel Hi, On 07/03/19 09:31, Quentin Perret wrote: > Hi Juri, > > On Thursday 07 Mar 2019 at 08:28:56 (+0100), Juri Lelli wrote: > > There are cases in which this needs to be RW, as recently discussed > > https://lore.kernel.org/lkml/20181123135807.GA14964@e107155-lin/ > > Yeah there's that problem when you can't fix your DT ... But I guess > this is a problem for _all_ values in the DT, not just capacities right ? > But these other values, I'd expected they just can't be fixed from > userspace most of the time, you just have to live with sub-optimal > values. So I don't find it unreasonable to do that for capacities too. > > > IMHO, if the core_sibling assumption doesn't work in all cases, one > > should be looking into fixing it, rather than making this RO. > > It's just that this thing keeps causing more harm than it helps IMO. > It's quite severely broken ATM, and it prevents us from assuming > 'stable' capacity values in places were we'd like to do so (e.g. EAS). > > And I'm not aware of a single platform where this is used. So, I'm > personally all for removing the write capability if we can. If people think it's best to simply make this RO, I won't be against it. Just pointed out a conversation we recently had. Guess we could also make it RW again (properly) in the future if somebody complains. Best, - Juri ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-07 9:57 ` Juri Lelli 0 siblings, 0 replies; 35+ messages in thread From: Juri Lelli @ 2019-03-07 9:57 UTC (permalink / raw) To: Quentin Perret Cc: Lingutla Chandrasekhar, gregkh, sudeep.holla, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, catalin.marinas, dietmar.eggemann, linux-arm-kernel Hi, On 07/03/19 09:31, Quentin Perret wrote: > Hi Juri, > > On Thursday 07 Mar 2019 at 08:28:56 (+0100), Juri Lelli wrote: > > There are cases in which this needs to be RW, as recently discussed > > https://lore.kernel.org/lkml/20181123135807.GA14964@e107155-lin/ > > Yeah there's that problem when you can't fix your DT ... But I guess > this is a problem for _all_ values in the DT, not just capacities right ? > But these other values, I'd expected they just can't be fixed from > userspace most of the time, you just have to live with sub-optimal > values. So I don't find it unreasonable to do that for capacities too. > > > IMHO, if the core_sibling assumption doesn't work in all cases, one > > should be looking into fixing it, rather than making this RO. > > It's just that this thing keeps causing more harm than it helps IMO. > It's quite severely broken ATM, and it prevents us from assuming > 'stable' capacity values in places were we'd like to do so (e.g. EAS). > > And I'm not aware of a single platform where this is used. So, I'm > personally all for removing the write capability if we can. If people think it's best to simply make this RO, I won't be against it. Just pointed out a conversation we recently had. Guess we could also make it RW again (properly) in the future if somebody complains. Best, - Juri _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-07 9:57 ` Juri Lelli @ 2019-03-07 12:14 ` Quentin Perret -1 siblings, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-07 12:14 UTC (permalink / raw) To: Juri Lelli Cc: Lingutla Chandrasekhar, sudeep.holla, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel On Thursday 07 Mar 2019 at 10:57:50 (+0100), Juri Lelli wrote: > If people think it's best to simply make this RO, I won't be against it. > Just pointed out a conversation we recently had. Guess we could also > make it RW again (properly) in the future if somebody complains. Right, now is probably the time to give it a go before folks start depending on it. And if I am wrong (and that happens more often than I'd like unfortunately :-)) and there are users of that thing, then the revert should be trivial. Thanks, Quentin ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-07 12:14 ` Quentin Perret 0 siblings, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-07 12:14 UTC (permalink / raw) To: Juri Lelli Cc: Lingutla Chandrasekhar, gregkh, sudeep.holla, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, catalin.marinas, dietmar.eggemann, linux-arm-kernel On Thursday 07 Mar 2019 at 10:57:50 (+0100), Juri Lelli wrote: > If people think it's best to simply make this RO, I won't be against it. > Just pointed out a conversation we recently had. Guess we could also > make it RW again (properly) in the future if somebody complains. Right, now is probably the time to give it a go before folks start depending on it. And if I am wrong (and that happens more often than I'd like unfortunately :-)) and there are users of that thing, then the revert should be trivial. Thanks, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-07 12:14 ` Quentin Perret @ 2019-03-07 15:04 ` Sudeep Holla -1 siblings, 0 replies; 35+ messages in thread From: Sudeep Holla @ 2019-03-07 15:04 UTC (permalink / raw) To: Quentin Perret Cc: Juri Lelli, Lingutla Chandrasekhar, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel On Thu, Mar 07, 2019 at 12:14:03PM +0000, Quentin Perret wrote: > On Thursday 07 Mar 2019 at 10:57:50 (+0100), Juri Lelli wrote: > > If people think it's best to simply make this RO, I won't be against it. > > Just pointed out a conversation we recently had. Guess we could also > > make it RW again (properly) in the future if somebody complains. > > Right, now is probably the time to give it a go before folks start > depending on it. And if I am wrong (and that happens more often than I'd > like unfortunately :-)) and there are users of that thing, then the > revert should be trivial. > +1 on all the points above ;)(I may also be getting things wrong here but I am not convinced that we can resolve the issue for all the ARM vendor possible combinations we may have to address) We should come up with some *magical* cpumask that we can use if we want to retain this write capability. And only way I see we can do that is using DT which in turn eliminates the need to have write capability for this sysfs. So I am going to ack the $subject patch for now. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-07 15:04 ` Sudeep Holla 0 siblings, 0 replies; 35+ messages in thread From: Sudeep Holla @ 2019-03-07 15:04 UTC (permalink / raw) To: Quentin Perret Cc: Lingutla Chandrasekhar, gregkh, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, catalin.marinas, dietmar.eggemann, linux-arm-kernel, Juri Lelli On Thu, Mar 07, 2019 at 12:14:03PM +0000, Quentin Perret wrote: > On Thursday 07 Mar 2019 at 10:57:50 (+0100), Juri Lelli wrote: > > If people think it's best to simply make this RO, I won't be against it. > > Just pointed out a conversation we recently had. Guess we could also > > make it RW again (properly) in the future if somebody complains. > > Right, now is probably the time to give it a go before folks start > depending on it. And if I am wrong (and that happens more often than I'd > like unfortunately :-)) and there are users of that thing, then the > revert should be trivial. > +1 on all the points above ;)(I may also be getting things wrong here but I am not convinced that we can resolve the issue for all the ARM vendor possible combinations we may have to address) We should come up with some *magical* cpumask that we can use if we want to retain this write capability. And only way I see we can do that is using DT which in turn eliminates the need to have write capability for this sysfs. So I am going to ack the $subject patch for now. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-06 15:27 ` Lingutla Chandrasekhar @ 2019-03-07 15:19 ` Sudeep Holla -1 siblings, 0 replies; 35+ messages in thread From: Sudeep Holla @ 2019-03-07 15:19 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: quentin.perret, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel On Wed, Mar 06, 2019 at 08:57:53PM +0530, 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 > 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 > 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. > Acked-by: Sudeep Holla <sudeep.holla@arm.com> IIRC this was added for 2 possibilities though I don't completely agree no one had any objections(including me though I wonder how/why I missed to notice it now, anyways it's too late) 1. For systems that don't provide this information via device-tree/any firmware though that's the highly recommended way. With more complex topologies in horizon, I can't think of fetching/deducing this information *correctly* in any other sane way. 2. For some sort of tuning(avoid rebuild and reboot), but that's questionable as this is not a software characteristic. It's more like deriving hardware characteristics using software experiments. So, for me, we can compare this with some hardware latencies we have like CPU idle entry/exit latencies. They are tuned but not in production kernels. So if there's a case for adding this back as write capable sysfs, I would prefer that in debugfs and this sysfs is read-only ABI. Hope that helps. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-07 15:19 ` Sudeep Holla 0 siblings, 0 replies; 35+ messages in thread From: Sudeep Holla @ 2019-03-07 15:19 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: catalin.marinas, will.deacon, linux-kernel, jeremy.linton, dietmar.eggemann, quentin.perret, gregkh, morten.rasmussen, linux-arm-kernel On Wed, Mar 06, 2019 at 08:57:53PM +0530, 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 > 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 > 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. > Acked-by: Sudeep Holla <sudeep.holla@arm.com> IIRC this was added for 2 possibilities though I don't completely agree no one had any objections(including me though I wonder how/why I missed to notice it now, anyways it's too late) 1. For systems that don't provide this information via device-tree/any firmware though that's the highly recommended way. With more complex topologies in horizon, I can't think of fetching/deducing this information *correctly* in any other sane way. 2. For some sort of tuning(avoid rebuild and reboot), but that's questionable as this is not a software characteristic. It's more like deriving hardware characteristics using software experiments. So, for me, we can compare this with some hardware latencies we have like CPU idle entry/exit latencies. They are tuned but not in production kernels. So if there's a case for adding this back as write capable sysfs, I would prefer that in debugfs and this sysfs is read-only ABI. Hope that helps. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-06 15:27 ` Lingutla Chandrasekhar @ 2019-03-08 11:45 ` Dietmar Eggemann -1 siblings, 0 replies; 35+ messages in thread From: Dietmar Eggemann @ 2019-03-08 11:45 UTC (permalink / raw) To: Lingutla Chandrasekhar, quentin.perret, sudeep.holla, gregkh Cc: will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel On 3/6/19 4:27 PM, Lingutla Chandrasekhar wrote: [...] > @@ -51,37 +50,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); Since we can't write to cpu_scale from here anymore, we could get rid of cpu_scale_mutex. topology_normalize_cpu_scale()->topology_set_cpu_scale() is now only called from: [ 0.202628] topology_normalize_cpu_scale+0x28/0x30 [ 0.207529] init_cpu_topology+0x168/0x1e8 [ 0.211644] smp_prepare_cpus+0x2c/0x108 [ 0.215585] kernel_init_freeable+0x104/0x518 [ 0.219963] kernel_init+0x18/0x110 [ 0.223469] ret_from_fork+0x10/0x1c for dts capacity-dmips-mhz properties and [ 3.130180] topology_normalize_cpu_scale.part.0+0xac/0xd0 [ 3.135619] init_cpu_capacity_callback+0x100/0x178 [ 3.140459] notifier_call_chain+0x5c/0xa0 [ 3.144522] blocking_notifier_call_chain+0x64/0x88 [ 3.149363] cpufreq_set_policy+0xd8/0x3c8 [ 3.153427] cpufreq_init_policy+0x78/0xc8 for cpufreq max frequency related adjustments to cpu capacity. The mutex was introduced for the sysfs interface here: https://lore.kernel.org/lkml/1468932048-31635-8-git-send-email-juri.lelli@arm.com > - 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) > { > Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> on Arm64 Juno with v5.0 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-08 11:45 ` Dietmar Eggemann 0 siblings, 0 replies; 35+ messages in thread From: Dietmar Eggemann @ 2019-03-08 11:45 UTC (permalink / raw) To: Lingutla Chandrasekhar, quentin.perret, sudeep.holla, gregkh Cc: catalin.marinas, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, linux-arm-kernel On 3/6/19 4:27 PM, Lingutla Chandrasekhar wrote: [...] > @@ -51,37 +50,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); Since we can't write to cpu_scale from here anymore, we could get rid of cpu_scale_mutex. topology_normalize_cpu_scale()->topology_set_cpu_scale() is now only called from: [ 0.202628] topology_normalize_cpu_scale+0x28/0x30 [ 0.207529] init_cpu_topology+0x168/0x1e8 [ 0.211644] smp_prepare_cpus+0x2c/0x108 [ 0.215585] kernel_init_freeable+0x104/0x518 [ 0.219963] kernel_init+0x18/0x110 [ 0.223469] ret_from_fork+0x10/0x1c for dts capacity-dmips-mhz properties and [ 3.130180] topology_normalize_cpu_scale.part.0+0xac/0xd0 [ 3.135619] init_cpu_capacity_callback+0x100/0x178 [ 3.140459] notifier_call_chain+0x5c/0xa0 [ 3.144522] blocking_notifier_call_chain+0x64/0x88 [ 3.149363] cpufreq_set_policy+0xd8/0x3c8 [ 3.153427] cpufreq_init_policy+0x78/0xc8 for cpufreq max frequency related adjustments to cpu capacity. The mutex was introduced for the sysfs interface here: https://lore.kernel.org/lkml/1468932048-31635-8-git-send-email-juri.lelli@arm.com > - 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) > { > Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> on Arm64 Juno with v5.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-08 11:45 ` Dietmar Eggemann @ 2019-03-08 12:38 ` Lingutla Chandrasekhar -1 siblings, 0 replies; 35+ messages in thread From: Lingutla Chandrasekhar @ 2019-03-08 12:38 UTC (permalink / raw) To: quentin.perret, sudeep.holla, dietmar.eggemann, gregkh Cc: will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel, Lingutla Chandrasekhar 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 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 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. While at it, remove cpu_scale_mutex usage, which used for sysfs write protection. Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> Acked-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> --- Changes from v1: - Removed cpu_scale_mutex usage, suggested by Dietmar Eggemann. --- drivers/base/arch_topology.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-) diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index edfcf8d..1739d7e 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) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-08 12:38 ` Lingutla Chandrasekhar 0 siblings, 0 replies; 35+ messages in thread From: Lingutla Chandrasekhar @ 2019-03-08 12:38 UTC (permalink / raw) To: quentin.perret, sudeep.holla, dietmar.eggemann, gregkh Cc: Lingutla Chandrasekhar, catalin.marinas, will.deacon, linux-kernel, jeremy.linton, morten.rasmussen, linux-arm-kernel 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 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 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. While at it, remove cpu_scale_mutex usage, which used for sysfs write protection. Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> Acked-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> --- Changes from v1: - Removed cpu_scale_mutex usage, suggested by Dietmar Eggemann. --- drivers/base/arch_topology.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-) diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index edfcf8d..1739d7e 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) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only 2019-03-08 12:38 ` Lingutla Chandrasekhar @ 2019-03-27 10:56 ` Quentin Perret -1 siblings, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-27 10:56 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: sudeep.holla, dietmar.eggemann, gregkh, will.deacon, catalin.marinas, morten.rasmussen, linux-arm-kernel, jeremy.linton, linux-kernel Hi, On Friday 08 Mar 2019 at 18:08:48 (+0530), 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 > 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 > 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. > While at it, remove cpu_scale_mutex usage, which used for sysfs write > protection. > > Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> > Acked-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> Reviewed-by: Quentin Perret <quentin.perret@arm.com> Tested-by: Quentin Perret <quentin.perret@arm.com> Thanks for doing this, Quentin ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only @ 2019-03-27 10:56 ` Quentin Perret 0 siblings, 0 replies; 35+ messages in thread From: Quentin Perret @ 2019-03-27 10:56 UTC (permalink / raw) To: Lingutla Chandrasekhar Cc: gregkh, sudeep.holla, will.deacon, linux-kernel, jeremy.linton, dietmar.eggemann, catalin.marinas, morten.rasmussen, linux-arm-kernel Hi, On Friday 08 Mar 2019 at 18:08:48 (+0530), 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 > 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 > 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. > While at it, remove cpu_scale_mutex usage, which used for sysfs write > protection. > > Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> > Acked-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org> Reviewed-by: Quentin Perret <quentin.perret@arm.com> Tested-by: Quentin Perret <quentin.perret@arm.com> Thanks for doing this, Quentin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster 2019-03-05 11:29 ` Quentin Perret 2019-03-05 11:36 ` Sudeep Holla @ 2019-03-06 9:48 ` Dietmar Eggemann 1 sibling, 0 replies; 35+ messages in thread From: Dietmar Eggemann @ 2019-03-06 9:48 UTC (permalink / raw) To: Quentin Perret, Sudeep Holla Cc: Chandra Sekhar Lingutla, catalin.marinas, will.deacon, Jeremy Linton, Morten Rasmussen, linux-arm-kernel On 3/5/19 12:29 PM, Quentin Perret wrote: > On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote: > [...] >> While I like the idea, I am afraid that linking this to cpufreq policy >> may not be good. How will we deal with it on systems without CPUfreq ? > > Maybe something like this ? > > policy = cpufreq_cpu_get(cpu); > if (policy) { > for_each_cpu(i, policy->related_cpus) { > /* Update capacity for @i*/ > } > cpufreq_cpu_put(policy); > } else { > /* Update capacity for @cpu*/ > } > > I think it's OK to assume per-cpu capacities w/o CPUFreq. The only > case where it makes sense to 'bind' the capacity of several CPUs > together is when they're part of the same perf domain, I think. If you > don't know what the perf domains are, then there's nothing sensible you > can do. > > And for the dependency, a large part of the arch_topology driver is > already dependent on CPUFreq -- it registers a CPUFreq notifier on boot > to re-scale the CPU capacities depending on the max freq of the various > policies and so on. So the dependency is already there somehow. > > [...] > >> I think if there are no valid users of this, we *must* remove it. As I >> have pointed out in past, giving user such access will need platform >> knowledge. Though it's debatable topic, firmware providing this >> information is the only correct solution IMO. > > Yeah, if nobody is using it then maybe we can just remove it. Or at > least we can give it a go and if somebody complains then we can 'fix' it > with something like my snippet above :-) +1 for dropping the write capability for this sysfs. I questioned the usefulness of the interface already in 2015: https://lkml.org/lkml/2015/12/10/324 Now in the meantime, reading the interface for all CPUs became a nice way of figuring out that we're on a system with asymmetric CPU capacities (big.LITTLE or DynamIQ). Especially for systems w/o an Energy Model. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-03-27 10:56 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-28 11:53 [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster Lingutla Chandrasekhar 2019-02-28 12:19 ` Sudeep Holla 2019-02-28 14:38 ` Chandra Sekhar Lingutla 2019-02-28 15:25 ` Sudeep Holla 2019-03-02 13:30 ` Chandra Sekhar Lingutla 2019-03-04 18:21 ` Sudeep Holla 2019-03-05 9:23 ` Quentin Perret 2019-03-05 11:13 ` Sudeep Holla 2019-03-05 11:29 ` Quentin Perret 2019-03-05 11:36 ` Sudeep Holla 2019-03-05 15:53 ` Chandra Sekhar Lingutla 2019-03-05 16:12 ` Quentin Perret 2019-03-05 16:54 ` Sudeep Holla 2019-03-06 15:22 ` Morten Rasmussen 2019-03-06 15:27 ` [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only Lingutla Chandrasekhar 2019-03-06 15:27 ` Lingutla Chandrasekhar 2019-03-07 7:28 ` Juri Lelli 2019-03-07 7:28 ` Juri Lelli 2019-03-07 9:31 ` Quentin Perret 2019-03-07 9:31 ` Quentin Perret 2019-03-07 9:57 ` Juri Lelli 2019-03-07 9:57 ` Juri Lelli 2019-03-07 12:14 ` Quentin Perret 2019-03-07 12:14 ` Quentin Perret 2019-03-07 15:04 ` Sudeep Holla 2019-03-07 15:04 ` Sudeep Holla 2019-03-07 15:19 ` Sudeep Holla 2019-03-07 15:19 ` Sudeep Holla 2019-03-08 11:45 ` Dietmar Eggemann 2019-03-08 11:45 ` Dietmar Eggemann 2019-03-08 12:38 ` [PATCH v2] " Lingutla Chandrasekhar 2019-03-08 12:38 ` Lingutla Chandrasekhar 2019-03-27 10:56 ` Quentin Perret 2019-03-27 10:56 ` Quentin Perret 2019-03-06 9:48 ` [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster Dietmar Eggemann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.