x86, x2apic: minimize IPI register writes using cluster groups

RFC In review Cyrill Gorcunov 2 participants 6 messages Feb 3, 2011 – Feb 16, 2011 x86

Share & tools

Cyrill Gorcunov

author

2011-02-03 21:03 UTC #0 lore raw

RFC 1/2 x86, x2apic: minimize IPI register writes using cluster groups

In the case of x2apic cluster mode we can group IPI register writes based on the cluster group instead of individual per-cpu destiantion messages. This reduces the apic register writes and reduces the amount of IPI messages (in the best case we can reduce it by a factor of 16).

With this change, microbenchmark measuring the cost of flush_tlb_others(), with the flush tlb IPI being sent from a cpu in the socket-1 to all the logical cpus in socket-2 (on a Westmere-EX system that has 20 logical cpus in a socket) is 3x times better now (compared to the former 'send one-by-one' algorithm).

Signed-off-by Cyrill Gorcunov <[email protected]>

Signed-off-by Suresh Siddha <[email protected]>

---
 arch/x86/include/asm/apic.h           |    2
 arch/x86/kernel/apic/probe_64.c       |    4
 arch/x86/kernel/apic/x2apic_cluster.c |  169 +++++++++++++++++++++++++---------
 3 files changed, 131 insertions(+), 44 deletions(-)

arch/x86/kernel/apic/probe_64.c +3 -1

