[PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[Konrad Rzeszutek Wilk]

After a bit of false starts, lots of debugging, and tons of help from Stefano and
David on how event mechanism is suppose to work I am happy to present a set
of bug-fixes that make PV ticketlocks work under Xen PVHVM with Linux v3.12.

v3.12 has now thanks to commit 816434ec4a674fcdb3c2221a6dffdc8f34020550
(Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)
PV ticketlocks. That means:
 - Xen PV bytelock has been replaced by Xen PV ticketlock.
 - Xen PVHVM is using ticketlock (not PV variant) - this patch makes it PV.
 - baremetal is still using ticketlocks.

In other words everything in the kernel is ticketlock based with the virtualizations
having the 'PV' part to aid.

Please take a look at the patches. If you are interested in testing them
out I will post a git tree on Monday based on v3.11 and v3.12-rc0. 

Note that I had not run any performance tests and I am not sure when I will
be able to (got other bugs to squash now). I've asked our internal performance
engineer to do some benchmarking - but those results won't be available for
some time as it takes time to do good benchmarking.

 arch/x86/xen/enlighten.c |    1 -
 arch/x86/xen/smp.c       |   17 +++++++++++++++++
 arch/x86/xen/spinlock.c  |   45 ++++++++-------------------------------------
 3 files changed, 25 insertions(+), 38 deletions(-)

(oh neat, more deletions!) 

Konrad Rzeszutek Wilk (5):
      xen/spinlock: Fix locking path engaging too soon under PVHVM.
      xen/spinlock: We don't need the old structure anymore
      xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM
      xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.
      Revert "xen/spinlock: Disable IRQ spinlock (PV) allocation on PVHVM"

[PATCH 1/5] xen/spinlock: Fix locking path engaging too soon under PVHVM.

[Konrad Rzeszutek Wilk]

The xen_lock_spinning has a check for the kicker interrupts
and if it is not initialised it will spin normally (not enter
the slowpath).

But for PVHVM case we would initialise the kicker interrupt
before the CPU came online. This meant that if the booting
CPU used a spinlock and went in the slowpath - it would
enter the slowpath and block forever. The forever part b/c
during bootup the interrupts are disabled - so the CPU would
never get an IPI kick and would stay stuck in the slowpath
logic forever.

Why would the booting CPU never get an IPI kick? B/c in both
PV and PVHVM we consult the cpu_online_mask to determine whether
the IPI should go to its CPU destination. Since the booting
CPU has not yet finished and set that flag, it meant that
if any spinlocks were taken before the booting CPU had gotten to:

  set_cpu_online(smp_processor_id(), true);

it (booting CPU) we would never get the unkicker IPI
(from xen_unlock_kick) and block forever.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/enlighten.c |    1 -
 arch/x86/xen/smp.c       |    9 +++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 193097e..fbc002c 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1689,7 +1689,6 @@ static int xen_hvm_cpu_notify(struct notifier_block *self, unsigned long action,
 	case CPU_UP_PREPARE:
 		xen_vcpu_setup(cpu);
 		if (xen_have_vector_callback) {
-			xen_init_lock_cpu(cpu);
 			if (xen_feature(XENFEAT_hvm_safe_pvclock))
 				xen_setup_timer(cpu);
 		}
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 597655b..4db779d 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -703,6 +703,15 @@ static int xen_hvm_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	WARN_ON(rc);
 	if (!rc)
 		rc =  native_cpu_up(cpu, tidle);
+
+	/*
+	 * We must initialize the slowpath CPU kicker _after_ the native
+	 * path has executed. If we initialized it before none of the
+	 * unlocker IPI kicks would reach the booting CPU as the booting
+	 * CPU had not set itself 'online' in cpu_online_mask. That mask
+	 * is checked when IPIs are sent (on HVM at least).
+	 */
+	xen_init_lock_cpu(cpu);
 	return rc;
 }
 
-- 
1.7.7.6

[PATCH 1/5] xen/spinlock: Fix locking path engaging too soon under PVHVM.

[Konrad Rzeszutek Wilk]

The xen_lock_spinning has a check for the kicker interrupts
and if it is not initialised it will spin normally (not enter
the slowpath).

But for PVHVM case we would initialise the kicker interrupt
before the CPU came online. This meant that if the booting
CPU used a spinlock and went in the slowpath - it would
enter the slowpath and block forever. The forever part b/c
during bootup the interrupts are disabled - so the CPU would
never get an IPI kick and would stay stuck in the slowpath
logic forever.

Why would the booting CPU never get an IPI kick? B/c in both
PV and PVHVM we consult the cpu_online_mask to determine whether
the IPI should go to its CPU destination. Since the booting
CPU has not yet finished and set that flag, it meant that
if any spinlocks were taken before the booting CPU had gotten to:

  set_cpu_online(smp_processor_id(), true);

it (booting CPU) we would never get the unkicker IPI
(from xen_unlock_kick) and block forever.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/enlighten.c |    1 -
 arch/x86/xen/smp.c       |    9 +++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 193097e..fbc002c 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1689,7 +1689,6 @@ static int xen_hvm_cpu_notify(struct notifier_block *self, unsigned long action,
 	case CPU_UP_PREPARE:
 		xen_vcpu_setup(cpu);
 		if (xen_have_vector_callback) {
-			xen_init_lock_cpu(cpu);
 			if (xen_feature(XENFEAT_hvm_safe_pvclock))
 				xen_setup_timer(cpu);
 		}
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 597655b..4db779d 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -703,6 +703,15 @@ static int xen_hvm_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	WARN_ON(rc);
 	if (!rc)
 		rc =  native_cpu_up(cpu, tidle);
+
+	/*
+	 * We must initialize the slowpath CPU kicker _after_ the native
+	 * path has executed. If we initialized it before none of the
+	 * unlocker IPI kicks would reach the booting CPU as the booting
+	 * CPU had not set itself 'online' in cpu_online_mask. That mask
+	 * is checked when IPIs are sent (on HVM at least).
+	 */
+	xen_init_lock_cpu(cpu);
 	return rc;
 }
 
-- 
1.7.7.6

[PATCH 2/5] xen/spinlock: We don't need the old structure anymore

[Konrad Rzeszutek Wilk]

As we are piggybacking on the generic ticketlock structs
and these old structures are not needed anymore.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/spinlock.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 0438b93..71db82c 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -81,7 +81,6 @@ static inline void spin_time_accum_blocked(u64 start)
 	spinlock_stats.time_blocked += delta;
 }
 #else  /* !CONFIG_XEN_DEBUG_FS */
