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

From: Sudeep Holla <sudeep.holla@arm.com>
To: Chandra Sekhar Lingutla <clingutla@codeaurora.org>
Cc: catalin.marinas@arm.com, will.deacon@arm.com,
	Jeremy Linton <jeremy.linton@arm.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>,
	Quentin Perret <Quentin.Perret@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: Mon, 4 Mar 2019 18:21:38 +0000	[thread overview]
Message-ID: <20190304182138.GA7553@e107155-lin> (raw)
In-Reply-To: <5785fd91-3d76-78e6-f4c5-7ed07be1a14d@codeaurora.org>

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