[PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Lingutla Chandrasekhar]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Chandra Sekhar Lingutla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Sudeep Holla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Chandra Sekhar Lingutla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Sudeep Holla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Quentin Perret]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Sudeep Holla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Quentin Perret]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Sudeep Holla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Chandra Sekhar Lingutla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Quentin Perret]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Sudeep Holla]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Dietmar Eggemann]

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

Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

[Morten Rasmussen]

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

[PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[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.

[PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Juri Lelli]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Juri Lelli]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Quentin Perret]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Quentin Perret]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Juri Lelli]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Juri Lelli]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Quentin Perret]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Quentin Perret]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Sudeep Holla]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Sudeep Holla]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Sudeep Holla]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Sudeep Holla]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Dietmar Eggemann]

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

Re: [PATCH v1] arch_topology: Make cpu_capacity sysfs node as ready-only

[Dietmar Eggemann]

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

[PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[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.

[PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Quentin Perret]

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

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Quentin Perret]

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

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Greg KH]

On Thu, Mar 28, 2019 at 10:17:05AM +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 hotplugged 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 asymmetry 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>
> Tested-by: Quentin Perret <quentin.perret@arm.com>
> Reviewed-by: Quentin Perret <quentin.perret@arm.com>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

I see 3 "v2" patches in my queue, all different :(

Also, you need to list what changed below the --- line as the
documentation says to do so.  Please fix up and resend v3.

thanks,

greg k-h

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Greg KH]

On Thu, Mar 28, 2019 at 10:17:05AM +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 hotplugged 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 asymmetry 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>
> Tested-by: Quentin Perret <quentin.perret@arm.com>
> Reviewed-by: Quentin Perret <quentin.perret@arm.com>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

I see 3 "v2" patches in my queue, all different :(

Also, you need to list what changed below the --- line as the
documentation says to do so.  Please fix up and resend v3.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Mukesh Ojha]

Thanks for making the change suggested.

Should not this be v3.

Please add version detail properly including what changes you made in 
which version after ---,  that makes the patch easy to review.

Thanks.
Mukesh


On 3/28/2019 10:17 AM, 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 hotplugged 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 asymmetry 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>
> Tested-by: Quentin Perret <quentin.perret@arm.com>
> Reviewed-by: Quentin Perret <quentin.perret@arm.com>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index edfcf8d982e4..1739d7e1952a 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -7,7 +7,6 @@
>    */
>   
>   #include <linux/acpi.h>
> -#include <linux/arch_topology.h>
>   #include <linux/cpu.h>
>   #include <linux/cpufreq.h>
>   #include <linux/device.h>
> @@ -31,7 +30,6 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>   		per_cpu(freq_scale, i) = scale;
>   }
>   
> -static DEFINE_MUTEX(cpu_scale_mutex);
>   DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
>   
>   void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
> @@ -51,37 +49,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
>   static void update_topology_flags_workfn(struct work_struct *work);
>   static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
>   
> -static ssize_t cpu_capacity_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf,
> -				  size_t count)
> -{
> -	struct cpu *cpu = container_of(dev, struct cpu, dev);
> -	int this_cpu = cpu->dev.id;
> -	int i;
> -	unsigned long new_capacity;
> -	ssize_t ret;
> -
> -	if (!count)
> -		return 0;
> -
> -	ret = kstrtoul(buf, 0, &new_capacity);
> -	if (ret)
> -		return ret;
> -	if (new_capacity > SCHED_CAPACITY_SCALE)
> -		return -EINVAL;
> -
> -	mutex_lock(&cpu_scale_mutex);
> -	for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> -		topology_set_cpu_scale(i, new_capacity);
> -	mutex_unlock(&cpu_scale_mutex);
> -
> -	schedule_work(&update_topology_flags_work);
> -
> -	return count;
> -}
> -
> -static DEVICE_ATTR_RW(cpu_capacity);
> +static DEVICE_ATTR_RO(cpu_capacity);
>   
>   static int register_cpu_capacity_sysctl(void)
>   {
> @@ -141,7 +109,6 @@ void topology_normalize_cpu_scale(void)
>   		return;
>   
>   	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
> -	mutex_lock(&cpu_scale_mutex);
>   	for_each_possible_cpu(cpu) {
>   		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
>   			 cpu, raw_capacity[cpu]);
> @@ -151,7 +118,6 @@ void topology_normalize_cpu_scale(void)
>   		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
>   			cpu, topology_get_cpu_scale(NULL, cpu));
>   	}
> -	mutex_unlock(&cpu_scale_mutex);
>   }
>   
>   bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Mukesh Ojha]