-#define TIMEOUT			(1 << 10)
 static inline void add_stats(enum xen_contention_stat var, u32 val)
 {
 }
@@ -96,23 +95,6 @@ static inline void spin_time_accum_blocked(u64 start)
 }
 #endif  /* CONFIG_XEN_DEBUG_FS */
 
-/*
- * Size struct xen_spinlock so it's the same as arch_spinlock_t.
- */
-#if NR_CPUS < 256
-typedef u8 xen_spinners_t;
-# define inc_spinners(xl) \
-	asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners) : : "memory");
-# define dec_spinners(xl) \
-	asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners) : : "memory");
-#else
-typedef u16 xen_spinners_t;
-# define inc_spinners(xl) \
-	asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners) : : "memory");
-# define dec_spinners(xl) \
-	asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners) : : "memory");
-#endif
-
 struct xen_lock_waiting {
 	struct arch_spinlock *lock;
 	__ticket_t want;
-- 
1.7.7.6

[PATCH 2/5] xen/spinlock: We don't need the old structure anymore

[Konrad Rzeszutek Wilk]

As we are piggybacking on the generic ticketlock structs
and these old structures are not needed anymore.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/spinlock.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 0438b93..71db82c 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -81,7 +81,6 @@ static inline void spin_time_accum_blocked(u64 start)
 	spinlock_stats.time_blocked += delta;
 }
 #else  /* !CONFIG_XEN_DEBUG_FS */
-#define TIMEOUT			(1 << 10)
 static inline void add_stats(enum xen_contention_stat var, u32 val)
 {
 }
@@ -96,23 +95,6 @@ static inline void spin_time_accum_blocked(u64 start)
 }
 #endif  /* CONFIG_XEN_DEBUG_FS */
 
-/*
- * Size struct xen_spinlock so it's the same as arch_spinlock_t.
- */
-#if NR_CPUS < 256
-typedef u8 xen_spinners_t;
-# define inc_spinners(xl) \
-	asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners) : : "memory");
-# define dec_spinners(xl) \
-	asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners) : : "memory");
-#else
-typedef u16 xen_spinners_t;
-# define inc_spinners(xl) \
-	asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners) : : "memory");
-# define dec_spinners(xl) \
-	asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners) : : "memory");
-#endif
-
 struct xen_lock_waiting {
 	struct arch_spinlock *lock;
 	__ticket_t want;
-- 
1.7.7.6

[PATCH 3/5] xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM

[Konrad Rzeszutek Wilk]

Before this patch we would patch all of the pv_lock_ops sites
using alternative assembler. Then later in the bootup cycle
change the unlock_kick and lock_spinning to the Xen specific -
without re patching.

That meant that for the core of the kernel we would be running
with the baremetal version of unlock_kick and lock_spinning while
for modules we would have the proper Xen specific slowpaths.

As most of the module uses some API from the core kernel that ended
up with slowpath lockers waiting forever to be kicked (b/c they
would be using the Xen specific slowpath logic). And the
kick never came b/c the unlock path that was taken was the
baremetal one.

On PV we do not have the problem as we initialise before the
alternative code kicks in.

The fix is to move the updating of the pv_lock_ops function
before the alternative code starts patching.

Note that this patch fixes issues discovered by commit
f10cd522c5fbfec9ae3cc01967868c9c2401ed23.
("xen: disable PV spinlocks on HVM") wherein it mentioned

   PV spinlocks cannot possibly work with the current code because they are
   enabled after pvops patching has already been done, and because PV
   spinlocks use a different data structure than native spinlocks so we
   cannot switch between them dynamically.

The first problem is solved by this patch.

The second problem has been solved by commit
816434ec4a674fcdb3c2221a6dffdc8f34020550
(Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)

P.S.
There is still the commit 70dd4998cb85f0ecd6ac892cc7232abefa432efb
(xen/spinlock: Disable IRQ spinlock (PV) allocation on PVHVM) to
revert but that can be done later after all other bugs have been
fixed.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/smp.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 4db779d..3404ee8 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -731,4 +731,12 @@ void __init xen_hvm_smp_init(void)
 	smp_ops.cpu_die = xen_hvm_cpu_die;
 	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
 	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
+
+	/*
+	 * The alternative logic (which patches the unlock/lock) runs before
+	 * the smp bootup up code is activated. That meant we would never patch
+	 * the core of the kernel with proper paravirt interfaces but would patch
+	 * modules.
+	 */
+	xen_init_spinlocks();
 }
-- 
1.7.7.6

[PATCH 3/5] xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM

[Konrad Rzeszutek Wilk]

Before this patch we would patch all of the pv_lock_ops sites
using alternative assembler. Then later in the bootup cycle
change the unlock_kick and lock_spinning to the Xen specific -
without re patching.

That meant that for the core of the kernel we would be running
with the baremetal version of unlock_kick and lock_spinning while
for modules we would have the proper Xen specific slowpaths.

