diff mbox

[v2,5/7] x86, acpi, cpu-hotplug: Introduce apicid_to_cpuid[] array to store persistent cpuid <-> apicid mapping.

Message ID 1441859269-25831-6-git-send-email-tangchen@cn.fujitsu.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

tangchen Sept. 10, 2015, 4:27 a.m. UTC 
From: Gu Zheng <guz.fnst@cn.fujitsu.com>

This patch finishes step2 mentioned in previous patch 4.

In this patch, we introduce a new static array named apicid_to_cpuid[],
which is large enough to store info for all possible cpus.

And then, we modify the cpuid calculation. In generic_processor_info(),
it simply finds the next unused cpuid. And it is also why the cpuid <-> nodeid
mapping changes with node hotplug.

After this patch, we find the next unused cpuid, map it to an apicid,
and store the mapping in apicid_to_cpuid[], so that cpuid <-> apicid
mapping will be persistent.

And finally we will use this array to make cpuid <-> nodeid persistent.

cpuid <-> apicid mapping is established at local apic registeration time.
But non-present or disabled cpus are ignored.

In this patch, we establish all possible cpuid <-> apicid mapping when
registering local apic.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 arch/x86/include/asm/mpspec.h |  1 +
 arch/x86/kernel/acpi/boot.c   |  6 ++---
 arch/x86/kernel/apic/apic.c   | 53 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 53 insertions(+), 7 deletions(-)

Comments

Tejun Heo Sept. 10, 2015, 7:55 p.m. UTC | #1 
Hello,

So, overall, I think this is the right way to go although I have no
idea whether the acpi part is okay.

> +/*
> + * Current allocated max logical CPU ID plus 1.
> + * All allocated CPU ID should be in [0, max_logical_cpuid),
> + * so the maximum of max_logical_cpuid is nr_cpu_ids.
> + *
> + * NOTE: Reserve 0 for BSP.
> + */
> +static int max_logical_cpuid = 1;

Rename it to nr_logical_cpuids and just mention that it's allocated
contiguously?

> +static int cpuid_to_apicid[] = {
> +	[0 ... NR_CPUS - 1] = -1,
> +};

And maybe mention how the two variables are synchronized?

> +static int allocate_logical_cpuid(int apicid)
> +{
> +	int i;
> +
> +	/*
> +	 * cpuid <-> apicid mapping is persistent, so when a cpu is up,
> +	 * check if the kernel has allocated a cpuid for it.
> +	 */
> +	for (i = 0; i < max_logical_cpuid; i++) {
> +		if (cpuid_to_apicid[i] == apicid)
> +			return i;
> +	}
> +
> +	/* Allocate a new cpuid. */
> +	if (max_logical_cpuid >= nr_cpu_ids) {
> +		WARN_ONCE(1, "Only %d processors supported."
> +			     "Processor %d/0x%x and the rest are ignored.\n",
> +			     nr_cpu_ids - 1, max_logical_cpuid, apicid);
> +		return -1;
> +	}

So, the original code didn't have this failure mode, why is this
different for the new code?

Thanks.
tangchen Sept. 26, 2015, 9:52 a.m. UTC | #2 
Hi tj,

On 09/11/2015 03:55 AM, Tejun Heo wrote:
> Hello,
>
> So, overall, I think this is the right way to go although I have no
> idea whether the acpi part is okay.

Thank you very much for reviewing. :)

>
>> +/*
>> + * Current allocated max logical CPU ID plus 1.
>> + * All allocated CPU ID should be in [0, max_logical_cpuid),
>> + * so the maximum of max_logical_cpuid is nr_cpu_ids.
>> + *
>> + * NOTE: Reserve 0 for BSP.
>> + */
>> +static int max_logical_cpuid = 1;
> Rename it to nr_logical_cpuids and just mention that it's allocated
> contiguously?

OK.

