[patch 0/2] x2apic optimization, round 4

[patch 0/2] x2apic optimization, round 4

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

Thanks,
  Cyrill

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

[Cyrill Gorcunov]

[-- Attachment #1: x86-x2apic-optimise-cluster-mode-v4 --]
[-- Type: text/plain, Size: 9461 bytes --]

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 <gorcunov@openvz.org>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---
 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
===================================================================
--- tip-linux-2.6.orig/arch/x86/include/asm/apic.h
+++ 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;
Index: tip-linux-2.6/arch/x86/kernel/apic/probe_64.c
===================================================================
--- tip-linux-2.6.orig/arch/x86/kernel/apic/probe_64.c
+++ 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);
 
Index: tip-linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
===================================================================
--- tip-linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c
+++ tip-linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
@@ -5,12 +5,15 @@
 #include <linux/ctype.h>
 #include <linux/init.h>
 #include <linux/dmar.h>
+#include <linux/cpu.h>
 
 #include <asm/smp.h>
 #include <asm/apic.h>
 #include <asm/ipi.h>
 
 static DEFINE_PER_CPU(u32, x86_cpu_to_logical_apicid);
+static DEFINE_PER_CPU(cpumask_var_t, cpus_in_cluster);
+static DEFINE_PER_CPU(cpumask_var_t, ipi_mask);
 
 static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
@@ -36,7 +39,7 @@ static void x2apic_vector_allocation_dom
 }
 
 static void
- __x2apic_send_IPI_dest(unsigned int apicid, int vector, unsigned int dest)
+__x2apic_send_IPI_dest(unsigned int apicid, int vector, unsigned int dest)
 {
 	unsigned long cfg;
 
@@ -48,70 +51,80 @@ static void
 	native_x2apic_icr_write(cfg, apicid);
 }
 
-/*
- * for now, we send the IPI's one by one in the cpumask.
- * TBD: Based on the cpu mask, we can send the IPI's to the cluster group
- * at once. We have 16 cpu's in a cluster. This will minimize IPI register
- * writes.
- */
-static void x2apic_send_IPI_mask(const struct cpumask *mask, int vector)
+static inline u32 x2apic_cluster(int cpu)
 {
-	unsigned long query_cpu;
-	unsigned long flags;
-
-	x2apic_wrmsr_fence();
-
-	local_irq_save(flags);
-	for_each_cpu(query_cpu, mask) {
-		__x2apic_send_IPI_dest(
-			per_cpu(x86_cpu_to_logical_apicid, query_cpu),
-			vector, apic->dest_logical);
-	}
-	local_irq_restore(flags);
+	return per_cpu(x86_cpu_to_logical_apicid, cpu) >> 16;
 }
 
 static void
- x2apic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
+__x2apic_send_IPI_mask(const struct cpumask *mask, int vector, int exclude_self)
 {
-	unsigned long this_cpu = smp_processor_id();
-	unsigned long query_cpu;
+	struct cpumask *cpus_in_cluster_ptr;
+	struct cpumask *ipi_mask_ptr;
+	unsigned int cpu, this_cpu;
 	unsigned long flags;
+	u32 dest;
 
 	x2apic_wrmsr_fence();
 
 	local_irq_save(flags);
-	for_each_cpu(query_cpu, mask) {
-		if (query_cpu == this_cpu)
+	this_cpu = smp_processor_id();
+
+	/*
+	 * We are to modify mask, so we need an own copy
+	 * and be sure it's manipulated with irq off.
+	 */
+	ipi_mask_ptr = __raw_get_cpu_var(ipi_mask);
+	cpumask_copy(ipi_mask_ptr, mask);
+
+	/*
+	 * The idea is to send one IPI per cluster.
+	 */
+	for_each_cpu(cpu, ipi_mask_ptr) {
+		unsigned long i;
+
+		cpus_in_cluster_ptr = per_cpu(cpus_in_cluster, cpu);
+		dest = 0;
+
+		/* Collect cpus in cluster. */
+		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);
+		}
+
+		if (!dest)
 			continue;
-		__x2apic_send_IPI_dest(
-				per_cpu(x86_cpu_to_logical_apicid, query_cpu),
-				vector, apic->dest_logical);
+
+		__x2apic_send_IPI_dest(dest, vector, apic->dest_logical);
+		/*
+		 * Cluster sibling cpus should be discared now so
+		 * we would not send IPI them second time.
+		 */
+		cpumask_andnot(ipi_mask_ptr, ipi_mask_ptr, cpus_in_cluster_ptr);
 	}