As most of the module uses some API from the core kernel that ended
up with slowpath lockers waiting forever to be kicked (b/c they
would be using the Xen specific slowpath logic). And the
kick never came b/c the unlock path that was taken was the
baremetal one.

On PV we do not have the problem as we initialise before the
alternative code kicks in.

The fix is to move the updating of the pv_lock_ops function
before the alternative code starts patching.

Note that this patch fixes issues discovered by commit
f10cd522c5fbfec9ae3cc01967868c9c2401ed23.
("xen: disable PV spinlocks on HVM") wherein it mentioned

   PV spinlocks cannot possibly work with the current code because they are
   enabled after pvops patching has already been done, and because PV
   spinlocks use a different data structure than native spinlocks so we
   cannot switch between them dynamically.

The first problem is solved by this patch.

The second problem has been solved by commit
816434ec4a674fcdb3c2221a6dffdc8f34020550
(Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)

P.S.
There is still the commit 70dd4998cb85f0ecd6ac892cc7232abefa432efb
(xen/spinlock: Disable IRQ spinlock (PV) allocation on PVHVM) to
revert but that can be done later after all other bugs have been
fixed.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/smp.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 4db779d..3404ee8 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -731,4 +731,12 @@ void __init xen_hvm_smp_init(void)
 	smp_ops.cpu_die = xen_hvm_cpu_die;
 	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
 	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
+
+	/*
+	 * The alternative logic (which patches the unlock/lock) runs before
+	 * the smp bootup up code is activated. That meant we would never patch
+	 * the core of the kernel with proper paravirt interfaces but would patch
+	 * modules.
+	 */
+	xen_init_spinlocks();
 }
-- 
1.7.7.6

[PATCH 4/5] xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.

[Konrad Rzeszutek Wilk]

There is no need to setup this IPI kicker if we are never going
to use the paravirtualized ticketlock mechanism.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/spinlock.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 71db82c..e1bff87 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -105,6 +105,7 @@ static DEFINE_PER_CPU(char *, irq_name);
 static DEFINE_PER_CPU(struct xen_lock_waiting, lock_waiting);
 static cpumask_t waiting_cpus;
 
+static bool xen_pvspin __initdata = true;
 static void xen_lock_spinning(struct arch_spinlock *lock, __ticket_t want)
 {
 	int irq = __this_cpu_read(lock_kicker_irq);
@@ -223,6 +224,9 @@ void xen_init_lock_cpu(int cpu)
 	int irq;
 	char *name;
 
+	if (!xen_pvspin)
+		return;
+
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
 	     cpu, per_cpu(lock_kicker_irq, cpu));
 
@@ -259,13 +263,15 @@ void xen_uninit_lock_cpu(int cpu)
 	if (xen_hvm_domain())
 		return;
 
+	if (!xen_pvspin)
+		return;
+
 	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
 	per_cpu(lock_kicker_irq, cpu) = -1;
 	kfree(per_cpu(irq_name, cpu));
 	per_cpu(irq_name, cpu) = NULL;
 }
 
-static bool xen_pvspin __initdata = true;
 
 void __init xen_init_spinlocks(void)
 {
@@ -305,6 +311,9 @@ static int __init xen_spinlock_debugfs(void)
 	if (d_xen == NULL)
 		return -ENOMEM;
 
+	if (!xen_pvspin)
+		return 0;
+
 	d_spin_debug = debugfs_create_dir("spinlocks", d_xen);
 
 	debugfs_create_u8("zero_stats", 0644, d_spin_debug, &zero_stats);
-- 
1.7.7.6

[PATCH 4/5] xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.

[Konrad Rzeszutek Wilk]

There is no need to setup this IPI kicker if we are never going
to use the paravirtualized ticketlock mechanism.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/spinlock.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 71db82c..e1bff87 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -105,6 +105,7 @@ static DEFINE_PER_CPU(char *, irq_name);
 static DEFINE_PER_CPU(struct xen_lock_waiting, lock_waiting);
 static cpumask_t waiting_cpus;
 
+static bool xen_pvspin __initdata = true;
 static void xen_lock_spinning(struct arch_spinlock *lock, __ticket_t want)
 {
 	int irq = __this_cpu_read(lock_kicker_irq);
@@ -223,6 +224,9 @@ void xen_init_lock_cpu(int cpu)
 	int irq;
 	char *name;
 
+	if (!xen_pvspin)
+		return;
+
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
 	     cpu, per_cpu(lock_kicker_irq, cpu));
 
@@ -259,13 +263,15 @@ void xen_uninit_lock_cpu(int cpu)
 	if (xen_hvm_domain())
 		return;
 
+	if (!xen_pvspin)
+		return;
+
 	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
 	per_cpu(lock_kicker_irq, cpu) = -1;
 	kfree(per_cpu(irq_name, cpu));
 	per_cpu(irq_name, cpu) = NULL;
 }
 
-static bool xen_pvspin __initdata = true;
 
 void __init xen_init_spinlocks(void)
 {
@@ -305,6 +311,9 @@ static int __init xen_spinlock_debugfs(void)
 	if (d_xen == NULL)
 		return -ENOMEM;
 
+	if (!xen_pvspin)
+		return 0;
+
 	d_spin_debug = debugfs_create_dir("spinlocks", d_xen);
 
 	debugfs_create_u8("zero_stats", 0644, d_spin_debug, &zero_stats);
-- 
1.7.7.6

[PATCH 5/5] Revert "xen/spinlock: Disable IRQ spinlock (PV) allocation on PVHVM"

[Konrad Rzeszutek Wilk]

This reverts commit 70dd4998cb85f0ecd6ac892cc7232abefa432efb.

Now that the bugs have been resolved we can re-enable the
PV ticketlock implementation under PVHVM Xen guests.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/spinlock.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index e1bff87..b3c1ee8 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -230,13 +230,6 @@ void xen_init_lock_cpu(int cpu)
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
 	     cpu, per_cpu(lock_kicker_irq, cpu));
 
