APICID: Avoid false sharing on the read mostly x86_cpu_to_apicid

PATCH In review Eial Czerwacki 3 participants 6 messages Oct 25, 2011 – Oct 26, 2011 2 messages not in the archive

Subsystems (4)

x86 ACPI x86 MM x86 platform uv hpe superdome flex

Share & tools

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 9B14D1490DDECA4E974F6B9FC9EBAB3140CD274367@VMBX108.ihostexchange.net

Eial Czerwacki

author

2011-10-25 11:50 UTC #2 lore raw

PATCH 1/1 APICID: Avoid false sharing on the read mostly x86_cpu_to_apicid

Avoid false sharing on the read mostly x86_cpu_to_apicid by moving it to __read_mostly section.
The per-cpu area is write and read and this symbol shows up high on ipi intensive/x86_cpu_to_apicid load.

Signed-off-by Eial Czerwacki<[email protected]>

Signed-off-by Shai Fultheim<[email protected]>

Author: Ravikiran Thirumalai<[email protected]>
---

arch/x86/include/asm/smp.h +3 -2

Index: b/arch/x86/include/asm/smp.h
===================================================================
@@ -36,7 +36,8 @@ static inline struct cpumask *cpu_core_m
  	return per_cpu(cpu_core_map, cpu);
  }
 
-DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid);
+extern u16 x86_cpu_to_apicid[NR_CPUS];
+
  DECLARE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid);
 
  /* Static state in head.S used to set up a CPU */

@@ -142,7 +143,7 @@ void native_send_call_func_ipi(const str
  void native_send_call_func_single_ipi(int cpu);
 
  void smp_store_cpu_info(int id);
-#define cpu_physical_id(cpu)	per_cpu(x86_cpu_to_apicid, cpu)
+#define cpu_physical_id(cpu)	x86_cpu_to_apicid[cpu]
 
  /* We don't mark CPUs online until __cpu_up(), so we need another measure */
  static inline int num_booting_cpus(void)

arch/x86/kernel/acpi/boot.c +1 -1

Index: b/arch/x86/kernel/acpi/boot.c
===================================================================
@@ -568,7 +568,7 @@ EXPORT_SYMBOL(acpi_map_lsapic);
 
  int acpi_unmap_lsapic(int cpu)
  {
-	per_cpu(x86_cpu_to_apicid, cpu) = -1;
+	x86_cpu_to_apicid[cpu] = -1;
  	set_cpu_present(cpu, false);
  	num_processors--;

arch/x86/kernel/apic/apic.c +3 -4

Index: b/arch/x86/kernel/apic/apic.c
===================================================================
@@ -78,9 +78,8 @@ physid_mask_t phys_cpu_present_map;
  /*
   * Map cpu index to physical APIC ID
   */
-DEFINE_EARLY_PER_CPU(u16, x86_cpu_to_apicid, BAD_APICID);
+u16 x86_cpu_to_apicid[NR_CPUS] __read_mostly = { [0 ... NR_CPUS-1] = BAD_APICID };
  DEFINE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid, BAD_APICID);
-EXPORT_EARLY_PER_CPU_SYMBOL(x86_cpu_to_apicid);
  EXPORT_EARLY_PER_CPU_SYMBOL(x86_bios_cpu_apicid);
 
  #ifdef CONFIG_X86_32

@@ -1248,7 +1247,7 @@
  	 * proper NUMA affinity.
  	 */
  	if (apic->x86_32_numa_cpu_node)
-		set_apicid_to_node(early_per_cpu(x86_cpu_to_apicid, cpu),
+		set_apicid_to_node(x86_cpu_to_apicid[cpu],
  				   apic->x86_32_numa_cpu_node(cpu));
  #endif

@@ -1916,7 +1915,7 @@ void __cpuinit generic_processor_info(in
  		max_physical_apicid = apicid;
 
  #if defined(CONFIG_SMP) || defined(CONFIG_X86_64)
-	early_per_cpu(x86_cpu_to_apicid, cpu) = apicid;
+	x86_cpu_to_apicid[cpu] = apicid;
  	early_per_cpu(x86_bios_cpu_apicid, cpu) = apicid;
  #endif

arch/x86/kernel/apic/apic_flat_64.c +2 -2

Index: b/arch/x86/kernel/apic/apic_flat_64.c
===================================================================
@@ -294,7 +294,7 @@ static unsigned int physflat_cpu_mask_to
  	 */
  	cpu = cpumask_first(cpumask);
  	if ((unsigned)cpu<   nr_cpu_ids)
-		return per_cpu(x86_cpu_to_apicid, cpu);
+		return x86_cpu_to_apicid[cpu];
  	else
  		return BAD_APICID;
  }

@@ -314,7 +314,7 @@ c(const st
  		if (cpumask_test_cpu(cpu, cpu_online_mask))
  			break;
  	}
-	return per_cpu(x86_cpu_to_apicid, cpu);
+	return x86_cpu_to_apicid[cpu];
  }
 
  static int physflat_probe(void)

arch/x86/kernel/apic/ipi.c +5 -5

Index: b/arch/x86/kernel/apic/ipi.c
===================================================================
@@ -31,8 +31,8 @@ void default_send_IPI_mask_sequence_phys
  	 */
  	local_irq_save(flags);
  	for_each_cpu(query_cpu, mask) {
-		__default_send_IPI_dest_field(per_cpu(x86_cpu_to_apicid,
-				query_cpu), vector, APIC_DEST_PHYSICAL);
+		__default_send_IPI_dest_field(x86_cpu_to_apicid[query_cpu],
+				vector, APIC_DEST_PHYSICAL);
  	}
  	local_irq_restore(flags);
  }

