x2apic optimization, round 4

PATCH In review Cyrill Gorcunov 5 participants 14 messages Apr 30, 2011 – May 2, 2011 x86

lore

Share & tools

lore

Cyrill Gorcunov

author

2011-04-30 17:20 UTC #0 lore raw

cover x2apic optimization, round 4

Hi Ingo, here is an updated series of two patches for
x2apic cluster mode ipi optimization. Please take a look.

The only concern which was unresolved is a messy bit
in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.

Anyway I think patches should be in a good shape now and
probably we could merge them to not lose?

Comments are welcome.

Thanks,
Cyrill

Cyrill Gorcunov

author

2011-04-30 17:21 UTC #1 lore raw

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

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).

v3: Address Ingo concerns on code style, also a note
added just to not forget that we need merge probe_64/32 into some common structure.

v4: Suresh discovered (and fixed) that cluster
infomation must be updated at CPU_UP_PREPARE state otherwise if IPI happens too early we will be in touble having incomplete cluster sibling map.

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       |   13 ++
 arch/x86/kernel/apic/x2apic_cluster.c |  167 ++++++++++++++++++++++++----------
 3 files changed, 136 insertions(+), 46 deletions(-)

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

Index: tip-linux-2.6/arch/x86/include/asm/apic.h
===================================================================
@@ -178,6 +178,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;

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