-	/*
-	 * See git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
-	 * (xen: disable PV spinlocks on HVM)
-	 */
-	if (xen_hvm_domain())
-		return;
-
 	name = kasprintf(GFP_KERNEL, "spinlock%d", cpu);
 	irq = bind_ipi_to_irqhandler(XEN_SPIN_UNLOCK_VECTOR,
 				     cpu,
@@ -256,13 +249,6 @@ void xen_init_lock_cpu(int cpu)
 
 void xen_uninit_lock_cpu(int cpu)
 {
-	/*
-	 * See git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
-	 * (xen: disable PV spinlocks on HVM)
-	 */
-	if (xen_hvm_domain())
-		return;
-
 	if (!xen_pvspin)
 		return;
 
@@ -275,12 +261,6 @@ void xen_uninit_lock_cpu(int cpu)
 
 void __init xen_init_spinlocks(void)
 {
-	/*
-	 * See git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
-	 * (xen: disable PV spinlocks on HVM)
-	 */
-	if (xen_hvm_domain())
-		return;
 
 	if (!xen_pvspin) {
 		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
-- 
1.7.7.6

[PATCH 5/5] Revert "xen/spinlock: Disable IRQ spinlock (PV) allocation on PVHVM"

[Konrad Rzeszutek Wilk]

This reverts commit 70dd4998cb85f0ecd6ac892cc7232abefa432efb.

Now that the bugs have been resolved we can re-enable the
PV ticketlock implementation under PVHVM Xen guests.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/spinlock.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index e1bff87..b3c1ee8 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -230,13 +230,6 @@ void xen_init_lock_cpu(int cpu)
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
 	     cpu, per_cpu(lock_kicker_irq, cpu));
 
-	/*
-	 * See git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
-	 * (xen: disable PV spinlocks on HVM)
-	 */
-	if (xen_hvm_domain())
-		return;
-
 	name = kasprintf(GFP_KERNEL, "spinlock%d", cpu);
 	irq = bind_ipi_to_irqhandler(XEN_SPIN_UNLOCK_VECTOR,
 				     cpu,
@@ -256,13 +249,6 @@ void xen_init_lock_cpu(int cpu)
 
 void xen_uninit_lock_cpu(int cpu)
 {
-	/*
-	 * See git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
-	 * (xen: disable PV spinlocks on HVM)
-	 */
-	if (xen_hvm_domain())
-		return;
-
 	if (!xen_pvspin)
 		return;
 
@@ -275,12 +261,6 @@ void xen_uninit_lock_cpu(int cpu)
 
 void __init xen_init_spinlocks(void)
 {
-	/*
-	 * See git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
-	 * (xen: disable PV spinlocks on HVM)
-	 */
-	if (xen_hvm_domain())
-		return;
 
 	if (!xen_pvspin) {
 		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
-- 
1.7.7.6

Re: [PATCH 2/5] xen/spinlock: We don't need the old structure anymore

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> As we are piggybacking on the generic ticketlock structs
> and these old structures are not needed anymore.

"As we are using the generic ticket lock..."

"piggybacking" suggests the Xen code isn't using a standard interface
for this.

Otherwise,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 2/5] xen/spinlock: We don't need the old structure anymore

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> As we are piggybacking on the generic ticketlock structs
> and these old structures are not needed anymore.

"As we are using the generic ticket lock..."

"piggybacking" suggests the Xen code isn't using a standard interface
for this.

Otherwise,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 1/5] xen/spinlock: Fix locking path engaging too soon under PVHVM.

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> The xen_lock_spinning has a check for the kicker interrupts
> and if it is not initialised it will spin normally (not enter
> the slowpath).
> 
> But for PVHVM case we would initialise the kicker interrupt
> before the CPU came online. This meant that if the booting
> CPU used a spinlock and went in the slowpath - it would
> enter the slowpath and block forever. The forever part b/c

b/c? Ewww.  Proper English please.

> during bootup the interrupts are disabled - so the CPU would
> never get an IPI kick and would stay stuck in the slowpath
> logic forever.

This description isn't right -- VCPUs blocked in SCHEDOP_poll can be
unblocked on the event they're waiting for even if local irq delivery is
disabled.

> Why would the booting CPU never get an IPI kick? B/c in both
> PV and PVHVM we consult the cpu_online_mask to determine whether
> the IPI should go to its CPU destination. Since the booting
> CPU has not yet finished and set that flag, it meant that
> if any spinlocks were taken before the booting CPU had gotten to:

I can't find where the online mask is being checked in
xen_send_IPI_one().  Is this really the reason why it didn't work?

This fix looks fine but both the description and the comment need to be
fixed/clarified.

David

Re: [PATCH 1/5] xen/spinlock: Fix locking path engaging too soon under PVHVM.

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> The xen_lock_spinning has a check for the kicker interrupts
> and if it is not initialised it will spin normally (not enter
> the slowpath).
> 
> But for PVHVM case we would initialise the kicker interrupt
> before the CPU came online. This meant that if the booting
> CPU used a spinlock and went in the slowpath - it would
> enter the slowpath and block forever. The forever part b/c

b/c? Ewww.  Proper English please.

> during bootup the interrupts are disabled - so the CPU would
> never get an IPI kick and would stay stuck in the slowpath
> logic forever.

This description isn't right -- VCPUs blocked in SCHEDOP_poll can be
unblocked on the event they're waiting for even if local irq delivery is
disabled.

> Why would the booting CPU never get an IPI kick? B/c in both
> PV and PVHVM we consult the cpu_online_mask to determine whether
> the IPI should go to its CPU destination. Since the booting
> CPU has not yet finished and set that flag, it meant that
> if any spinlocks were taken before the booting CPU had gotten to:

I can't find where the online mask is being checked in
xen_send_IPI_one().  Is this really the reason why it didn't work?

This fix looks fine but both the description and the comment need to be
fixed/clarified.

David

Re: [PATCH 3/5] xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> Before this patch we would patch all of the pv_lock_ops sites
> using alternative assembler. Then later in the bootup cycle
> change the unlock_kick and lock_spinning to the Xen specific -
> without re patching.
> 
> That meant that for the core of the kernel we would be running
> with the baremetal version of unlock_kick and lock_spinning while
> for modules we would have the proper Xen specific slowpaths.
> 
> As most of the module uses some API from the core kernel that ended
> up with slowpath lockers waiting forever to be kicked (b/c they
> would be using the Xen specific slowpath logic). And the
> kick never came b/c the unlock path that was taken was the
> baremetal one.
> 
> On PV we do not have the problem as we initialise before the
> alternative code kicks in.
> 
> The fix is to move the updating of the pv_lock_ops function
> before the alternative code starts patching.

This comment seems odd.  The xen_spinlock_init() call is added not moved.

> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -731,4 +731,12 @@ void __init xen_hvm_smp_init(void)
>  	smp_ops.cpu_die = xen_hvm_cpu_die;
>  	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
>  	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
> +
> +	/*
> +	 * The alternative logic (which patches the unlock/lock) runs before
> +	 * the smp bootup up code is activated. That meant we would never patch
> +	 * the core of the kernel with proper paravirt interfaces but would patch
> +	 * modules.
> +	 */
> +	xen_init_spinlocks();

PV does this in xen_smp_prepare_boot_cpu.  It would be better if the
PVHVM case followed this same pattern and provide a smp_prepare_boot_cpu
implementation to do this?

David

Re: [PATCH 3/5] xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> Before this patch we would patch all of the pv_lock_ops sites
> using alternative assembler. Then later in the bootup cycle
> change the unlock_kick and lock_spinning to the Xen specific -
> without re patching.
> 
> That meant that for the core of the kernel we would be running
> with the baremetal version of unlock_kick and lock_spinning while
> for modules we would have the proper Xen specific slowpaths.
> 
> As most of the module uses some API from the core kernel that ended
> up with slowpath lockers waiting forever to be kicked (b/c they
> would be using the Xen specific slowpath logic). And the
> kick never came b/c the unlock path that was taken was the
> baremetal one.
> 
> On PV we do not have the problem as we initialise before the
> alternative code kicks in.
> 
> The fix is to move the updating of the pv_lock_ops function
> before the alternative code starts patching.

This comment seems odd.  The xen_spinlock_init() call is added not moved.

> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -731,4 +731,12 @@ void __init xen_hvm_smp_init(void)
>  	smp_ops.cpu_die = xen_hvm_cpu_die;
>  	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
>  	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
> +
> +	/*
> +	 * The alternative logic (which patches the unlock/lock) runs before
> +	 * the smp bootup up code is activated. That meant we would never patch
> +	 * the core of the kernel with proper paravirt interfaces but would patch
> +	 * modules.
> +	 */
> +	xen_init_spinlocks();

PV does this in xen_smp_prepare_boot_cpu.  It would be better if the
PVHVM case followed this same pattern and provide a smp_prepare_boot_cpu
implementation to do this?

David

Re: [PATCH 4/5] xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> There is no need to setup this IPI kicker if we are never going
> to use the paravirtualized ticketlock mechanism.

"kicker IPI"

Otherwise,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 4/5] xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> There is no need to setup this IPI kicker if we are never going
> to use the paravirtualized ticketlock mechanism.

"kicker IPI"

Otherwise,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> After a bit of false starts, lots of debugging, and tons of help from Stefano and
> David on how event mechanism is suppose to work I am happy to present a set
> of bug-fixes that make PV ticketlocks work under Xen PVHVM with Linux v3.12.
> 
> v3.12 has now thanks to commit 816434ec4a674fcdb3c2221a6dffdc8f34020550
> (Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)
> PV ticketlocks. That means:
>  - Xen PV bytelock has been replaced by Xen PV ticketlock.
>  - Xen PVHVM is using ticketlock (not PV variant) - this patch makes it PV.
>  - baremetal is still using ticketlocks.
> 
> In other words everything in the kernel is ticketlock based with the virtualizations
> having the 'PV' part to aid.
> 
> Please take a look at the patches. If you are interested in testing them
> out I will post a git tree on Monday based on v3.11 and v3.12-rc0. 
> 
> Note that I had not run any performance tests and I am not sure when I will
> be able to (got other bugs to squash now). I've asked our internal performance
> engineer to do some benchmarking - but those results won't be available for
> some time as it takes time to do good benchmarking.

If there was favourable performance data I would be happy for these to
go in 3.12, without I think they'll have to wait for 3.13.

David

Re: [PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[David Vrabel]

On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> After a bit of false starts, lots of debugging, and tons of help from Stefano and
> David on how event mechanism is suppose to work I am happy to present a set
> of bug-fixes that make PV ticketlocks work under Xen PVHVM with Linux v3.12.
> 
> v3.12 has now thanks to commit 816434ec4a674fcdb3c2221a6dffdc8f34020550
> (Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)
> PV ticketlocks. That means:
>  - Xen PV bytelock has been replaced by Xen PV ticketlock.
>  - Xen PVHVM is using ticketlock (not PV variant) - this patch makes it PV.
>  - baremetal is still using ticketlocks.
> 
> In other words everything in the kernel is ticketlock based with the virtualizations
> having the 'PV' part to aid.
> 
> Please take a look at the patches. If you are interested in testing them
> out I will post a git tree on Monday based on v3.11 and v3.12-rc0. 
> 
> Note that I had not run any performance tests and I am not sure when I will
> be able to (got other bugs to squash now). I've asked our internal performance
> engineer to do some benchmarking - but those results won't be available for
> some time as it takes time to do good benchmarking.

If there was favourable performance data I would be happy for these to
go in 3.12, without I think they'll have to wait for 3.13.

David

Re: [PATCH 2/5] xen/spinlock: We don't need the old structure anymore

[Ramkumar Ramachandra]

Konrad Rzeszutek Wilk wrote:
> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
> index 0438b93..71db82c 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -81,7 +81,6 @@ static inline void spin_time_accum_blocked(u64 start)
>         spinlock_stats.time_blocked += delta;
>  }
>  #else  /* !CONFIG_XEN_DEBUG_FS */
> -#define TIMEOUT                        (1 << 10)

The timeout can be reduced, I think.

>  static inline void add_stats(enum xen_contention_stat var, u32 val)
>  {
>  }
> @@ -96,23 +95,6 @@ static inline void spin_time_accum_blocked(u64 start)
>  }
>  #endif  /* CONFIG_XEN_DEBUG_FS */
>
> -/*
> - * Size struct xen_spinlock so it's the same as arch_spinlock_t.
> - */
> -#if NR_CPUS < 256
> -typedef u8 xen_spinners_t;
> -# define inc_spinners(xl) \
> -       asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners) : : "memory");
> -# define dec_spinners(xl) \
> -       asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners) : : "memory");
> -#else
> -typedef u16 xen_spinners_t;
> -# define inc_spinners(xl) \
> -       asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners) : : "memory");
> -# define dec_spinners(xl) \
> -       asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners) : : "memory");
> -#endif
> -