Thanks for making the change suggested.

Should not this be v3.

Please add version detail properly including what changes you made in 
which version after ---,  that makes the patch easy to review.

Thanks.
Mukesh


On 3/28/2019 10:17 AM, 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 hotplugged 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 asymmetry 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>
> Tested-by: Quentin Perret <quentin.perret@arm.com>
> Reviewed-by: Quentin Perret <quentin.perret@arm.com>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index edfcf8d982e4..1739d7e1952a 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -7,7 +7,6 @@
>    */
>   
>   #include <linux/acpi.h>
> -#include <linux/arch_topology.h>
>   #include <linux/cpu.h>
>   #include <linux/cpufreq.h>
>   #include <linux/device.h>
> @@ -31,7 +30,6 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>   		per_cpu(freq_scale, i) = scale;
>   }
>   
> -static DEFINE_MUTEX(cpu_scale_mutex);
>   DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
>   
>   void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
> @@ -51,37 +49,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
>   static void update_topology_flags_workfn(struct work_struct *work);
>   static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
>   
> -static ssize_t cpu_capacity_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf,
> -				  size_t count)
> -{
> -	struct cpu *cpu = container_of(dev, struct cpu, dev);
> -	int this_cpu = cpu->dev.id;
> -	int i;
> -	unsigned long new_capacity;
> -	ssize_t ret;
> -
> -	if (!count)
> -		return 0;
> -
> -	ret = kstrtoul(buf, 0, &new_capacity);
> -	if (ret)
> -		return ret;
> -	if (new_capacity > SCHED_CAPACITY_SCALE)
> -		return -EINVAL;
> -
> -	mutex_lock(&cpu_scale_mutex);
> -	for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> -		topology_set_cpu_scale(i, new_capacity);
> -	mutex_unlock(&cpu_scale_mutex);
> -
> -	schedule_work(&update_topology_flags_work);
> -
> -	return count;
> -}
> -
> -static DEVICE_ATTR_RW(cpu_capacity);
> +static DEVICE_ATTR_RO(cpu_capacity);
>   
>   static int register_cpu_capacity_sysctl(void)
>   {
> @@ -141,7 +109,6 @@ void topology_normalize_cpu_scale(void)
>   		return;
>   
>   	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
> -	mutex_lock(&cpu_scale_mutex);
>   	for_each_possible_cpu(cpu) {
>   		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
>   			 cpu, raw_capacity[cpu]);
> @@ -151,7 +118,6 @@ void topology_normalize_cpu_scale(void)
>   		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
>   			cpu, topology_get_cpu_scale(NULL, cpu));
>   	}
> -	mutex_unlock(&cpu_scale_mutex);
>   }
>   
>   bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[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 hotplugged 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 asymmetry 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>
Tested-by: Quentin Perret <quentin.perret@arm.com>
Reviewed-by: Quentin Perret <quentin.perret@arm.com>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index edfcf8d982e4..1739d7e1952a 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/arch_topology.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
@@ -31,7 +30,6 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 		per_cpu(freq_scale, i) = scale;
 }
 
-static DEFINE_MUTEX(cpu_scale_mutex);
 DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
 
 void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
@@ -51,37 +49,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
 static void update_topology_flags_workfn(struct work_struct *work);
 static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
 