Index: tip-linux-2.6/arch/x86/kernel/apic/probe_64.c
===================================================================
@@ -55,6 +55,15 @@ static int apicid_phys_pkg_id(int initia
 void __init default_setup_apic_routing(void)
 {
 
+	/*
+	 * FIXME:
+	 *
+	 * Cleanup the apic routing selection by having an apic driver specific
+	 * selection routine. Then all we need to do here is iterate through
+	 * them to finalize the apic selection. That would get rid of the
+	 * ifdef mess and most of the code here.
+	 */
+
 	enable_IR_x2apic();
 
 #ifdef CONFIG_X86_X2APIC

@@ -71,7 +80,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 +122 ~~-45~~

230 lines not on this page · raw message

Ingo Molnar

2011-05-01 17:18 UTC #2 lore raw

Re: [patch 1/2] x86, x2apic: minimize IPI register writes using cluster groups v4

Quoted 2 lines · Cyrill Gorcunov

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

This SOB section is wrong, the last SOB must be the person sending the patch. Please see section "13)" in Documentation/Submittingpatches.

Thanks,

Ingo

Cyrill Gorcunov

author

2011-04-30 17:21 UTC #3 lore raw

PATCH 2/2 x86, x2apic: Move the common bits of physical and cluster modes to x2apic.h v4

In reply to Cyrill Gorcunov

To eliminate code duplication.

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

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

---
 arch/x86/include/asm/x2apic.h         |   64 ++++++++++++++++++++++++++++++
 arch/x86/kernel/apic/x2apic_cluster.c |   71 +---------------------------------
 arch/x86/kernel/apic/x2apic_phys.c    |   71 +++-------------------------------
 3 files changed, 76 insertions(+), 130 deletions(-)

arch/x86/include/asm/x2apic.h new file +64 -0

Index: tip-linux-2.6/arch/x86/include/asm/x2apic.h
===================================================================
@@ -0,0 +1,64 @@
+/*
+ * Common bits for X2APIC cluster/physical modes.
+ */
+
+#ifndef _ASM_X86_X2APIC_H
+#define _ASM_X86_X2APIC_H
+
+#include <asm/apic.h>
+#include <asm/ipi.h>
+#include <linux/cpumask.h>
+
+/*
+ * Need to use more than cpu 0, because we need more vectors
+ * when MSI-X are used.
+ */
+static const struct cpumask *x2apic_target_cpus(void)
+{
+	return cpu_online_mask;
+}
+
+static int x2apic_apic_id_registered(void)
+{
+	return 1;
+}
+
+/*
+ * For now each logical cpu is in its own vector allocation domain.
+ */
+static void x2apic_vector_allocation_domain(int cpu, struct cpumask *retmask)
+{
+	cpumask_clear(retmask);
+	cpumask_set_cpu(cpu, retmask);
+}
+
+static void
+__x2apic_send_IPI_dest(unsigned int apicid, int vector, unsigned int dest)
+{
+	unsigned long cfg = __prepare_ICR(0, vector, dest);
+	native_x2apic_icr_write(cfg, apicid);
+}
+
+static unsigned int x2apic_get_apic_id(unsigned long id)
+{
+	return id;
+}
+
+static unsigned long x2apic_set_apic_id(unsigned int id)
+{
+	return id;
+}
+
+static int x2apic_phys_pkg_id(int initial_apicid, int index_msb)
+{
+	return initial_apicid >> index_msb;
+}
+
+static void x2apic_send_IPI_self(int vector)
+{
+	apic_write(APIC_SELF_IPI, vector);
+}
+
+void x2apic_init_cpu_notifier(void);
+
+#endif /* _ASM_X86_X2APIC_H */

arch/x86/kernel/apic/x2apic_cluster.c +4 ~~-67~~

103 lines not on this page · raw message

arch/x86/kernel/apic/x2apic_phys.c +8 ~~-63~~

118 lines not on this page · raw message

Avi Kivity

2011-04-30 20:15 UTC #4 lore raw

In reply to Cyrill Gorcunov

Quoted 10 lines · Cyrill Gorcunov

Hi Ingo, here is an updated series of two patches for
x2apic cluster mode ipi optimization. Please take a look.
The only concern which was unresolved is a messy bit
in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.

Quoted 4 lines

Anyway I think patches should be in a good shape now and
probably we could merge them to not lose?
Comments are welcome.

Very nice! This will also significantly speed up kvm guests using x2apic.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Cyrill Gorcunov

2011-04-30 20:34 UTC #5 lore raw

Quoted 11 lines · Avi Kivity

Quoted 10 lines · Cyrill Gorcunov
Hi Ingo, here is an updated series of two patches for
x2apic cluster mode ipi optimization. Please take a look.
The only concern which was unresolved is a messy bit
in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.

Quoted 7 lines

Quoted 4 lines
Anyway I think patches should be in a good shape now and
probably we could merge them to not lose?
Comments are welcome.
Very nice! This will also significantly speed up kvm guests using x2apic.

Yeah, indeed, i somehow forgot that we have x2apic mode in kvm (and just remember you've been pointing me already about x2apic parameter for kvm ;)

Signature · 2 lines

Thanks,
  Cyrill

Ingo Molnar

2011-05-01 16:46 UTC #6 lore raw

In reply to Avi Kivity

Quoted 11 lines · Avi Kivity

Quoted 10 lines · Cyrill Gorcunov
Hi Ingo, here is an updated series of two patches for
x2apic cluster mode ipi optimization. Please take a look.
The only concern which was unresolved is a messy bit
in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.

Quoted 6 lines

Quoted 4 lines
Anyway I think patches should be in a good shape now and
probably we could merge them to not lose?
Comments are welcome.
Very nice! This will also significantly speed up kvm guests using x2apic.

Ok. It would be nice to know on what systems these patches were tested, i.e. i suppose x2apic-enabled ones?

Thanks,

Ingo

Cyrill Gorcunov

author

2011-05-01 17:11 UTC #7 lore raw

Quoted 19 lines · Ingo Molnar

Quoted 16 lines · Avi Kivity
Quoted 14 lines · Cyrill Gorcunov
Hi Ingo, here is an updated series of two patches for x2apic cluster mode ipi optimization. Please take a look.
The only concern which was unresolved is a messy bit in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.
Anyway I think patches should be in a good shape now and probably we could merge them to not lose?
Comments are welcome.

Quoted 9 lines

Quoted 1 line
Very nice! This will also significantly speed up kvm guests using x2apic.
Ok. It would be nice to know on what systems these patches were tested, i.e. i suppose x2apic-enabled ones?
Thanks,
Ingo

Suresh has tested them on Westmere-Ex system, I personally don't have such machine.

Gleb Natapov

2011-05-02 05:39 UTC #8 lore raw

Quoted 25 lines · Cyrill Gorcunov

Quoted 24 lines · Ingo Molnar
Quoted 11 lines · Avi Kivity
Quoted 10 lines · Cyrill Gorcunov
Hi Ingo, here is an updated series of two patches for x2apic cluster mode ipi optimization. Please take a look.
The only concern which was unresolved is a messy bit in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.
Quoted 6 lines
Quoted 4 lines
Anyway I think patches should be in a good shape now and probably we could merge them to not lose?
Comments are welcome.
Very nice! This will also significantly speed up kvm guests using x2apic.
Ok. It would be nice to know on what systems these patches were tested, i.e. i suppose x2apic-enabled ones?

Quoted 7 lines

Quoted 4 lines
Thanks,
Ingo
Suresh has tested them on Westmere-Ex system, I personally don't have such machine.

Can you test in in KVM with x2apic emulation enabled?

--
Gleb.

Cyrill Gorcunov

author

2011-05-02 05:51 UTC #9 lore raw

Yeah, i'll do so as only bring my laptop to life, I managed to kill grub and can't restore it back for a while, will ping you back as soon as I can.

Sorry for top post.

Quoted 38 lines · Gleb Natapov

Quoted 25 lines · Cyrill Gorcunov
Quoted 24 lines · Ingo Molnar
Quoted 11 lines · Avi Kivity
Quoted 10 lines · Cyrill Gorcunov
Hi Ingo, here is an updated series of two patches for x2apic cluster mode ipi optimization. Please take a look.
The only concern which was unresolved is a messy bit in default_setup_apic_routing(), so we've added a FIXME there and clean it up as only time permits. Actually I have a plan to merge probe_32/64.c into single file (and even started working on this task) but not yes succeed.
Quoted 6 lines
Quoted 4 lines
Anyway I think patches should be in a good shape now and probably we could merge them to not lose?
Comments are welcome.
Very nice! This will also significantly speed up kvm guests using x2apic.
Ok. It would be nice to know on what systems these patches were tested, i.e. i suppose x2apic-enabled ones?
Quoted 7 lines
Quoted 4 lines
Thanks,
Ingo
Suresh has tested them on Westmere-Ex system, I personally don't have such machine.
Can you test in in KVM with x2apic emulation enabled?
--
Gleb.

Cyrill Gorcunov

2011-05-02 07:09 UTC #10 lore raw

Quoted 6 lines · Cyrill Gorcunov

Yeah, i'll do so as only bring my laptop to life, I managed to kill grub and can't restore it back for a while, will ping you back as soon as I can.
Sorry for top post.

I've managed to semi-restore it. Gleb, maybe there some additional
parameter needed for kvm (except -cpu host,+x2apic) to make it work with cluster mode? I always see physical mode which was not the target of the patch series.

Signature · 2 lines

Thanks,
  Cyrill

Gleb Natapov

2011-05-02 07:45 UTC #11 lore raw

Quoted 5 lines · Cyrill Gorcunov

Quoted 4 lines · Cyrill Gorcunov
Yeah, i'll do so as only bring my laptop to life, I managed to kill grub and can't restore it back for a while, will ping you back as soon as I can.

Quoted 8 lines

Quoted 2 lines
Sorry for top post.
I've managed to semi-restore it. Gleb, maybe there some additional
parameter needed for kvm (except -cpu host,+x2apic) to make it work with cluster mode? I always see physical mode which was not the target of the patch series.

Hmm, +x2apic is enough to advertise x2apic presents to the OS. What logic Linux uses to decide which mode to use?

--
Gleb.

Cyrill Gorcunov

2011-05-02 07:52 UTC #12 lore raw

Quoted 14 lines · Gleb Natapov

Quoted 5 lines · Cyrill Gorcunov
Quoted 4 lines · Cyrill Gorcunov
Yeah, i'll do so as only bring my laptop to life, I managed to kill grub and can't restore it back for a while, will ping you back as soon as I can.
Quoted 8 lines
Quoted 2 lines
Sorry for top post.
I've managed to semi-restore it. Gleb, maybe there some additional
parameter needed for kvm (except -cpu host,+x2apic) to make it work with cluster mode? I always see physical mode which was not the target of the patch series.

Quoted 5 lines

Hmm, +x2apic is enough to advertise x2apic presents to the OS. What logic Linux uses to decide which mode to use?
--
Gleb.

As far as I know interrupt remapping should be supported by kernel
to switch to logic cluster mode, so I presume kvm doesn't support it yet and in result only physical mode is used. I think ACPI_SIG_DMAR is missed in acpi tables (actually I didn't find it in kvm sources).

Signature · 2 lines

Thanks,
  Cyrill

Gleb Natapov

2011-05-02 08:04 UTC #13 lore raw

Quoted 18 lines · Cyrill Gorcunov

Quoted 17 lines · Gleb Natapov
Quoted 5 lines · Cyrill Gorcunov
Quoted 4 lines · Cyrill Gorcunov
Yeah, i'll do so as only bring my laptop to life, I managed to kill grub and can't restore it back for a while, will ping you back as soon as I can.
Quoted 8 lines
Quoted 2 lines
Sorry for top post.
I've managed to semi-restore it. Gleb, maybe there some additional
parameter needed for kvm (except -cpu host,+x2apic) to make it work with cluster mode? I always see physical mode which was not the target of the patch series.
Hmm, +x2apic is enough to advertise x2apic presents to the OS. What logic Linux uses to decide which mode to use?

Quoted 8 lines

Quoted 2 lines
--
Gleb.
As far as I know interrupt remapping should be supported by kernel
to switch to logic cluster mode, so I presume kvm doesn't support it yet and in result only physical mode is used. I think ACPI_SIG_DMAR is missed in acpi tables (actually I didn't find it in kvm sources).

Ahh, definitely. I recall that now.

--
Gleb.