Spinlocks on the filesystem help ensure consistency; otherwise, there
is a chance of a lot of noise coming through. Although NR_CPUS > 256,
very few CPUS are doing consistency checks.

Re: [PATCH 2/5] xen/spinlock: We don't need the old structure anymore

[Ramkumar Ramachandra]

Konrad Rzeszutek Wilk wrote:
> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
> index 0438b93..71db82c 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -81,7 +81,6 @@ static inline void spin_time_accum_blocked(u64 start)
>         spinlock_stats.time_blocked += delta;
>  }
>  #else  /* !CONFIG_XEN_DEBUG_FS */
> -#define TIMEOUT                        (1 << 10)

The timeout can be reduced, I think.

>  static inline void add_stats(enum xen_contention_stat var, u32 val)
>  {
>  }
> @@ -96,23 +95,6 @@ static inline void spin_time_accum_blocked(u64 start)
>  }
>  #endif  /* CONFIG_XEN_DEBUG_FS */
>
> -/*
> - * Size struct xen_spinlock so it's the same as arch_spinlock_t.
> - */
> -#if NR_CPUS < 256
> -typedef u8 xen_spinners_t;
> -# define inc_spinners(xl) \
> -       asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners) : : "memory");
> -# define dec_spinners(xl) \
> -       asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners) : : "memory");
> -#else
> -typedef u16 xen_spinners_t;
> -# define inc_spinners(xl) \
> -       asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners) : : "memory");
> -# define dec_spinners(xl) \
> -       asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners) : : "memory");
> -#endif
> -

