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

From: Quentin Perret <quentin.perret@arm.com>
To: Chandra Sekhar Lingutla <clingutla@codeaurora.org>
Cc: catalin.marinas@arm.com, will.deacon@arm.com,
	Jeremy Linton <jeremy.linton@arm.com>,
	dietmar.eggemann@arm.com, Sudeep Holla <sudeep.holla@arm.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster
Date: Tue, 5 Mar 2019 16:12:03 +0000	[thread overview]
Message-ID: <20190305161201.rccb6e43wv7ugyxs@queper01-lin> (raw)
In-Reply-To: <f5c3765a-f8a9-3b35-2c42-51430e562f5f@codeaurora.org>

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