x2apic optimization, v4 log updated

PATCH In review Cyrill Gorcunov 4 participants 11 messages May 2, 2011 – May 3, 2011 x86

lore

Share & tools

lore

Cyrill Gorcunov

author

2011-05-02 11:40 UTC #0 lore raw

cover x2apic optimization, v4 log updated

Hi Ingo, this series consist of updated commit messages,
so I hope this time I made all things right.

Please ping me if something still not confirm the submitting
patch guide.

Suresh, I've putted your ack and tested-by in a sake of consistency
with submitting patch guide.

Thanks,
Cyrill

Cyrill Gorcunov

author

2011-05-02 11:40 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).

v2: Suresh fixed cpumask allocation to be dynamic,
stack allocation is not acceptable for such things because can be exhausted.

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.

Tested-by Suresh Siddha <[email protected]>

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

Acked-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-02 13:22 UTC #2 lore raw

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

Quoted 3 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.

typo.

Quoted 10 lines

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

What kind of microbenchmark was this, could the actual results and measurement methods be shared as well?

Quoted 57 lines

v2: Suresh fixed cpumask allocation to be dynamic,
stack allocation is not acceptable for such things because can be exhausted.
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.
Tested-by Suresh Siddha <[email protected]>
Signed-off-by Cyrill Gorcunov <[email protected]>
Acked-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(-)
Index: tip-linux-2.6/arch/x86/include/asm/apic.h
===================================================================
arch/x86/include/asm/apic.h +2 -0
@@ -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;
Index: tip-linux-2.6/arch/x86/kernel/apic/probe_64.c
===================================================================
arch/x86/kernel/apic/probe_64.c +9 -0
@@ -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

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

Quoted 7 lines

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

@@ -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();

Why is there an x2apic specific function in the generic default_setup_apic_routing() function?

Instead of that it would be cleaner to extend the apic driver functions with an init method, which would be filled in for x2apic and left NULL for the others.

Thanks,

Ingo

Cyrill Gorcunov

author

2011-05-02 14:02 UTC #3 lore raw

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

Quoted 9 lines · Ingo Molnar

Quoted 3 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.
typo.

ok, will fix, thanks.

Quoted 11 lines

Quoted 10 lines
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).

Quoted 2 lines

What kind of microbenchmark was this, could the actual results and measurement methods be shared as well?

Suresh, could you please post the microbenchmark?
...

Quoted 33 lines