Spinlocks on the filesystem help ensure consistency; otherwise, there
is a chance of a lot of noise coming through. Although NR_CPUS > 256,
very few CPUS are doing consistency checks.

Re: [PATCH 4/5] xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.

[Ramkumar Ramachandra]

Konrad Rzeszutek Wilk wrote:
> There is no need to setup this IPI kicker if we are never going
> to use the paravirtualized ticketlock mechanism.

Excellent patch; the important takeaway is that paravirtualization is
still quite useful in the real world.

Re: [PATCH 4/5] xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.

[Ramkumar Ramachandra]

Konrad Rzeszutek Wilk wrote:
> There is no need to setup this IPI kicker if we are never going
> to use the paravirtualized ticketlock mechanism.

Excellent patch; the important takeaway is that paravirtualization is
still quite useful in the real world.

Re: [PATCH 1/5] xen/spinlock: Fix locking path engaging too soon under PVHVM.

[Konrad Rzeszutek Wilk]

On Mon, Sep 09, 2013 at 11:31:23AM +0100, David Vrabel wrote:
> On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> > The xen_lock_spinning has a check for the kicker interrupts
> > and if it is not initialised it will spin normally (not enter
> > the slowpath).
> > 
> > But for PVHVM case we would initialise the kicker interrupt
> > before the CPU came online. This meant that if the booting
> > CPU used a spinlock and went in the slowpath - it would
> > enter the slowpath and block forever. The forever part b/c
> 
> b/c? Ewww.  Proper English please.
> 
> > during bootup the interrupts are disabled - so the CPU would
> > never get an IPI kick and would stay stuck in the slowpath
> > logic forever.
> 
> This description isn't right -- VCPUs blocked in SCHEDOP_poll can be
> unblocked on the event they're waiting for even if local irq delivery is
> disabled.
> 
> > Why would the booting CPU never get an IPI kick? B/c in both
> > PV and PVHVM we consult the cpu_online_mask to determine whether
> > the IPI should go to its CPU destination. Since the booting
> > CPU has not yet finished and set that flag, it meant that
> > if any spinlocks were taken before the booting CPU had gotten to:
> 
> I can't find where the online mask is being checked in
> xen_send_IPI_one().  Is this really the reason why it didn't work?

More details in fc78d343fa74514f6fd117b5ef4cd27e4ac30236
Author: Chuck Anderson <chuck.anderson@oracle.com>
Date:   Tue Aug 6 15:12:19 2013 -0700

    xen/smp: initialize IPI vectors before marking CPU online