@@ -50,8 +50,8 @@ void default_send_IPI_mask_allbutself_ph
  	for_each_cpu(query_cpu, mask) {
  		if (query_cpu == this_cpu)
  			continue;
-		__default_send_IPI_dest_field(per_cpu(x86_cpu_to_apicid,
-				 query_cpu), vector, APIC_DEST_PHYSICAL);
+		__default_send_IPI_dest_field(x86_cpu_to_apicid[query_cpu],
+				 vector, APIC_DEST_PHYSICAL);
  	}
  	local_irq_restore(flags);
  }

@@ -143,7 +143,7 @@ static int convert_apicid_to_cpu(int api
  	int i;
 
  	for_each_possible_cpu(i) {
-		if (per_cpu(x86_cpu_to_apicid, i) == apic_id)
+		if (x86_cpu_to_apicid[i] == apic_id)
  			return i;
  	}
  	return -1;

arch/x86/kernel/apic/x2apic_phys.c +3 -3

Index: b/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
@@ -103,7 +103,7 @@ static void x2apic_send_IPI_allbutself(i
  	for_each_cpu(query_cpu) {
  		if (apic_dest == APIC_DEST_ALLBUT&&   query_cpu == this_cpu)
  			continue;
-		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
+		__x2apic_send_IPI_dest(x86_cpu_to_apicid[query_cpu],
  				       vector, APIC_DEST_PHYSICAL);
  	}
  	local_irq_restore(flags);

@@ -128,7 +128,7 @@ static unsigned int x2apic_cpu_mask_to_a
  	int cpu = cpumask_first(cpumask);
 
  	if ((unsigned)cpu<   nr_cpu_ids)
-		return per_cpu(x86_cpu_to_apicid, cpu);
+		return x86_cpu_to_apicid[cpu];
  	else
  		return BAD_APICID;
  }

@@ -149,7 +149,7 @@ x2apic_cpu_mask_to_apicid_and(const stru
  			break;
  	}
 
-	return per_cpu(x86_cpu_to_apicid, cpu);
+	return x86_cpu_to_apicid[cpu];
  }
 
  static void init_x2apic_ldr(void)

arch/x86/kernel/apic/x2apic_uv_x.c +4 -4

Index: b/arch/x86/kernel/apic/x2apic_uv_x.c
===================================================================
@@ -135,7 +135,7 @@ static void uv_send_IPI_one(int cpu, int
  	unsigned long apicid;
  	int pnode;
 
-	apicid = per_cpu(x86_cpu_to_apicid, cpu);
+	apicid = x86_cpu_to_apicid[cpu];
  	pnode = uv_apicid_to_pnode(apicid);
  	uv_hub_send_ipi(pnode, apicid, vector);
  }