Index: tip-linux-2.6/arch/x86/kernel/apic/probe_64.c
===================================================================
@@ -71,7 +71,9 @@ void __init default_setup_apic_routing(v
 #endif
 
 	if (apic == &apic_flat && num_possible_cpus() > 8)
-			apic = &apic_physflat;
+		apic = &apic_physflat;
+	else if (apic == &apic_x2apic_cluster)
+		x2apic_init_cpu_notifier();
 
 	printk(KERN_INFO "Setting APIC routing to %s\n", apic->name);

arch/x86/kernel/apic/x2apic_cluster.c +126 ~~-43~~

221 lines not on this page · raw message

arch/x86/include/asm/apic.h +2 -0

Index: tip-linux-2.6/arch/x86/include/asm/apic.h
===================================================================
@@ -179,6 +179,8 @@ extern int x2apic_phys;
 extern void check_x2apic(void);
 extern void enable_x2apic(void);
 extern void x2apic_icr_write(u32 low, u32 id);
+extern void x2apic_init_cpu_notifier(void);
+
 static inline int x2apic_enabled(void)
 {
 	u64 msr;

Ingo Molnar

2011-02-14 11:45 UTC #1 lore raw

Quoted 9 lines · Cyrill Gorcunov

In the case of x2apic cluster mode we can group IPI register writes based on the cluster group instead of individual per-cpu destiantion messages. This reduces the apic register writes and reduces the amount of IPI messages (in the best case we can reduce it by a factor of 16).
With this change, microbenchmark measuring the cost of flush_tlb_others(), with the flush tlb IPI being sent from a cpu in the socket-1 to all the logical cpus in socket-2 (on a Westmere-EX system that has 20 logical cpus in a socket) is 3x times better now (compared to the former 'send one-by-one' algorithm).

Pretty nice!

I have a few structural and nitpicking comments:

arch/x86/kernel/apic/probe_64.c void __init default_setup_apic_routing(v · line 76

Quoted 9 lines

+++ tip-linux-2.6/arch/x86/kernel/apic/probe_64.c

arch/x86/kernel/apic/probe_64.c +3 -1

@@ -71,7 +71,9 @@ void __init default_setup_apic_routing(v
 #endif
 
 	if (apic == &apic_flat && num_possible_cpus() > 8)
-			apic = &apic_physflat;
+		apic = &apic_physflat;
+	else if (apic == &apic_x2apic_cluster)
+		x2apic_init_cpu_notifier();

This should be encapsulated cleaner - this higher layer does not care what the x2apic code tries to initialize.

So i think the cleanest method would be to clean up default_setup_apic_routing() and get rid of the CONFIG_X86_X2APIC (and UV and vsmp) mess from there.

We want some sort of proper driver init sequence between the various APIC drivers, which sequence the default_setup_apic_routing() code just iterates and as a result it gets a pointer to a 'struct apic'. This would be like all other driver init sequences work - no ugly 'glue' like probe_64.c would be needed.

arch/x86/kernel/apic/x2apic_cluster.c static void · line 60

Quoted 2 lines

arch/x86/kernel/apic/x2apic_cluster.c +2 -0

+static void __x2apic_send_IPI_mask(const struct cpumask *mask, int vector,
+				   int exclude_self)

Please if possible don't break function prototypes in the middle of the argument list. There are more natural places to break the line:

static void
__x2apic_send_IPI_mask(const struct cpumask *mask, int vector, int exclude_self)

But it can be in a single line as well. (up to 90-100 cols is fine.)

arch/x86/kernel/apic/x2apic_cluster.c static void · line 65

Quoted 5 lines

arch/x86/kernel/apic/x2apic_cluster.c +4 -0

+{
+	unsigned long cpu;
 	unsigned long flags;
+	struct cpumask *cpus_in_cluster_ptr, *ipi_mask_ptr;
+	u32 dest, this_cpu;

Ok, this is a pet peeve of mine: look how inconsistent this variable definition block has become. This would be cleaner:

	struct cpumask *cpus_in_cluster_ptr, *ipi_mask_ptr;
	unsigned long cpu, flags;
	u32 dest, this_cpu;

arch/x86/kernel/apic/x2apic_cluster.c static void · line 75

Quoted 4 lines

arch/x86/kernel/apic/x2apic_cluster.c +4 -0

+	/*
+	 * we are to modify mask, so we need an own copy
+	 * and be sure it's manipulated with irq off
+	 */

Most comments in this file capitalize the first word in a comment block.

(this holds for a number of similar cases as well in later portions of your patch)

arch/x86/kernel/apic/x2apic_cluster.c static void · line 84

Quoted 3 lines

arch/x86/kernel/apic/x2apic_cluster.c +3 -0
+	for_each_cpu(cpu, ipi_mask_ptr) {
+		unsigned long i;
+		dest = 0;

Please separate variable definitions from the first statement in a more visible manner.

arch/x86/kernel/apic/x2apic_cluster.c static void · line 90

Quoted 6 lines

arch/x86/kernel/apic/x2apic_cluster.c +6 -0

+		cpus_in_cluster_ptr = per_cpu(cpus_in_cluster, cpu);
+
+		/* only cpus in cluster involved */
+		for_each_cpu_and(i, ipi_mask_ptr, cpus_in_cluster_ptr)
+			if (!exclude_self || i != this_cpu)
+				dest |= per_cpu(x86_cpu_to_logical_apicid, i);

Please use curly braces in blocks that are longer than a single line.

arch/x86/kernel/apic/x2apic_cluster.c x2apic_cpu_mask_to_apicid_and(const stru · line 167

Quoted 5 lines

arch/x86/kernel/apic/x2apic_cluster.c +5 -0

+#define x2apic_propagate_cpu_cluster_status_online(cpu)		\
+	x2apic_propagate_cpu_cluster_status(cpu, 1)
+
+#define x2apic_propagate_cpu_cluster_status_offline(cpu)	\
+	x2apic_propagate_cpu_cluster_status(cpu, 0)

Is there any particular reason why we use preprocessor technology here, instead of the fine C language that .c files are generally written in?

Enumerating 0/1 symbolically would be cleaner that introducing two smaller helper functions in any case.

Quoted 5 lines

arch/x86/kernel/apic/x2apic_cluster.c +5 -0

+static int __cpuinit
+cluster_setup(struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	int err = 0;

arch/x86/kernel/apic/x2apic_cluster.c static void init_x2apic_ldr(void) · line 238

Quoted 10 lines

arch/x86/kernel/apic/x2apic_cluster.c +10 -0

+
+	switch (action) {
+	case CPU_UP_PREPARE:
+		zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+		zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+		if (!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu)) {
+			free_cpumask_var(per_cpu(cpus_in_cluster, cpu));
+			free_cpumask_var(per_cpu(ipi_mask, cpu));
+			err = -ENOMEM;
+		}

zalloc_cpumask_var() has a return code that could be used for a slightly more standard deinit/teardown sequence.

arch/x86/kernel/apic/x2apic_cluster.c static void init_x2apic_ldr(void) · line 266

Quoted 7 lines

arch/x86/kernel/apic/x2apic_cluster.c +6 -0

+void x2apic_init_cpu_notifier(void)
+{
+	int cpu = smp_processor_id();
 
+	zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+	zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+	BUG_ON(!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu));

Such a BUG_ON() is not particularly user friendly - and this could trigger during CPU hotplug events, i.e. while the system is fully booted up, right?

Thanks,

Ingo

Cyrill Gorcunov

author

2011-02-14 15:10 UTC #2 lore raw

Quoted 8 lines · Ingo Molnar

Quoted 5 lines · Cyrill Gorcunov
In the case of x2apic cluster mode we can group IPI register writes based on the cluster group instead of individual per-cpu destiantion messages. This reduces the apic register writes and reduces the amount of IPI messages (in the best case we can reduce it by a factor of 16).

Quoted 8 lines

Quoted 4 lines
With this change, microbenchmark measuring the cost of flush_tlb_others(), with the flush tlb IPI being sent from a cpu in the socket-1 to all the logical cpus in socket-2 (on a Westmere-EX system that has 20 logical cpus in a socket) is 3x times better now (compared to the former 'send one-by-one' algorithm).
Pretty nice!
I have a few structural and nitpicking comments:

Thanks a lot for review, Ingo! I'll address all the nits during this week.

...

Quoted 12 lines

arch/x86/kernel/apic/x2apic_cluster.c static void init_x2apic_ldr(void) · line 266
Quoted 8 lines
arch/x86/kernel/apic/x2apic_cluster.c +6 -0
+void x2apic_init_cpu_notifier(void)
+{
+	int cpu = smp_processor_id();
 
+	zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+	zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+	BUG_ON(!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu));
Such a BUG_ON() is not particularly user friendly - and this could trigger during CPU hotplug events, i.e. while the system is fully booted up, right?

Quoted 3 lines

Thanks,
Ingo

Yup is not that much friendly but it's called during system bootup, hotplug events are handled by

+static int __cpuinit
+cluster_setup(struct notifier_block *nfb, unsigned long action, void *hcpu) +{
+ unsigned int cpu = (unsigned long)hcpu;
+ int err = 0;
+
+ switch (action) {
+ case CPU_UP_PREPARE:
+ zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+ zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+ if (!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu)) {
+ free_cpumask_var(per_cpu(cpus_in_cluster, cpu));
+ free_cpumask_var(per_cpu(ipi_mask, cpu));
+ err = -ENOMEM;
+ }
+ break;

so it returns -ENOMEM if failed. And btw just noted that we forgot to make x2apic_init_cpu_notifier being in __init section.

Or I miss something?

Signature · 1 line

    Cyrill

Ingo Molnar

2011-02-15 03:23 UTC #3 lore raw

Quoted 57 lines · Cyrill Gorcunov

Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Cyrill Gorcunov
In the case of x2apic cluster mode we can group IPI register writes based on the cluster group instead of individual per-cpu destiantion messages. This reduces the apic register writes and reduces the amount of IPI messages (in the best case we can reduce it by a factor of 16).
Quoted 8 lines
Quoted 4 lines
With this change, microbenchmark measuring the cost of flush_tlb_others(), with the flush tlb IPI being sent from a cpu in the socket-1 to all the logical cpus in socket-2 (on a Westmere-EX system that has 20 logical cpus in a socket) is 3x times better now (compared to the former 'send one-by-one' algorithm).
Pretty nice!
I have a few structural and nitpicking comments:
Thanks a lot for review, Ingo! I'll address all the nits during this week.
...
Quoted 12 lines
arch/x86/kernel/apic/x2apic_cluster.c static void init_x2apic_ldr(void) · line 266
Quoted 8 lines
arch/x86/kernel/apic/x2apic_cluster.c +6 -0
+void x2apic_init_cpu_notifier(void)
+{
+	int cpu = smp_processor_id();
 
+	zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+	zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+	BUG_ON(!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu));
Such a BUG_ON() is not particularly user friendly - and this could trigger during CPU hotplug events, i.e. while the system is fully booted up, right?
Quoted 3 lines
Thanks,
Ingo
Yup is not that much friendly but it's called during system bootup, hotplug events are handled by
arch/x86/kernel/apic/x2apic_cluster.c +16 -0
+static int __cpuinit
+cluster_setup(struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	int err = 0;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+		zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+		zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+		if (!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu)) {
+			free_cpumask_var(per_cpu(cpus_in_cluster, cpu));
+			free_cpumask_var(per_cpu(ipi_mask, cpu));
+			err = -ENOMEM;
+		}
+		break;

Quoted 4 lines

so it returns -ENOMEM if failed. And btw just noted that we forgot to make x2apic_init_cpu_notifier being in __init section.
Or I miss something?

Is there no GFP_NOFAIL or GFP_FAIL_ON_PANIC variant that could be used the 'must not fail' property of the boot-time allocation?

Thanks,

Ingo

Cyrill Gorcunov

author

2011-02-15 08:39 UTC #4 lore raw

Quoted 77 lines · Ingo Molnar

Quoted 65 lines · Cyrill Gorcunov
Quoted 19 lines · Ingo Molnar
Quoted 8 lines · Cyrill Gorcunov
In the case of x2apic cluster mode we can group IPI register writes based on the
cluster group instead of individual per-cpu destiantion messages. This reduces the
apic register writes and reduces the amount of IPI messages (in the best case we
can reduce it by a factor of 16).
Quoted 7 lines
With this change, microbenchmark measuring the cost of flush_tlb_others(), with
the flush tlb IPI being sent from a cpu in the socket-1 to all the logical cpus in
socket-2 (on a Westmere-EX system that has 20 logical cpus in a socket) is 3x
times better now (compared to the former 'send one-by-one' algorithm).
Quoted 3 lines
Pretty nice!
I have a few structural and nitpicking comments:
Thanks a lot for review, Ingo! I'll address all the nits during this week.
...
Quoted 13 lines
arch/x86/kernel/apic/x2apic_cluster.c static void init_x2apic_ldr(void) · line 266
Quoted 8 lines
arch/x86/kernel/apic/x2apic_cluster.c +6 -0
+void x2apic_init_cpu_notifier(void)
+{
+	int cpu = smp_processor_id();
 
+	zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+	zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+	BUG_ON(!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu));
Such a BUG_ON() is not particularly user friendly - and this could trigger during
CPU hotplug events, i.e. while the system is fully booted up, right?
Quoted 3 lines
Thanks,
Ingo
Yup is not that much friendly but it's called during system bootup, hotplug events are handled by
(quoted diff) +2 -0
+static int __cpuinit
+cluster_setup(struct notifier_block *nfb, unsigned long action, void
*hcpu)
arch/x86/kernel/apic/x2apic_cluster.c +14 -0
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	int err = 0;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+		zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+		zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+		if (!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu)) {
+			free_cpumask_var(per_cpu(cpus_in_cluster, cpu));
+			free_cpumask_var(per_cpu(ipi_mask, cpu));
+			err = -ENOMEM;
+		}
+		break;
Quoted 4 lines
so it returns -ENOMEM if failed. And btw just noted that we forgot to make x2apic_init_cpu_notifier being in __init section.
Or I miss something?
Is there no GFP_NOFAIL or GFP_FAIL_ON_PANIC variant that could be used the 'must not
fail' property of the boot-time allocation?

Quoted 4 lines

Thanks,
Ingo

If only i'm not missing something obvious we can set GFP_NOFAIL ending in endless loop if allocation failed (slab should be already running at this point of boot). Probably another option might be to switch to no-apic mode if there is no enough memory to allocate this masks (though i guess if allocation failed at this point we likely to fail in further allocations in kernel anyway)

Ingo Molnar

2011-02-16 09:23 UTC #5 lore raw

Quoted 83 lines · Cyrill Gorcunov

Quoted 77 lines · Ingo Molnar
Quoted 65 lines · Cyrill Gorcunov
Quoted 19 lines · Ingo Molnar
Quoted 8 lines · Cyrill Gorcunov
In the case of x2apic cluster mode we can group IPI register writes based on the
cluster group instead of individual per-cpu destiantion messages. This reduces the
apic register writes and reduces the amount of IPI messages (in the best case we
can reduce it by a factor of 16).
Quoted 7 lines
With this change, microbenchmark measuring the cost of flush_tlb_others(), with
the flush tlb IPI being sent from a cpu in the socket-1 to all the logical cpus in
socket-2 (on a Westmere-EX system that has 20 logical cpus in a socket) is 3x
times better now (compared to the former 'send one-by-one' algorithm).
Quoted 3 lines
Pretty nice!
I have a few structural and nitpicking comments:
Thanks a lot for review, Ingo! I'll address all the nits during this week.
...
Quoted 13 lines
arch/x86/kernel/apic/x2apic_cluster.c static void init_x2apic_ldr(void) · line 266
Quoted 8 lines
arch/x86/kernel/apic/x2apic_cluster.c +6 -0
+void x2apic_init_cpu_notifier(void)
+{
+	int cpu = smp_processor_id();
 
+	zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+	zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+	BUG_ON(!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu));
Such a BUG_ON() is not particularly user friendly - and this could trigger during
CPU hotplug events, i.e. while the system is fully booted up, right?
Quoted 3 lines
Thanks,
Ingo
Yup is not that much friendly but it's called during system bootup, hotplug events are handled by
(quoted diff) +2 -0
+static int __cpuinit
+cluster_setup(struct notifier_block *nfb, unsigned long action, void
*hcpu)
arch/x86/kernel/apic/x2apic_cluster.c +14 -0
+{
+	unsigned int cpu = (unsigned long)hcpu;
+	int err = 0;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+		zalloc_cpumask_var(&per_cpu(cpus_in_cluster, cpu), GFP_KERNEL);
+		zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL);
+		if (!per_cpu(cpus_in_cluster, cpu) || !per_cpu(ipi_mask, cpu)) {
+			free_cpumask_var(per_cpu(cpus_in_cluster, cpu));
+			free_cpumask_var(per_cpu(ipi_mask, cpu));
+			err = -ENOMEM;
+		}
+		break;
Quoted 4 lines
so it returns -ENOMEM if failed. And btw just noted that we forgot to make x2apic_init_cpu_notifier being in __init section.
Or I miss something?
Is there no GFP_NOFAIL or GFP_FAIL_ON_PANIC variant that could be used the 'must not
fail' property of the boot-time allocation?
Quoted 4 lines
Thanks,
Ingo

Quoted 6 lines

If only i'm not missing something obvious we can set GFP_NOFAIL ending in endless loop if allocation failed (slab should be already running at this point of boot). Probably another option might be to switch to no-apic mode if there is no enough memory to allocate this masks (though i guess if allocation failed at this point we likely to fail in further allocations in kernel anyway)

Ok, if it's boot time only then it's no big deal.

Thanks,

Ingo