I will add that part in.
> 
> This fix looks fine but both the description and the comment need to be
> fixed/clarified.

U r Right!

> 
> David

Re: [PATCH 1/5] xen/spinlock: Fix locking path engaging too soon under PVHVM.

[Konrad Rzeszutek Wilk]

On Mon, Sep 09, 2013 at 11:31:23AM +0100, David Vrabel wrote:
> On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> > The xen_lock_spinning has a check for the kicker interrupts
> > and if it is not initialised it will spin normally (not enter
> > the slowpath).
> > 
> > But for PVHVM case we would initialise the kicker interrupt
> > before the CPU came online. This meant that if the booting
> > CPU used a spinlock and went in the slowpath - it would
> > enter the slowpath and block forever. The forever part b/c
> 
> b/c? Ewww.  Proper English please.
> 
> > during bootup the interrupts are disabled - so the CPU would
> > never get an IPI kick and would stay stuck in the slowpath
> > logic forever.
> 
> This description isn't right -- VCPUs blocked in SCHEDOP_poll can be
> unblocked on the event they're waiting for even if local irq delivery is
> disabled.
> 
> > Why would the booting CPU never get an IPI kick? B/c in both
> > PV and PVHVM we consult the cpu_online_mask to determine whether
> > the IPI should go to its CPU destination. Since the booting
> > CPU has not yet finished and set that flag, it meant that
> > if any spinlocks were taken before the booting CPU had gotten to:
> 
> I can't find where the online mask is being checked in
> xen_send_IPI_one().  Is this really the reason why it didn't work?

More details in fc78d343fa74514f6fd117b5ef4cd27e4ac30236
Author: Chuck Anderson <chuck.anderson@oracle.com>
Date:   Tue Aug 6 15:12:19 2013 -0700

    xen/smp: initialize IPI vectors before marking CPU online

I will add that part in.
> 
> This fix looks fine but both the description and the comment need to be
> fixed/clarified.

U r Right!

> 
> David

Re: [PATCH 3/5] xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM

[Konrad Rzeszutek Wilk]

On Mon, Sep 09, 2013 at 11:31:48AM +0100, David Vrabel wrote:
> On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> > Before this patch we would patch all of the pv_lock_ops sites
> > using alternative assembler. Then later in the bootup cycle
> > change the unlock_kick and lock_spinning to the Xen specific -
> > without re patching.
> > 
> > That meant that for the core of the kernel we would be running
> > with the baremetal version of unlock_kick and lock_spinning while
> > for modules we would have the proper Xen specific slowpaths.
> > 
> > As most of the module uses some API from the core kernel that ended
> > up with slowpath lockers waiting forever to be kicked (b/c they
> > would be using the Xen specific slowpath logic). And the
> > kick never came b/c the unlock path that was taken was the
> > baremetal one.
> > 
> > On PV we do not have the problem as we initialise before the
> > alternative code kicks in.
> > 
> > The fix is to move the updating of the pv_lock_ops function
> > before the alternative code starts patching.
> 
> This comment seems odd.  The xen_spinlock_init() call is added not moved.

Ah, yes. The joy of rebasing and having the patches out of sync.
It was originally removed by git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
(xen: disable PV spinlocks on HVM) which as part of the patch
series I had reverted. Then I dropped the revert :-)

> 
> > --- a/arch/x86/xen/smp.c
> > +++ b/arch/x86/xen/smp.c
> > @@ -731,4 +731,12 @@ void __init xen_hvm_smp_init(void)
> >  	smp_ops.cpu_die = xen_hvm_cpu_die;
> >  	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
> >  	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
> > +
> > +	/*
> > +	 * The alternative logic (which patches the unlock/lock) runs before
> > +	 * the smp bootup up code is activated. That meant we would never patch
> > +	 * the core of the kernel with proper paravirt interfaces but would patch
> > +	 * modules.
> > +	 */
> > +	xen_init_spinlocks();
> 
> PV does this in xen_smp_prepare_boot_cpu.  It would be better if the
> PVHVM case followed this same pattern and provide a smp_prepare_boot_cpu
> implementation to do this?

Good eye. I can certainly try it out that way and see how it behaves. It would
make it more consistent.

> 
> David

Re: [PATCH 3/5] xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM

[Konrad Rzeszutek Wilk]

On Mon, Sep 09, 2013 at 11:31:48AM +0100, David Vrabel wrote:
> On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> > Before this patch we would patch all of the pv_lock_ops sites
> > using alternative assembler. Then later in the bootup cycle
> > change the unlock_kick and lock_spinning to the Xen specific -
> > without re patching.
> > 
> > That meant that for the core of the kernel we would be running
> > with the baremetal version of unlock_kick and lock_spinning while
> > for modules we would have the proper Xen specific slowpaths.
> > 
> > As most of the module uses some API from the core kernel that ended
> > up with slowpath lockers waiting forever to be kicked (b/c they
> > would be using the Xen specific slowpath logic). And the
> > kick never came b/c the unlock path that was taken was the
> > baremetal one.
> > 
> > On PV we do not have the problem as we initialise before the
> > alternative code kicks in.
> > 
> > The fix is to move the updating of the pv_lock_ops function
> > before the alternative code starts patching.
> 
> This comment seems odd.  The xen_spinlock_init() call is added not moved.

Ah, yes. The joy of rebasing and having the patches out of sync.
It was originally removed by git commit f10cd522c5fbfec9ae3cc01967868c9c2401ed23
(xen: disable PV spinlocks on HVM) which as part of the patch
series I had reverted. Then I dropped the revert :-)