+
 	local_irq_restore(flags);
 }
 
-static void x2apic_send_IPI_allbutself(int vector)
+static void
+x2apic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
 {
-	unsigned long this_cpu = smp_processor_id();
-	unsigned long query_cpu;
-	unsigned long flags;
+	__x2apic_send_IPI_mask(mask, vector, 1);
+}
 
-	x2apic_wrmsr_fence();
+static void x2apic_send_IPI_allbutself(int vector)
+{
+	__x2apic_send_IPI_mask(cpu_online_mask, vector, 1);
+}
 
-	local_irq_save(flags);
-	for_each_online_cpu(query_cpu) {
-		if (query_cpu == this_cpu)
-			continue;
-		__x2apic_send_IPI_dest(
-				per_cpu(x86_cpu_to_logical_apicid, query_cpu),
-				vector, apic->dest_logical);
-	}
-	local_irq_restore(flags);
+static void x2apic_send_IPI_mask(const struct cpumask *mask, int vector)
+{
+	__x2apic_send_IPI_mask(mask, vector, 0);
 }
 
 static void x2apic_send_IPI_all(int vector)
 {
-	x2apic_send_IPI_mask(cpu_online_mask, vector);
+	__x2apic_send_IPI_mask(cpu_online_mask, vector, 0);
 }
 
 static int x2apic_apic_id_registered(void)
@@ -151,6 +164,7 @@ x2apic_cpu_mask_to_apicid_and(const stru
 	return per_cpu(x86_cpu_to_logical_apicid, cpu);
 }
 
