On Tue, Dec 04, 2018 at 09:43:53PM -0800, Alison Schofield wrote:

On Tue, Dec 04, 2018 at 10:21:45AM +0100, Peter Zijlstra wrote:

quoted

On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote:

quoted

+static int mktme_build_leadcpus_mask(void)
+{
+	int online_cpu, mktme_cpu;
+	int online_pkgid, mktme_pkgid = -1;
+
+	if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL))
+		return -ENOMEM;
+
+	for_each_online_cpu(online_cpu) {
+		online_pkgid = topology_physical_package_id(online_cpu);
+
+		for_each_cpu(mktme_cpu, mktme_leadcpus) {
+			mktme_pkgid = topology_physical_package_id(mktme_cpu);
+			if (mktme_pkgid == online_pkgid)
+				break;
+		}
+		if (mktme_pkgid != online_pkgid)
+			cpumask_set_cpu(online_cpu, mktme_leadcpus);

Do you really need LOCK prefixed bit set here?

No. Changed to __cpumask_set_cpu(). Will check for other instances
where I can skip LOCK prefix.

quoted

How is that serialized and kept relevant in the face of hotplug?

mktme_leadcpus is updated on hotplug startup and teardowns.

Not in this patch it is not. That is added in a subsequent patch, which
means that during bisection hotplug is utterly wrecked if you happen to
land between these patches, that is bad.

quoted

Also, do you really need O(n^2) to find the first occurence of a value
in an array?

How about this O(n)?
	
	unsigned long *pkg_map;
	int cpu, pkgid;

	if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL))
		return -ENOMEM;

	pkg_map = bitmap_zalloc(topology_max_packages(), GFP_KERNEL);
	if (!pkg_map) {
		free_cpumask_var(mktme_leadcpus);
		return -ENOMEM;
	}
	for_each_online_cpu(cpu) {
		pkgid = topology_physical_package_id(cpu);
		if (!test_and_set_bit(pkgid, pkg_map))

You again don't need that LOCK prefix here.

	__test_and_set_bit() :-)

			__cpumask_set_cpu(cpu, mktme_leadcpus);
	}

Right.