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

From: Chandra Sekhar Lingutla <clingutla@codeaurora.org>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: catalin.marinas@arm.com, will.deacon@arm.com,
	dietmar.eggemann@arm.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster
Date: Sat, 2 Mar 2019 19:00:43 +0530	[thread overview]
Message-ID: <5785fd91-3d76-78e6-f4c5-7ed07be1a14d@codeaurora.org> (raw)
In-Reply-To: <20190228152555.GA13165@e107155-lin>

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