+
 static unsigned int x2apic_cluster_phys_get_apic_id(unsigned long x)
 {
 	unsigned int id;
@@ -179,13 +193,76 @@ static void x2apic_send_IPI_self(int vec
 
 static void init_x2apic_ldr(void)
 {
+	unsigned int this_cpu = smp_processor_id();
+	unsigned int cpu;
+
+	per_cpu(x86_cpu_to_logical_apicid, this_cpu) = apic_read(APIC_LDR);
+
+	__cpu_set(this_cpu, per_cpu(cpus_in_cluster, this_cpu));
+	for_each_online_cpu(cpu) {
+		if (x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
+			continue;
+		__cpu_set(this_cpu, per_cpu(cpus_in_cluster, cpu));
+		__cpu_set(cpu, per_cpu(cpus_in_cluster, this_cpu));
+	}
+}
+
+/*
+ * At CPU state changes, update the x2apic cluster sibling info.
+ */
+static int __cpuinit
+update_clusterinfo(struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+	unsigned int this_cpu = (unsigned long)hcpu;
+	unsigned int cpu;
+	int err = 0;
+
+	switch (action) {
+	case CPU_UP_PREPARE:
+		if (!zalloc_cpumask_var(&per_cpu(cpus_in_cluster, this_cpu),
+					GFP_KERNEL)) {
+			err = -ENOMEM;
+		} else if (!zalloc_cpumask_var(&per_cpu(ipi_mask, this_cpu),
+					       GFP_KERNEL)) {
+			free_cpumask_var(per_cpu(cpus_in_cluster, this_cpu));
+			err = -ENOMEM;
+		}
+		break;
+	case CPU_UP_CANCELED:
+	case CPU_UP_CANCELED_FROZEN:
+	case CPU_DEAD:
+		for_each_online_cpu(cpu) {
+			if (x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
+				continue;
+			__cpu_clear(this_cpu, per_cpu(cpus_in_cluster, cpu));
+			__cpu_clear(cpu, per_cpu(cpus_in_cluster, this_cpu));
+		}
+		free_cpumask_var(per_cpu(cpus_in_cluster, this_cpu));
+		free_cpumask_var(per_cpu(ipi_mask, this_cpu));
+		break;
+	}
+
+	return notifier_from_errno(err);
+}
+
+static struct notifier_block __refdata x2apic_cpu_notifier = {
+	.notifier_call = update_clusterinfo,
+};
+
+void x2apic_init_cpu_notifier(void)
+{
 	int cpu = smp_processor_id();
 
-	per_cpu(x86_cpu_to_logical_apicid, cpu) = apic_read(APIC_LDR);
+	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));
+
+	__cpu_set(cpu, per_cpu(cpus_in_cluster, cpu));
+	register_hotcpu_notifier(&x2apic_cpu_notifier);
 }
 
 struct apic apic_x2apic_cluster = {
-
 	.name				= "cluster x2apic",
 	.probe				= NULL,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,

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

[Cyrill Gorcunov]

[-- Attachment #1: x86-x2apic-merge-phys-cluster-v4 --]
[-- Type: text/plain, Size: 7799 bytes --]

To eliminate code duplication.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---
 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(-)

Index: tip-linux-2.6/arch/x86/include/asm/x2apic.h
===================================================================
--- /dev/null
+++ 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 */
Index: tip-linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
===================================================================
--- tip-linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c
+++ tip-linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
@@ -8,8 +8,7 @@
 #include <linux/cpu.h>
 
 #include <asm/smp.h>
-#include <asm/apic.h>
-#include <asm/ipi.h>
+#include <asm/x2apic.h>
 
 static DEFINE_PER_CPU(u32, x86_cpu_to_logical_apicid);
 static DEFINE_PER_CPU(cpumask_var_t, cpus_in_cluster);
@@ -20,37 +19,6 @@ static int x2apic_acpi_madt_oem_check(ch
 	return x2apic_enabled();
 }
 
-/*
- * 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;
-}
-
-/*
- * 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;
-
-	cfg = __prepare_ICR(0, vector, dest);
-
-	/*
-	 * send the IPI.
-	 */
-	native_x2apic_icr_write(cfg, apicid);
-}
-
 static inline u32 x2apic_cluster(int cpu)
 {
 	return per_cpu(x86_cpu_to_logical_apicid, cpu) >> 16;
@@ -127,11 +95,6 @@ static void x2apic_send_IPI_all(int vect
 	__x2apic_send_IPI_mask(cpu_online_mask, vector, 0);
 }
 
-static int x2apic_apic_id_registered(void)
-{
-	return 1;
-}
-
 static unsigned int x2apic_cpu_mask_to_apicid(const struct cpumask *cpumask)
 {
 	/*
@@ -165,32 +128,6 @@ x2apic_cpu_mask_to_apicid_and(const stru
 }
 
 
-static unsigned int x2apic_cluster_phys_get_apic_id(unsigned long x)
-{
-	unsigned int id;
-
-	id = x;
-	return id;
-}
-
-static unsigned long set_apic_id(unsigned int id)
-{
-	unsigned long x;
-
-	x = id;
-	return x;
-}
-
-static int x2apic_cluster_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);
-}
-
 static void init_x2apic_ldr(void)
 {
 	unsigned int this_cpu = smp_processor_id();
@@ -288,11 +225,11 @@ struct apic apic_x2apic_cluster = {
 	.setup_portio_remap		= NULL,
 	.check_phys_apicid_present	= default_check_phys_apicid_present,
 	.enable_apic_mode		= NULL,
-	.phys_pkg_id			= x2apic_cluster_phys_pkg_id,
+	.phys_pkg_id			= x2apic_phys_pkg_id,
 	.mps_oem_check			= NULL,
 
-	.get_apic_id			= x2apic_cluster_phys_get_apic_id,
-	.set_apic_id			= set_apic_id,
+	.get_apic_id			= x2apic_get_apic_id,
+	.set_apic_id			= x2apic_set_apic_id,
 	.apic_id_mask			= 0xFFFFFFFFu,
 
 	.cpu_mask_to_apicid		= x2apic_cpu_mask_to_apicid,
Index: tip-linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- tip-linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ tip-linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -7,8 +7,7 @@
 #include <linux/dmar.h>
 
 #include <asm/smp.h>
-#include <asm/apic.h>
-#include <asm/ipi.h>
+#include <asm/x2apic.h>
 
 int x2apic_phys;
 
@@ -27,34 +26,6 @@ static int x2apic_acpi_madt_oem_check(ch
 		return 0;
 }
 
-/*
- * 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 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;
-
-	cfg = __prepare_ICR(0, vector, dest);
-
-	/*
-	 * send the IPI.
-	 */
-	native_x2apic_icr_write(cfg, apicid);
-}
-
 static void x2apic_send_IPI_mask(const struct cpumask *mask, int vector)
 {
 	unsigned long query_cpu;
@@ -71,7 +42,7 @@ static void x2apic_send_IPI_mask(const s
 }
 
 static void
- x2apic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
+x2apic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
 {
 	unsigned long this_cpu = smp_processor_id();
 	unsigned long query_cpu;
@@ -81,10 +52,10 @@ static void
 
 	local_irq_save(flags);
 	for_each_cpu(query_cpu, mask) {
-		if (query_cpu != this_cpu)
-			__x2apic_send_IPI_dest(
-				per_cpu(x86_cpu_to_apicid, query_cpu),
-				vector, APIC_DEST_PHYSICAL);
+		if (query_cpu == this_cpu)
+			continue;
+		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
+				       vector, APIC_DEST_PHYSICAL);
 	}
 	local_irq_restore(flags);
 }
@@ -112,11 +83,6 @@ static void x2apic_send_IPI_all(int vect
 	x2apic_send_IPI_mask(cpu_online_mask, vector);
 }
 
-static int x2apic_apic_id_registered(void)
-{
-	return 1;
-}
-
 static unsigned int x2apic_cpu_mask_to_apicid(const struct cpumask *cpumask)
 {
 	/*
@@ -149,32 +115,11 @@ x2apic_cpu_mask_to_apicid_and(const stru
 	return per_cpu(x86_cpu_to_apicid, cpu);
 }
 
-static unsigned int x2apic_phys_get_apic_id(unsigned long x)
-{
-	return x;
-}
-
-static unsigned long 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);
-}
-
 static void init_x2apic_ldr(void)
 {
 }
 
 struct apic apic_x2apic_phys = {
-
 	.name				= "physical x2apic",
 	.probe				= NULL,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
@@ -203,8 +148,8 @@ struct apic apic_x2apic_phys = {
 	.phys_pkg_id			= x2apic_phys_pkg_id,
 	.mps_oem_check			= NULL,
 
-	.get_apic_id			= x2apic_phys_get_apic_id,
-	.set_apic_id			= set_apic_id,
+	.get_apic_id			= x2apic_get_apic_id,
+	.set_apic_id			= x2apic_set_apic_id,
 	.apic_id_mask			= 0xFFFFFFFFu,
 
 	.cpu_mask_to_apicid		= x2apic_cpu_mask_to_apicid,

Re: [patch 0/2] x2apic optimization, round 4

[Avi Kivity]

On 04/30/2011 08:14 PM, Cyrill Gorcunov wrote:
>    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.

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

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

Re: [patch 0/2] x2apic optimization, round 4

[Cyrill Gorcunov]

On 05/01/2011 12:14 AM, Avi Kivity wrote:
> On 04/30/2011 08:14 PM, Cyrill Gorcunov wrote:
>>    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.
> 
> 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 ;)

-- 
Thanks,
  Cyrill

Re: [patch 0/2] x2apic optimization, round 4

[Ingo Molnar]

* Avi Kivity <avi@redhat.com> wrote:

> On 04/30/2011 08:14 PM, Cyrill Gorcunov wrote:
> >   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.
> 
> 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

Re: [patch 0/2] x2apic optimization, round 4

[Cyrill Gorcunov]

On Sun, May 1, 2011 at 8:46 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Avi Kivity <avi@redhat.com> wrote:
>
>> On 04/30/2011 08:14 PM, Cyrill Gorcunov wrote:
>> >   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.
>>
>> 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.

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

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>

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

Thanks,

	Ingo

Re: [patch 0/2] x2apic optimization, round 4

[Gleb Natapov]

On Sun, May 01, 2011 at 09:11:35PM +0400, Cyrill Gorcunov wrote:
> On Sun, May 1, 2011 at 8:46 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Avi Kivity <avi@redhat.com> wrote:
> >
> >> On 04/30/2011 08:14 PM, Cyrill Gorcunov wrote:
> >> >   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.
> >>
> >> 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.
Can you test in in KVM with x2apic emulation enabled?

--
			Gleb.

Re: [patch 0/2] x2apic optimization, round 4

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

On Monday, May 2, 2011, Gleb Natapov <gleb@redhat.com> wrote:
> On Sun, May 01, 2011 at 09:11:35PM +0400, Cyrill Gorcunov wrote:
>> On Sun, May 1, 2011 at 8:46 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Avi Kivity <avi@redhat.com> wrote:
>> >
>> >> On 04/30/2011 08:14 PM, Cyrill Gorcunov wrote:
>> >> >   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.
>> >>
>> >> 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.
> Can you test in in KVM with x2apic emulation enabled?
>
> --
>                         Gleb.
>

Re: [patch 0/2] x2apic optimization, round 4

[Cyrill Gorcunov]

On 05/02/2011 09:51 AM, Cyrill Gorcunov wrote:
> 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.

-- 
Thanks,
  Cyrill

Re: [patch 0/2] x2apic optimization, round 4

[Gleb Natapov]

On Mon, May 02, 2011 at 11:09:16AM +0400, Cyrill Gorcunov wrote:
> On 05/02/2011 09:51 AM, Cyrill Gorcunov wrote:
> > 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.
> 
Hmm, +x2apic is enough to advertise x2apic presents to the OS. What
logic Linux uses to decide which mode to use?

--
			Gleb.

Re: [patch 0/2] x2apic optimization, round 4

[Cyrill Gorcunov]

On 05/02/2011 11:45 AM, Gleb Natapov wrote:
> On Mon, May 02, 2011 at 11:09:16AM +0400, Cyrill Gorcunov wrote:
>> On 05/02/2011 09:51 AM, Cyrill Gorcunov wrote:
>>> 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.
>>
> 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).

-- 
Thanks,
  Cyrill

Re: [patch 0/2] x2apic optimization, round 4

[Gleb Natapov]

On Mon, May 02, 2011 at 11:52:26AM +0400, Cyrill Gorcunov wrote:
> On 05/02/2011 11:45 AM, Gleb Natapov wrote:
> > On Mon, May 02, 2011 at 11:09:16AM +0400, Cyrill Gorcunov wrote:
> >> On 05/02/2011 09:51 AM, Cyrill Gorcunov wrote:
> >>> 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.
> >>
> > 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).
> 
Ahh, definitely. I recall that now. 

--
			Gleb.

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

[Cyrill Gorcunov]

[-- Attachment #1: x86-x2apic-merge-phys-cluster-v4 --]
[-- Type: text/plain, Size: 7794 bytes --]

To eliminate code duplication.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
---
 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(-)

Index: tip-linux-2.6/arch/x86/include/asm/x2apic.h
===================================================================
--- /dev/null
+++ 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 */
Index: tip-linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
===================================================================
--- tip-linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c
+++ tip-linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c
@@ -8,8 +8,7 @@
 #include <linux/cpu.h>
 
 #include <asm/smp.h>
-#include <asm/apic.h>
-#include <asm/ipi.h>
+#include <asm/x2apic.h>
 
 static DEFINE_PER_CPU(u32, x86_cpu_to_logical_apicid);
 static DEFINE_PER_CPU(cpumask_var_t, cpus_in_cluster);
@@ -20,37 +19,6 @@ static int x2apic_acpi_madt_oem_check(ch
 	return x2apic_enabled();
 }
 
-/*
- * 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;
-}
-
-/*
- * 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;
-
-	cfg = __prepare_ICR(0, vector, dest);
-
-	/*
-	 * send the IPI.
-	 */
-	native_x2apic_icr_write(cfg, apicid);
-}
-
 static inline u32 x2apic_cluster(int cpu)
 {
 	return per_cpu(x86_cpu_to_logical_apicid, cpu) >> 16;
@@ -127,11 +95,6 @@ static void x2apic_send_IPI_all(int vect
 	__x2apic_send_IPI_mask(cpu_online_mask, vector, 0);
 }
 
-static int x2apic_apic_id_registered(void)
-{
-	return 1;
-}
-
 static unsigned int x2apic_cpu_mask_to_apicid(const struct cpumask *cpumask)
 {
 	/*
@@ -165,32 +128,6 @@ x2apic_cpu_mask_to_apicid_and(const stru
 }
 
 
-static unsigned int x2apic_cluster_phys_get_apic_id(unsigned long x)
-{
-	unsigned int id;
-
-	id = x;
-	return id;
-}
-
-static unsigned long set_apic_id(unsigned int id)
-{
-	unsigned long x;
-
-	x = id;
-	return x;
-}
-
-static int x2apic_cluster_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);
-}
-
 static void init_x2apic_ldr(void)
 {
 	unsigned int this_cpu = smp_processor_id();
@@ -288,11 +225,11 @@ struct apic apic_x2apic_cluster = {
 	.setup_portio_remap		= NULL,
 	.check_phys_apicid_present	= default_check_phys_apicid_present,
 	.enable_apic_mode		= NULL,
-	.phys_pkg_id			= x2apic_cluster_phys_pkg_id,
+	.phys_pkg_id			= x2apic_phys_pkg_id,
 	.mps_oem_check			= NULL,
 
-	.get_apic_id			= x2apic_cluster_phys_get_apic_id,
-	.set_apic_id			= set_apic_id,
+	.get_apic_id			= x2apic_get_apic_id,
+	.set_apic_id			= x2apic_set_apic_id,
 	.apic_id_mask			= 0xFFFFFFFFu,
 
 	.cpu_mask_to_apicid		= x2apic_cpu_mask_to_apicid,
Index: tip-linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
===================================================================
--- tip-linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c
+++ tip-linux-2.6/arch/x86/kernel/apic/x2apic_phys.c
@@ -7,8 +7,7 @@
 #include <linux/dmar.h>
 
 #include <asm/smp.h>
-#include <asm/apic.h>
-#include <asm/ipi.h>
+#include <asm/x2apic.h>
 
 int x2apic_phys;
 
@@ -27,34 +26,6 @@ static int x2apic_acpi_madt_oem_check(ch
 		return 0;
 }
 
-/*
- * 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 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;
-
-	cfg = __prepare_ICR(0, vector, dest);
-
-	/*
-	 * send the IPI.
-	 */
-	native_x2apic_icr_write(cfg, apicid);
-}
-
 static void x2apic_send_IPI_mask(const struct cpumask *mask, int vector)
 {
 	unsigned long query_cpu;
@@ -71,7 +42,7 @@ static void x2apic_send_IPI_mask(const s
 }
 
 static void
- x2apic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
+x2apic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
 {
 	unsigned long this_cpu = smp_processor_id();
 	unsigned long query_cpu;
@@ -81,10 +52,10 @@ static void
 
 	local_irq_save(flags);
 	for_each_cpu(query_cpu, mask) {
-		if (query_cpu != this_cpu)
-			__x2apic_send_IPI_dest(
-				per_cpu(x86_cpu_to_apicid, query_cpu),
-				vector, APIC_DEST_PHYSICAL);
+		if (query_cpu == this_cpu)
+			continue;
+		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
+				       vector, APIC_DEST_PHYSICAL);
 	}
 	local_irq_restore(flags);
 }
@@ -112,11 +83,6 @@ static void x2apic_send_IPI_all(int vect
 	x2apic_send_IPI_mask(cpu_online_mask, vector);
 }
 
-static int x2apic_apic_id_registered(void)
-{
-	return 1;
-}
-
 static unsigned int x2apic_cpu_mask_to_apicid(const struct cpumask *cpumask)
 {
 	/*
@@ -149,32 +115,11 @@ x2apic_cpu_mask_to_apicid_and(const stru
 	return per_cpu(x86_cpu_to_apicid, cpu);
 }
 
-static unsigned int x2apic_phys_get_apic_id(unsigned long x)
-{
-	return x;
-}
-
-static unsigned long 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);
-}
-
 static void init_x2apic_ldr(void)
 {
 }
 
 struct apic apic_x2apic_phys = {
-
 	.name				= "physical x2apic",
 	.probe				= NULL,
 	.acpi_madt_oem_check		= x2apic_acpi_madt_oem_check,
@@ -203,8 +148,8 @@ struct apic apic_x2apic_phys = {
 	.phys_pkg_id			= x2apic_phys_pkg_id,
 	.mps_oem_check			= NULL,
 
-	.get_apic_id			= x2apic_phys_get_apic_id,
-	.set_apic_id			= set_apic_id,
+	.get_apic_id			= x2apic_get_apic_id,
+	.set_apic_id			= x2apic_set_apic_id,
 	.apic_id_mask			= 0xFFFFFFFFu,
 
 	.cpu_mask_to_apicid		= x2apic_cpu_mask_to_apicid,