-static ssize_t cpu_capacity_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf,
-				  size_t count)
-{
-	struct cpu *cpu = container_of(dev, struct cpu, dev);
-	int this_cpu = cpu->dev.id;
-	int i;
-	unsigned long new_capacity;
-	ssize_t ret;
-
-	if (!count)
-		return 0;
-
-	ret = kstrtoul(buf, 0, &new_capacity);
-	if (ret)
-		return ret;
-	if (new_capacity > SCHED_CAPACITY_SCALE)
-		return -EINVAL;
-
-	mutex_lock(&cpu_scale_mutex);
-	for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
-		topology_set_cpu_scale(i, new_capacity);
-	mutex_unlock(&cpu_scale_mutex);
-
-	schedule_work(&update_topology_flags_work);
-
-	return count;
-}
-
-static DEVICE_ATTR_RW(cpu_capacity);
+static DEVICE_ATTR_RO(cpu_capacity);
 
 static int register_cpu_capacity_sysctl(void)
 {
@@ -141,7 +109,6 @@ void topology_normalize_cpu_scale(void)
 		return;
 
 	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
-	mutex_lock(&cpu_scale_mutex);
 	for_each_possible_cpu(cpu) {
 		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
 			 cpu, raw_capacity[cpu]);
@@ -151,7 +118,6 @@ void topology_normalize_cpu_scale(void)
 		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
 			cpu, topology_get_cpu_scale(NULL, cpu));
 	}
-	mutex_unlock(&cpu_scale_mutex);
 }
 
 bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
 a Linux Foundation Collaborative Project.

[PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[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 hotplugged 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 asymmetry 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>
Tested-by: Quentin Perret <quentin.perret@arm.com>
Reviewed-by: Quentin Perret <quentin.perret@arm.com>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

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

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Mukesh Ojha]

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

s/hot plugged/hotplugged

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

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


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

Re: [PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[Mukesh Ojha]

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

s/hot plugged/hotplugged

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

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


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[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>
Tested-by: Quentin Perret <quentin.perret@arm.com>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Quentin Perret <quentin.perret@arm.com>
Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index edfcf8d982e4..1739d7e1952a 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -7,7 +7,6 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/arch_topology.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
@@ -31,7 +30,6 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 		per_cpu(freq_scale, i) = scale;
 }
 
-static DEFINE_MUTEX(cpu_scale_mutex);
 DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
 
 void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
@@ -51,37 +49,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
 static void update_topology_flags_workfn(struct work_struct *work);
 static DECLARE_WORK(update_topology_flags_work, update_topology_flags_workfn);
 
-static ssize_t cpu_capacity_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf,
-				  size_t count)
-{
-	struct cpu *cpu = container_of(dev, struct cpu, dev);
-	int this_cpu = cpu->dev.id;
-	int i;
-	unsigned long new_capacity;
-	ssize_t ret;
-
-	if (!count)
-		return 0;
-
-	ret = kstrtoul(buf, 0, &new_capacity);
-	if (ret)
-		return ret;
-	if (new_capacity > SCHED_CAPACITY_SCALE)
-		return -EINVAL;
-
-	mutex_lock(&cpu_scale_mutex);
-	for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
-		topology_set_cpu_scale(i, new_capacity);
-	mutex_unlock(&cpu_scale_mutex);
-
-	schedule_work(&update_topology_flags_work);
-
-	return count;
-}
-
-static DEVICE_ATTR_RW(cpu_capacity);
+static DEVICE_ATTR_RO(cpu_capacity);
 
 static int register_cpu_capacity_sysctl(void)
 {
@@ -141,7 +109,6 @@ void topology_normalize_cpu_scale(void)
 		return;
 
 	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
-	mutex_lock(&cpu_scale_mutex);
 	for_each_possible_cpu(cpu) {
 		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
 			 cpu, raw_capacity[cpu]);
@@ -151,7 +118,6 @@ void topology_normalize_cpu_scale(void)
 		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
 			cpu, topology_get_cpu_scale(NULL, cpu));
 	}
-	mutex_unlock(&cpu_scale_mutex);
 }
 
 bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
 a Linux Foundation Collaborative Project.

[PATCH v2] arch_topology: Make cpu_capacity sysfs node as ready-only

[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>
Tested-by: Quentin Perret <quentin.perret@arm.com>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Quentin Perret <quentin.perret@arm.com>
Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

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