@@ -193,7 +193,7 @@ static unsigned int uv_cpu_mask_to_apici
  	int cpu = cpumask_first(cpumask);
 
  	if ((unsigned)cpu<   nr_cpu_ids)
-		return per_cpu(x86_cpu_to_apicid, cpu) | uv_apicid_hibits;
+		return x86_cpu_to_apicid[cpu] | uv_apicid_hibits;
  	else
  		return BAD_APICID;
  }

@@ -213,6 +213,6 @@ uv_cpu_mask_to_apicid_and(const struct c
  		if (cpumask_test_cpu(cpu, cpu_online_mask))
  			break;
  	}
-	return per_cpu(x86_cpu_to_apicid, cpu) | uv_apicid_hibits;
+	return return x86_cpu_to_apicid[cpu] | uv_apicid_hibits;
  }

@@ -608,7 +608,7 @@ void __init uv_system_init(void)
  	uv_rtc_init();
 
  	for_each_present_cpu(cpu) {
-		int apicid = per_cpu(x86_cpu_to_apicid, cpu);
+		int apicid = x86_cpu_to_apicid[cpu];
 
  		nid = cpu_to_node(cpu);
  		/*

arch/x86/kernel/setup_percpu.c +0 -3

Index: b/arch/x86/kernel/setup_percpu.c
===================================================================
@@ -225,8 +225,6 @@ void __init setup_per_cpu_areas(void)
  		 * gone.
  		 */
  #ifdef CONFIG_X86_LOCAL_APIC
-		per_cpu(x86_cpu_to_apicid, cpu) =
-			early_per_cpu_map(x86_cpu_to_apicid, cpu);
  		per_cpu(x86_bios_cpu_apicid, cpu) =
  			early_per_cpu_map(x86_bios_cpu_apicid, cpu);
  #endif

@@ -249,7 +247,6 @@ void __init setup_per_cpu_areas(void)
 
  	/* indicate the early static arrays will soon be gone */
  #ifdef CONFIG_X86_LOCAL_APIC
-	early_per_cpu_ptr(x86_cpu_to_apicid) = NULL;
  	early_per_cpu_ptr(x86_bios_cpu_apicid) = NULL;
  #endif
  #if defined(CONFIG_X86_64)&&   defined(CONFIG_NUMA)

arch/x86/kernel/smpboot.c +1 -1

Index: b/arch/x86/kernel/smpboot.c
===================================================================
@@ -818,7 +818,7 @@ do_rest:
  		cpumask_clear_cpu(cpu, cpu_initialized_mask);
 
  		set_cpu_present(cpu, false);
-		per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
+		x86_cpu_to_apicid[cpu] = BAD_APICID;
  	}
 
  	/* mark "stuck" area as not stuck */

arch/x86/platform/uv/tlb_uv.c +1 -1

Index: b/arch/x86/platform/uv/tlb_uv.c
===================================================================
@@ -55,7 +55,7 @@ static int __init blade_to_first_uvhub(
 
  	for_each_present_cpu(cpu)
  		if (uvhub == uv_cpu_to_blade_id(cpu))
-			return per_cpu(x86_cpu_to_apicid, cpu);
+			return x86_cpu_to_apicid[cpu];
  	return -1;
  }

arch/x86/mm/numa.c +1 -4