>
>> +static int cpuid_to_apicid[] = {
>> +	[0 ... NR_CPUS - 1] = -1,
>> +};
> And maybe mention how the two variables are synchronized?

User should call allocate_logical_cpuid() to get a new logical cpuid.
This allocator will ensure the synchronization.

Will mention it in the comment.

>
>> +static int allocate_logical_cpuid(int apicid)
>> +{
>> +	int i;
>> +
>> +	/*
>> +	 * cpuid <-> apicid mapping is persistent, so when a cpu is up,
>> +	 * check if the kernel has allocated a cpuid for it.
>> +	 */
>> +	for (i = 0; i < max_logical_cpuid; i++) {
>> +		if (cpuid_to_apicid[i] == apicid)
>> +			return i;
>> +	}
>> +
>> +	/* Allocate a new cpuid. */
>> +	if (max_logical_cpuid >= nr_cpu_ids) {
>> +		WARN_ONCE(1, "Only %d processors supported."
>> +			     "Processor %d/0x%x and the rest are ignored.\n",
>> +			     nr_cpu_ids - 1, max_logical_cpuid, apicid);
>> +		return -1;
>> +	}
> So, the original code didn't have this failure mode, why is this
> different for the new code?

It is not different. Since max_logical_cpuid is new, this is ensure it 
won't
go beyond NR_CPUS.

Thanks.

>
> Thanks.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Sept. 26, 2015, 5:56 p.m. UTC | #3 
On Sat, Sep 26, 2015 at 05:52:09PM +0800, Tang Chen wrote:
> >>+static int allocate_logical_cpuid(int apicid)
> >>+{
> >>+	int i;
> >>+
> >>+	/*
> >>+	 * cpuid <-> apicid mapping is persistent, so when a cpu is up,
> >>+	 * check if the kernel has allocated a cpuid for it.
> >>+	 */
> >>+	for (i = 0; i < max_logical_cpuid; i++) {
> >>+		if (cpuid_to_apicid[i] == apicid)
> >>+			return i;
> >>+	}
> >>+
> >>+	/* Allocate a new cpuid. */
> >>+	if (max_logical_cpuid >= nr_cpu_ids) {
> >>+		WARN_ONCE(1, "Only %d processors supported."
> >>+			     "Processor %d/0x%x and the rest are ignored.\n",
> >>+			     nr_cpu_ids - 1, max_logical_cpuid, apicid);
> >>+		return -1;
> >>+	}
> >So, the original code didn't have this failure mode, why is this
> >different for the new code?
> 
> It is not different. Since max_logical_cpuid is new, this is ensure it won't
> go beyond NR_CPUS.

If the above condition can happen, the original code should have had a
similar check as above, right?  Sure, max_logical_cpuid is a new thing
but that doesn't seem to change whether the above condition can happen
or not, no?

Thanks.
tangchen Sept. 28, 2015, 1:57 a.m. UTC | #4 
On 09/27/2015 01:56 AM, Tejun Heo wrote:
> On Sat, Sep 26, 2015 at 05:52:09PM +0800, Tang Chen wrote:
>>>> +static int allocate_logical_cpuid(int apicid)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	/*
>>>> +	 * cpuid <-> apicid mapping is persistent, so when a cpu is up,
>>>> +	 * check if the kernel has allocated a cpuid for it.
>>>> +	 */
>>>> +	for (i = 0; i < max_logical_cpuid; i++) {
>>>> +		if (cpuid_to_apicid[i] == apicid)
>>>> +			return i;
>>>> +	}
>>>> +
>>>> +	/* Allocate a new cpuid. */
>>>> +	if (max_logical_cpuid >= nr_cpu_ids) {
>>>> +		WARN_ONCE(1, "Only %d processors supported."
>>>> +			     "Processor %d/0x%x and the rest are ignored.\n",
>>>> +			     nr_cpu_ids - 1, max_logical_cpuid, apicid);
>>>> +		return -1;
>>>> +	}
>>> So, the original code didn't have this failure mode, why is this
>>> different for the new code?
>> It is not different. Since max_logical_cpuid is new, this is ensure it won't
>> go beyond NR_CPUS.
> If the above condition can happen, the original code should have had a
> similar check as above, right?  Sure, max_logical_cpuid is a new thing
> but that doesn't seem to change whether the above condition can happen
> or not, no?

Right, indeed. It is in

generic_processor_info()
|--> if (num_processors >= nr_cpu_ids)

Will remove my new added check.

Thanks.

>
> Thanks.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h
index b07233b..db902d8 100644
--- a/arch/x86/include/asm/mpspec.h
+++ b/arch/x86/include/asm/mpspec.h
@@ -86,6 +86,7 @@  static inline void early_reserve_e820_mpc_new(void) { }
 #endif
 
 int generic_processor_info(int apicid, int version);
+int __generic_processor_info(int apicid, int version, bool enabled);
 
 #define PHYSID_ARRAY_SIZE	BITS_TO_LONGS(MAX_LOCAL_APIC)
 
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e49ee24..bcc85b2 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -174,15 +174,13 @@  static int acpi_register_lapic(int id, u8 enabled)
 		return -EINVAL;
 	}
 
-	if (!enabled) {
+	if (!enabled)
 		++disabled_cpus;
-		return -EINVAL;
-	}
 
 	if (boot_cpu_physical_apicid != -1U)
 		ver = apic_version[boot_cpu_physical_apicid];
 
-	return generic_processor_info(id, ver);
+	return __generic_processor_info(id, ver, enabled);
 }
 
 static int __init
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index a9c9830..42b2a9c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1977,7 +1977,45 @@  void disconnect_bsp_APIC(int virt_wire_setup)
 	apic_write(APIC_LVT1, value);
 }
 
