RE: [PATCH] x86_64: store Socket ID in phys_proc_id

RE: [PATCH] x86_64: store Socket ID in phys_proc_id

[Lu, Yinghai]

From: Andi Kleen [mailto:ak@muc.de] 

>> Socket ID is 0 for first Physical processor?
>It must just be some unique ID for each socket.

For dual core, if I lift AP's APIC ID and no touch BSP's APIC ID.
For example, BSP is still 0, and second core is 0x11.
The phys_proc_id will be 0 for BSP, and 8 for second core.
So I suggest you to use initial APIC ID to get socket id instead of APIC
ID.

YH

Re: [PATCH] x86_64: store Socket ID in phys_proc_id

[Andi Kleen]

On Tue, Oct 17, 2006 at 02:15:38PM -0700, Lu, Yinghai wrote:
> From: Andi Kleen [mailto:ak@muc.de] 
> 
> >> Socket ID is 0 for first Physical processor?
> >It must just be some unique ID for each socket.
> 
> For dual core, if I lift AP's APIC ID and no touch BSP's APIC ID.
> For example, BSP is still 0, and second core is 0x11.
> The phys_proc_id will be 0 for BSP, and 8 for second core.
> So I suggest you to use initial APIC ID to get socket id instead of APIC
> ID.

Hmm, that might make sense yes.

I'm just afraid what I will break again if i touch this -- it took several
iterations to get it to this state which AFAIK works everywhere right now.
And when it breaks it's usually subtle as "system boots, but runs
slower when ACPI is turned off because NUMA nodes are off" 

Can you remind me again what would be fixed by using the initial APIC ID?
Just prettier numbering in your lifted APIC IDs case? Or something
else too?

-Andi

RE: [PATCH] x86_64: store Socket ID in phys_proc_id

[Lu, Yinghai]

From: Andi Kleen [mailto:ak@muc.de] 

>Can you remind me again what would be fixed by using the initial APIC
ID?
>Just prettier numbering in your lifted APIC IDs case? Or something
>else too?

For easier understanding for socket id and apic id mapping. Also doesn't
care about BIOS how to set the APIC ID.

YH

RE: [PATCH] x86_64: store Socket ID in phys_proc_id

[Lu, Yinghai]

From: Andi Kleen [mailto:ak@muc.de] 

>> So for the apic id lifted system, for example BSP with apicid 0x10,
the
>> phys_proc_id will be 8.

>How is that a problem? 

Socket ID is 0 for first Physical processor?

>> It also removed ht_nodeid calculating, because We already have
correct
>> socket id for sure.

>we've had cases where it wasn't identical, that is when i originally
>added that code.

OK, it must be system with Horus?

YH

Re: [PATCH] x86_64: store Socket ID in phys_proc_id

[Andi Kleen]

On Tue, Oct 17, 2006 at 11:26:30AM -0700, Lu, Yinghai wrote:
> From: Andi Kleen [mailto:ak@muc.de] 
> 
> >> So for the apic id lifted system, for example BSP with apicid 0x10,
> the
> >> phys_proc_id will be 8.
> 
> >How is that a problem? 
> 
> Socket ID is 0 for first Physical processor?

It must just be some unique ID for each socket.

> >> It also removed ht_nodeid calculating, because We already have
> correct
> >> socket id for sure.
> 
> >we've had cases where it wasn't identical, that is when i originally
> >added that code.
> 
> OK, it must be system with Horus?

No it was with some dual core systems. Not all systems follow
the standard convention. Also there can be missing memory on
some nodes, this code handles this too.

-Andi

[PATCH] x86_64: store Socket ID in phys_proc_id

[Lu, Yinghai]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

Current code store phys_proc_id with init APIC ID, and later will change
to apicid>>bits.

So for the apic id lifted system, for example BSP with apicid 0x10, the
phys_proc_id will be 8.

This patch use initial APIC ID to get Socket ID.

It also removed ht_nodeid calculating, because We already have correct
socket id for sure.

Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>



[-- Attachment #2: setup_c.diff --]
[-- Type: application/octet-stream, Size: 2199 bytes --]

diff --git a/arch/x86_64/kernel/setup.c b/arch/x86_64/kernel/setup.c
index fc944b5..d30f78a 100644
--- a/arch/x86_64/kernel/setup.c
+++ b/arch/x86_64/kernel/setup.c
@@ -643,34 +643,20 @@ #endif
 
 	/* Low order bits define the core id (index of core in socket) */
 	c->cpu_core_id = c->phys_proc_id & ((1 << bits)-1);
-	/* Convert the APIC ID into the socket ID */
-	c->phys_proc_id = phys_pkg_id(bits);
+	/* Convert the initial APIC ID into the socket ID */
+	c->phys_proc_id >>= bits; 
 
 #ifdef CONFIG_NUMA
   	node = c->phys_proc_id;
  	if (apicid_to_node[apicid] != NUMA_NO_NODE)
  		node = apicid_to_node[apicid];
  	if (!node_online(node)) {
- 		/* Two possibilities here:
- 		   - The CPU is missing memory and no node was created.
- 		   In that case try picking one from a nearby CPU
- 		   - The APIC IDs differ from the HyperTransport node IDs
- 		   which the K8 northbridge parsing fills in.
- 		   Assume they are all increased by a constant offset,
- 		   but in the same order as the HT nodeids.
- 		   If that doesn't result in a usable node fall back to the
- 		   path for the previous case.  */
- 		int ht_nodeid = apicid - (cpu_data[0].phys_proc_id << bits);
- 		if (ht_nodeid >= 0 &&
- 		    apicid_to_node[ht_nodeid] != NUMA_NO_NODE)
- 			node = apicid_to_node[ht_nodeid];
  		/* Pick a nearby node */
- 		if (!node_online(node))
- 			node = nearby_node(apicid);
+		node = nearby_node(apicid);
  	}
 	numa_set_node(cpu, node);
 
-	printk(KERN_INFO "CPU %d/%x -> Node %d\n", cpu, apicid, node);
+	printk(KERN_INFO "CPU %d/0x%02x -> Node %d\n", cpu, apicid, node);
 #endif
 #endif
 }