> 
> > --- a/arch/x86/xen/smp.c
> > +++ b/arch/x86/xen/smp.c
> > @@ -731,4 +731,12 @@ void __init xen_hvm_smp_init(void)
> >  	smp_ops.cpu_die = xen_hvm_cpu_die;
> >  	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
> >  	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
> > +
> > +	/*
> > +	 * The alternative logic (which patches the unlock/lock) runs before
> > +	 * the smp bootup up code is activated. That meant we would never patch
> > +	 * the core of the kernel with proper paravirt interfaces but would patch
> > +	 * modules.
> > +	 */
> > +	xen_init_spinlocks();
> 
> PV does this in xen_smp_prepare_boot_cpu.  It would be better if the
> PVHVM case followed this same pattern and provide a smp_prepare_boot_cpu
> implementation to do this?

Good eye. I can certainly try it out that way and see how it behaves. It would
make it more consistent.

> 
> David

Re: [Xen-devel] [PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[Konrad Rzeszutek Wilk]

On Mon, Sep 09, 2013 at 11:34:42AM +0100, David Vrabel wrote:
> On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> > After a bit of false starts, lots of debugging, and tons of help from Stefano and
> > David on how event mechanism is suppose to work I am happy to present a set
> > of bug-fixes that make PV ticketlocks work under Xen PVHVM with Linux v3.12.
> > 
> > v3.12 has now thanks to commit 816434ec4a674fcdb3c2221a6dffdc8f34020550
> > (Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)
> > PV ticketlocks. That means:
> >  - Xen PV bytelock has been replaced by Xen PV ticketlock.
> >  - Xen PVHVM is using ticketlock (not PV variant) - this patch makes it PV.
> >  - baremetal is still using ticketlocks.
> > 
> > In other words everything in the kernel is ticketlock based with the virtualizations
> > having the 'PV' part to aid.
> > 
> > Please take a look at the patches. If you are interested in testing them
> > out I will post a git tree on Monday based on v3.11 and v3.12-rc0. 
> > 
> > Note that I had not run any performance tests and I am not sure when I will
> > be able to (got other bugs to squash now). I've asked our internal performance
> > engineer to do some benchmarking - but those results won't be available for
> > some time as it takes time to do good benchmarking.
> 
> If there was favourable performance data I would be happy for these to
> go in 3.12, without I think they'll have to wait for 3.13.

Then they will have to wait.

I am not going to be able to provide those within this month.
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[Konrad Rzeszutek Wilk]

On Mon, Sep 09, 2013 at 11:34:42AM +0100, David Vrabel wrote:
> On 07/09/13 14:46, Konrad Rzeszutek Wilk wrote:
> > After a bit of false starts, lots of debugging, and tons of help from Stefano and
> > David on how event mechanism is suppose to work I am happy to present a set
> > of bug-fixes that make PV ticketlocks work under Xen PVHVM with Linux v3.12.
> > 
> > v3.12 has now thanks to commit 816434ec4a674fcdb3c2221a6dffdc8f34020550
> > (Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)
> > PV ticketlocks. That means:
> >  - Xen PV bytelock has been replaced by Xen PV ticketlock.
> >  - Xen PVHVM is using ticketlock (not PV variant) - this patch makes it PV.
> >  - baremetal is still using ticketlocks.
> > 
> > In other words everything in the kernel is ticketlock based with the virtualizations
> > having the 'PV' part to aid.
> > 
> > Please take a look at the patches. If you are interested in testing them
> > out I will post a git tree on Monday based on v3.11 and v3.12-rc0. 
> > 
> > Note that I had not run any performance tests and I am not sure when I will
> > be able to (got other bugs to squash now). I've asked our internal performance
> > engineer to do some benchmarking - but those results won't be available for
> > some time as it takes time to do good benchmarking.
> 
> If there was favourable performance data I would be happy for these to
> go in 3.12, without I think they'll have to wait for 3.13.

Then they will have to wait.

I am not going to be able to provide those within this month.
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH] Bug-fixes to enable PV ticketlock to work under Xen PVHVM with Linux v3.12. (v1)

[Konrad Rzeszutek Wilk]

After a bit of false starts, lots of debugging, and tons of help from Stefano and
David on how event mechanism is suppose to work I am happy to present a set
of bug-fixes that make PV ticketlocks work under Xen PVHVM with Linux v3.12.

v3.12 has now thanks to commit 816434ec4a674fcdb3c2221a6dffdc8f34020550
(Merge branch 'x86-spinlocks-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip)
PV ticketlocks. That means:
 - Xen PV bytelock has been replaced by Xen PV ticketlock.
 - Xen PVHVM is using ticketlock (not PV variant) - this patch makes it PV.
 - baremetal is still using ticketlocks.

In other words everything in the kernel is ticketlock based with the virtualizations
having the 'PV' part to aid.

Please take a look at the patches. If you are interested in testing them
out I will post a git tree on Monday based on v3.11 and v3.12-rc0. 

Note that I had not run any performance tests and I am not sure when I will
be able to (got other bugs to squash now). I've asked our internal performance
engineer to do some benchmarking - but those results won't be available for
some time as it takes time to do good benchmarking.

 arch/x86/xen/enlighten.c |    1 -
 arch/x86/xen/smp.c       |   17 +++++++++++++++++
 arch/x86/xen/spinlock.c  |   45 ++++++++-------------------------------------
 3 files changed, 25 insertions(+), 38 deletions(-)

(oh neat, more deletions!) 

Konrad Rzeszutek Wilk (5):
      xen/spinlock: Fix locking path engaging too soon under PVHVM.
      xen/spinlock: We don't need the old structure anymore
      xen/smp: Update pv_lock_ops functions before alternative code starts under PVHVM
      xen/spinlock: Don't setup xen spinlock IPI kicker if disabled.
      Revert "xen/spinlock: Disable IRQ spinlock (PV) allocation on PVHVM"