-static int __generic_processor_info(int apicid, int version, bool enabled)
+/*
+ * Current allocated max logical CPU ID plus 1.
+ * All allocated CPU ID should be in [0, max_logical_cpuid),
+ * so the maximum of max_logical_cpuid is nr_cpu_ids.
+ *
+ * NOTE: Reserve 0 for BSP.
+ */
+static int max_logical_cpuid = 1;
+
+static int cpuid_to_apicid[] = {
+	[0 ... NR_CPUS - 1] = -1,
+};
+
+static int allocate_logical_cpuid(int apicid)
+{
+	int i;
+
+	/*
+	 * cpuid <-> apicid mapping is persistent, so when a cpu is up,
+	 * check if the kernel has allocated a cpuid for it.
+	 */
+	for (i = 0; i < max_logical_cpuid; i++) {
+		if (cpuid_to_apicid[i] == apicid)
+			return i;
+	}
+
+	/* Allocate a new cpuid. */
+	if (max_logical_cpuid >= nr_cpu_ids) {
+		WARN_ONCE(1, "Only %d processors supported."
+			     "Processor %d/0x%x and the rest are ignored.\n",
+			     nr_cpu_ids - 1, max_logical_cpuid, apicid);
+		return -1;
+	}
+
+	cpuid_to_apicid[max_logical_cpuid] = apicid;
+	return max_logical_cpuid++;
+}
+
+int __generic_processor_info(int apicid, int version, bool enabled)
 {
 	int cpu, max = nr_cpu_ids;
 	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
@@ -2058,8 +2096,17 @@  static int __generic_processor_info(int apicid, int version, bool enabled)
 		 * for BSP.
 		 */
 		cpu = 0;
-	} else
-		cpu = cpumask_next_zero(-1, cpu_present_mask);
+
+		/* Logical cpuid 0 is reserved for BSP. */
+		cpuid_to_apicid[0] = apicid;
+	} else {
+		cpu = allocate_logical_cpuid(apicid);
+		if (cpu < 0) {
+			if (enabled)
+				disabled_cpus++;
+			return -EINVAL;
+		}
+	}
 
 	/*
 	 * Validate version