Quoted 21 lines
Index: tip-linux-2.6/arch/x86/kernel/apic/probe_64.c
===================================================================
arch/x86/kernel/apic/probe_64.c +9 -0
@@ -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
arch/x86/kernel/apic/probe_64.c void __init default_setup_apic_routing(v · line 85
Quoted 7 lines
arch/x86/kernel/apic/probe_64.c +3 -1
@@ -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();
Why is there an x2apic specific function in the generic default_setup_apic_routing() function?

Quoted 6 lines

Instead of that it would be cleaner to extend the apic driver functions with an init method, which would be filled in for x2apic and left NULL for the others.
Thanks,
Ingo

Ingo, the idea was to merge probe_x.c completely, and put all this not into init()
but rather into apic->probe() or something like that. I don't have a clear picture in mind yet what the best way would be, so instead of fast designed method I thought to leave it opencoded with fixme note.

So lets wait until Suresh post the benchmark and I will make apic->init() meanwhile.

Signature · 1 line

    Cyrill

Cyrill Gorcunov

2011-05-02 14:23 UTC #4 lore raw

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

On 05/02/2011 06:02 PM, Cyrill Gorcunov wrote:
...

Quoted 8 lines

Ingo, the idea was to merge probe_x.c completely, and put all this not into init()
but rather into apic->probe() or something like that. I don't have a clear picture in mind yet what the best way would be, so instead of fast designed method I thought to leave it opencoded with fixme note.
So lets wait until Suresh post the benchmark and I will make apic->init() meanwhile.

Ingo, would it be fine to make apic->init() either _before_ this series or on
top of them (because if I introduce it inside this particular patch it would contain some unrelated code snippets such as .init = NULL for all apics declaration).

Signature · 2 lines

Thanks,
  Cyrill

Ingo Molnar

2011-05-02 15:06 UTC #5 lore raw

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

Quoted 8 lines · Cyrill Gorcunov

On 05/02/2011 06:02 PM, Cyrill Gorcunov wrote:
...
Quoted 6 lines
Ingo, the idea was to merge probe_x.c completely, and put all this not into init()
but rather into apic->probe() or something like that. I don't have a clear picture in mind yet what the best way would be, so instead of fast designed method I thought to leave it opencoded with fixme note.

Quoted 7 lines

Quoted 2 lines
So lets wait until Suresh post the benchmark and I will make apic->init() meanwhile.
Ingo, would it be fine to make apic->init() either _before_ this series or
on top of them (because if I introduce it inside this particular patch it would contain some unrelated code snippets such as .init = NULL for all apics declaration).

Of course it should be a separate patch - even this patch looks a bit large - any way to split it up further?

And yes, if it fits it should be in apic->probe() instead of introducing a new ->init() method.

Thanks,

Ingo

Cyrill Gorcunov

2011-05-02 15:16 UTC #6 lore raw

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

On 05/02/2011 07:05 PM, Ingo Molnar wrote:
...

Quoted 8 lines

Quoted 5 lines
Ingo, would it be fine to make apic->init() either _before_ this series or
on top of them (because if I introduce it inside this particular patch it would contain some unrelated code snippets such as .init = NULL for all apics declaration).
Of course it should be a separate patch - even this patch looks a bit large - any way to split it up further?

Well, for this particular path the only minimum is used, so i fear there is no
way to split it, probably I could drop some 'cleanup' bits from it and make it a separate one. Gimme some time.

Quoted 3 lines

And yes, if it fits it should be in apic->probe() instead of introducing a new ->init() method.

I'll recheck. Thanks for all comments Ingo, drop this series for a while then.

Quoted 4 lines

Thanks,
Ingo

Signature · 2 lines

Thanks,
  Cyrill

Ingo Molnar

2011-05-03 06:31 UTC #7 lore raw

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

Quoted 8 lines · Cyrill Gorcunov

On 05/02/2011 07:05 PM, Ingo Molnar wrote:
...
Quoted 6 lines
Quoted 5 lines
Ingo, would it be fine to make apic->init() either _before_ this series or
on top of them (because if I introduce it inside this particular patch it would contain some unrelated code snippets such as .init = NULL for all apics declaration).

Quoted 6 lines

Quoted 2 lines
Of course it should be a separate patch - even this patch looks a bit large - any way to split it up further?
Well, for this particular path the only minimum is used, so i fear there is no
way to split it, probably I could drop some 'cleanup' bits from it and make it a separate one. Gimme some time.

Well, first try to do *all* preparatory and cleanup changes that have low regression risk.

*Then* keep the most dangerous part to the end of it - so that it's easily reverted, should the need arise. Preferably the dangerous part should be much smaller than:

 3 files changed, 136 insertions(+), 46 deletions(-)

And no, it is not at all true that there is 'no way' to split the patch up any
further: you could certainly add the data structures, init methods and such support code (which is low regression risk) in a separate patch than the changes that modify the existing x2apic_send_IPI_mask_allbutself() function and such.

Also, the loop body in the new __x2apic_send_IPI_mask() function could certainly be split out into a helper inline, making the code flow clearer.

Thanks,

Ingo

Cyrill Gorcunov

2011-05-03 06:59 UTC #8 lore raw

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

Quoted 14 lines · Ingo Molnar

Quoted 11 lines · Cyrill Gorcunov
On 05/02/2011 07:05 PM, Ingo Molnar wrote:
...
Quoted 8 lines
Quoted 5 lines
Ingo, would it be fine to make apic->init() either _before_ this series or
on top of them (because if I introduce it inside this particular patch it would contain some unrelated code snippets such as .init = NULL for all apics declaration).
Of course it should be a separate patch - even this patch looks a bit large - any way to split it up further?

Quoted 6 lines

Quoted 3 lines
Well, for this particular path the only minimum is used, so i fear there is no
way to split it, probably I could drop some 'cleanup' bits from it and make it a separate one. Gimme some time.
Well, first try to do *all* preparatory and cleanup changes that have low regression risk.

Quoted 5 lines

*Then* keep the most dangerous part to the end of it - so that it's easily reverted, should the need arise. Preferably the dangerous part should be much smaller than:

Quoted 7 lines

 3 files changed, 136 insertions(+), 46 deletions(-)
And no, it is not at all true that there is 'no way' to split the patch up any
further: you could certainly add the data structures, init methods and such support code (which is low regression risk) in a separate patch than the changes that modify the existing x2apic_send_IPI_mask_allbutself() function and such.

Quoted 3 lines

Also, the loop body in the new __x2apic_send_IPI_mask() function could certainly be split out into a helper inline, making the code flow clearer.

OK, will try so, thanks Ingo!

Quoted 4 lines

Thanks,
Ingo

Signature · 2 lines

Thanks,
  Cyrill

Suresh Siddha

2011-05-02 18:26 UTC #9 lore raw

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

In reply to Cyrill Gorcunov

Quoted 11 lines · Cyrill Gorcunov

Quoted 10 lines · Ingo Molnar
Quoted 6 lines · Cyrill Gorcunov
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 4 lines

Quoted 2 lines
What kind of microbenchmark was this, could the actual results and measurement methods be shared as well?
Suresh, could you please post the microbenchmark?

It is a simple kernel hack to measure the TSC cost of flush_tlb_others() with and with out this change. 3x better was specifically for the test condition where we called flush_tlb_others() on a logical cpu in socket-1, which sent the flush tlb IPI to all the logical cpu's in another socket.

This is done on WSM-EX which has 20 logical cpu's on one socket. 20 logical cpu's in that socket will fall under two cluster groups. So 2 batches of grouped IPI's vs 20 serialized(atleast the sending part) IPI's.

thanks,
suresh

Cyrill Gorcunov

author

2011-05-02 11:40 UTC #10 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]>

Acked-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