Index: b/arch/x86/mm/numa.c
===================================================================
@@ -62,7 +62,7 @@ s16 __apicid_to_node[MAX_LOCAL_APIC] __c
 
  int __cpuinit numa_cpu_node(int cpu)
  {
-	int apicid = early_per_cpu(x86_cpu_to_apicid, cpu);
+	int apicid = x86_cpu_to_apicid[cpu];
 
  	if (apicid != BAD_APICID)
  		return __apicid_to_node[apicid];

@@ -694,9 +694,6 @@ static __init int find_near_online_node(
  void __init init_cpu_to_node(void)
  {
  	int cpu;
-	u16 *cpu_to_apicid = early_per_cpu_ptr(x86_cpu_to_apicid);
-
-	BUG_ON(cpu_to_apicid == NULL);
 
  	for_each_possible_cpu(cpu) {
  		int node = numa_cpu_node(cpu);

David Rientjes

2011-10-26 06:21 UTC #3 lore raw

Quoted 2 lines · Eial Czerwacki

Avoid false sharing on the read mostly x86_cpu_to_apicid by moving it to __read_mostly section.

x86_cpu_to_apicid used to be in .init.data and now it's never freed after boot?

Quoted 6 lines

The per-cpu area is write and read and this symbol shows up high on ipi intensive/x86_cpu_to_apicid load.
Signed-off-by Eial Czerwacki<[email protected]>
Signed-off-by Shai Fultheim<[email protected]>
Author: Ravikiran Thirumalai<[email protected]>

If the author isn't you, then the patch should start with a single line that says "From: name <email>" of the author followed by a blank line. It's also strange that the author doesn't have a sign-off line.

Your email client also has corrupted the inline patch, please see Documentation/email-clients.txt.

And, finally, please send this to the x86 maintainers once fixed. See ./scripts/get_maintainers.pl <your patch>.

Quoted 28 lines

Index: b/arch/x86/include/asm/smp.h
===================================================================

arch/x86/include/asm/smp.h +3 -2

@@ -36,7 +36,8 @@ static inline struct cpumask *cpu_core_m
  	return per_cpu(cpu_core_map, cpu);
  }
 
-DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid);
+extern u16 x86_cpu_to_apicid[NR_CPUS];
+
  DECLARE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid);
 
  /* Static state in head.S used to set up a CPU */

@@ -142,7 +143,7 @@ void native_send_call_func_ipi(const str
  void native_send_call_func_single_ipi(int cpu);
 
  void smp_store_cpu_info(int id);
-#define cpu_physical_id(cpu)	per_cpu(x86_cpu_to_apicid, cpu)
+#define cpu_physical_id(cpu)	x86_cpu_to_apicid[cpu]
 
  /* We don't mark CPUs online until __cpu_up(), so we need another measure */
  static inline int num_booting_cpus(void)

Index: b/arch/x86/kernel/acpi/boot.c
===================================================================

arch/x86/kernel/acpi/boot.c EXPORT_SYMBOL(acpi_map_lsapic); · line 574

Quoted 8 lines

arch/x86/kernel/acpi/boot.c +1 -1

@@ -568,7 +568,7 @@ EXPORT_SYMBOL(acpi_map_lsapic);
 
  int acpi_unmap_lsapic(int cpu)
  {
-	per_cpu(x86_cpu_to_apicid, cpu) = -1;
+	x86_cpu_to_apicid[cpu] = -1;
  	set_cpu_present(cpu, false);
  	num_processors--;

Shouldn't this be BAD_APICID? Not sure where we check for -1.

Eial Czerwacki

author

2011-10-26 06:41 UTC #4 lore raw

Quoted 11 lines · David Rientjes

Quoted 2 lines · Eial Czerwacki
Avoid false sharing on the read mostly x86_cpu_to_apicid by moving it to __read_mostly section.
x86_cpu_to_apicid used to be in .init.data and now it's never freed after boot?
Quoted 3 lines
The per-cpu area is write and read and this symbol shows up high on ipi intensive/x86_cpu_to_apicid load.

Quoted 8 lines

Quoted 3 lines
Signed-off-by Eial Czerwacki<[email protected]>
Signed-off-by Shai Fultheim<[email protected]>
Author: Ravikiran Thirumalai<[email protected]>
If the author isn't you, then the patch should start with a single line that says "From: name <email>" of the author followed by a blank line. It's also strange that the author doesn't have a sign-off line.

the author is mentioned after the last sign-off.

Quoted 3 lines

Your email client also has corrupted the inline patch, please see Documentation/email-clients.txt.

thanks for the tips, I hope it is ok now.

Quoted 3 lines

And, finally, please send this to the x86 maintainers once fixed. See ./scripts/get_maintainers.pl <your patch>.

sure.

Quoted 29 lines

Index: b/arch/x86/include/asm/smp.h
===================================================================

arch/x86/include/asm/smp.h +3 -2

@@ -36,7 +36,8 @@ static inline struct cpumask *cpu_core_m
  	return per_cpu(cpu_core_map, cpu);
  }
 
-DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid);
+extern u16 x86_cpu_to_apicid[NR_CPUS];
+
  DECLARE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid);
 
  /* Static state in head.S used to set up a CPU */