@@ -928,7 +914,7 @@ void __cpuinit early_identify_cpu(struct
 	}
 
 #ifdef CONFIG_SMP
-	c->phys_proc_id = (cpuid_ebx(1) >> 24) & 0xff;
+	c->phys_proc_id = (cpuid_ebx(1) >> 24) & 0xff; /* initial APIC ID */
 #endif
 }
 
@@ -1136,6 +1122,7 @@ #endif
 #ifdef CONFIG_SMP
 	if (smp_num_siblings * c->x86_max_cores > 1) {
 		int cpu = c - cpu_data;
+		seq_printf(m, "apic id\t\t: 0x%02x\n", x86_cpu_to_apicid[cpu]);
 		seq_printf(m, "physical id\t: %d\n", c->phys_proc_id);
 		seq_printf(m, "siblings\t: %d\n", cpus_weight(cpu_core_map[cpu]));
 		seq_printf(m, "core id\t\t: %d\n", c->cpu_core_id);

Re: [PATCH] x86_64: store Socket ID in phys_proc_id

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

Current code store phys_proc_id with init APIC ID, and later will change
to apicid>>bits.

So for the apic id lifted system, for example BSP with apicid 0x10, the
phys_proc_id will be 8.

This patch use initial APIC ID to get Socket ID.

It also removed ht_nodeid calculating, because We already have correct
socket id for sure.

Signed-off-by: Yinghai Lu <yinghai.lu@amd.com>

[-- Attachment #2: setup_c.diff --]
[-- Type: text/x-patch, Size: 2199 bytes --]

diff --git a/arch/x86_64/kernel/setup.c b/arch/x86_64/kernel/setup.c
index fc944b5..d30f78a 100644
--- a/arch/x86_64/kernel/setup.c
+++ b/arch/x86_64/kernel/setup.c
@@ -643,34 +643,20 @@ #endif
 
 	/* Low order bits define the core id (index of core in socket) */
 	c->cpu_core_id = c->phys_proc_id & ((1 << bits)-1);
-	/* Convert the APIC ID into the socket ID */
-	c->phys_proc_id = phys_pkg_id(bits);
+	/* Convert the initial APIC ID into the socket ID */
+	c->phys_proc_id >>= bits; 
 
 #ifdef CONFIG_NUMA
   	node = c->phys_proc_id;
  	if (apicid_to_node[apicid] != NUMA_NO_NODE)
  		node = apicid_to_node[apicid];
  	if (!node_online(node)) {
- 		/* Two possibilities here:
- 		   - The CPU is missing memory and no node was created.
- 		   In that case try picking one from a nearby CPU
- 		   - The APIC IDs differ from the HyperTransport node IDs
- 		   which the K8 northbridge parsing fills in.
- 		   Assume they are all increased by a constant offset,
- 		   but in the same order as the HT nodeids.
- 		   If that doesn't result in a usable node fall back to the
- 		   path for the previous case.  */
- 		int ht_nodeid = apicid - (cpu_data[0].phys_proc_id << bits);
- 		if (ht_nodeid >= 0 &&
- 		    apicid_to_node[ht_nodeid] != NUMA_NO_NODE)
- 			node = apicid_to_node[ht_nodeid];
  		/* Pick a nearby node */
- 		if (!node_online(node))
- 			node = nearby_node(apicid);
+		node = nearby_node(apicid);
  	}
 	numa_set_node(cpu, node);
 
-	printk(KERN_INFO "CPU %d/%x -> Node %d\n", cpu, apicid, node);
+	printk(KERN_INFO "CPU %d/0x%02x -> Node %d\n", cpu, apicid, node);
 #endif
 #endif
 }
@@ -928,7 +914,7 @@ void __cpuinit early_identify_cpu(struct
 	}
 
 #ifdef CONFIG_SMP
-	c->phys_proc_id = (cpuid_ebx(1) >> 24) & 0xff;
+	c->phys_proc_id = (cpuid_ebx(1) >> 24) & 0xff; /* initial APIC ID */
 #endif
 }
 
@@ -1136,6 +1122,7 @@ #endif
 #ifdef CONFIG_SMP
 	if (smp_num_siblings * c->x86_max_cores > 1) {
 		int cpu = c - cpu_data;
+		seq_printf(m, "apic id\t\t: 0x%02x\n", x86_cpu_to_apicid[cpu]);
 		seq_printf(m, "physical id\t: %d\n", c->phys_proc_id);
 		seq_printf(m, "siblings\t: %d\n", cpus_weight(cpu_core_map[cpu]));
 		seq_printf(m, "core id\t\t: %d\n", c->cpu_core_id);

Re: [PATCH] x86_64: store Socket ID in phys_proc_id

[Andi Kleen]

On Tue, Oct 17, 2006 at 08:34:47AM -0700, Yinghai Lu wrote:
> Current code store phys_proc_id with init APIC ID, and later will change
> to apicid>>bits.
> 
> So for the apic id lifted system, for example BSP with apicid 0x10, the
> phys_proc_id will be 8.

How is that a problem? 


> This patch use initial APIC ID to get Socket ID.
> 
> It also removed ht_nodeid calculating, because We already have correct
> socket id for sure.

we've had cases where it wasn't identical, that is when i originally
added that code.

-Andi