@@ -142,7 +143,7 @@ void native_send_call_func_ipi(const str
  void native_send_call_func_single_ipi(int cpu);
 
  void smp_store_cpu_info(int id);
-#define cpu_physical_id(cpu)	per_cpu(x86_cpu_to_apicid, cpu)
+#define cpu_physical_id(cpu)	x86_cpu_to_apicid[cpu]
 
  /* We don't mark CPUs online until __cpu_up(), so we need another measure */
  static inline int num_booting_cpus(void)

Index: b/arch/x86/kernel/acpi/boot.c
===================================================================

arch/x86/kernel/acpi/boot.c +0 -0

@@ -568,7 +568,7 @@ EXPORT_SYMBOL(acpi_map_lsapic);

Quoted 15 lines

arch/x86/kernel/acpi/boot.c EXPORT_SYMBOL(acpi_map_lsapic); · line 574
Quoted 7 lines
arch/x86/kernel/acpi/boot.c +1 -1
  int acpi_unmap_lsapic(int cpu)
  {
-	per_cpu(x86_cpu_to_apicid, cpu) = -1;
+	x86_cpu_to_apicid[cpu] = -1;
  	set_cpu_present(cpu, false);
  	num_processors--;
 
Shouldn't this be BAD_APICID? Not sure where we check for -1. --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I'll check the rest of your comments.

Thanks,

Eial.

David Rientjes

2011-10-26 06:47 UTC #5 lore raw

Quoted 4 lines · Eial Czerwacki

Quoted 4 lines
Quoted 3 lines
Signed-off-by Eial Czerwacki<[email protected]>
Signed-off-by Shai Fultheim<[email protected]>
Author: Ravikiran Thirumalai<[email protected]>

Quoted 8 lines

Quoted 4 lines
If the author isn't you, then the patch should start with a single line that says "From: name <email>" of the author followed by a blank line. It's also strange that the author doesn't have a sign-off line.
the author is mentioned after the last sign-off.

That's not the way it's done, as I said, the patch needs to start with a single line like this:

From: Ravikiran Thirumalai <[email protected]>

...

and if that person signed it off, then it needs to appear as

Signed-off-by ...

as the first one in the chain. Please see part 15 of section 1 of Documentation/SubmittingPatches.

Eial Czerwacki

author

2011-10-26 07:01 UTC #6 lore raw

Quoted 26 lines · David Rientjes

Quoted 4 lines · Eial Czerwacki
Quoted 4 lines
Quoted 3 lines
Signed-off-by Eial Czerwacki<[email protected]>
Signed-off-by Shai Fultheim<[email protected]>
Author: Ravikiran Thirumalai<[email protected]>
Quoted 8 lines
Quoted 4 lines
If the author isn't you, then the patch should start with a single line that says "From: name <email>" of the author followed by a blank line. It's also strange that the author doesn't have a sign-off line.
the author is mentioned after the last sign-off.
That's not the way it's done, as I said, the patch needs to start with a single line like this:
From: Ravikiran Thirumalai <[email protected]>
...
and if that person signed it off, then it needs to appear as
Signed-off-by ...

Quoted 8 lines

as the first one in the chain. Please see part 15 of section 1 of Documentation/SubmittingPatches.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

sure, thanks got the info

Eial.

Andi Kleen

2011-10-26 08:20 UTC #7 lore raw

In reply to Eial Czerwacki

arch/x86/include/asm/smp.h static inline struct cpumask *cpu_core_m · line 39

Quoted 9 lines · Eial Czerwacki

===================================================================
arch/x86/include/asm/smp.h +1 -1
@@ -36,7 +36,8 @@ static inline struct cpumask *cpu_core_m
  	return per_cpu(cpu_core_map, cpu);
  }
 
-DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid);
+extern u16 x86_cpu_to_apicid[NR_CPUS];

That's a big waste of memory. On a CONFIG_MAX_SMP kernel compiled for 4096 CPUs you're wasting 8k now on smaller systems. The per cpu data only allocates what is needed.

NR_CPUS is usually a bad idea and it

Perhaps need a __read_mostly cache line padded per CPU section instead for cases like this.

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only