Paravirtualized ticket spinlocks

arch/x86/kernel/kvm.c +47 -0

+	w = &__get_cpu_var(lock_waiting);
+	cpu = smp_processor_id();
+	start = spin_time_start();
+
+	/*
+	 * Make sure an interrupt handler can't upset things in a
+	 * partially setup state.
+	 */
+	local_irq_save(flags);
+
+	/*
+	 * The ordering protocol on this is that the "lock" pointer
+	 * may only be set non-NULL if the "want" ticket is correct.
+	 * If we're updating "want", we must first clear "lock".
+	 */
+	w->lock = NULL;
+	smp_wmb();
+	w->want = want;
+	smp_wmb();
+	w->lock = lock;
+
+	add_stats(TAKEN_SLOW, 1);
+
+	/*
+	 * This uses set_bit, which is atomic but we should not rely on its
+	 * reordering gurantees. So barrier is needed after this call.
+	 */
+	cpumask_set_cpu(cpu, &waiting_cpus);
+
+	barrier();
+
+	/*
+	 * Mark entry to slowpath before doing the pickup test to make
+	 * sure we don't deadlock with an unlocker.
+	 */
+	__ticket_enter_slowpath(lock);
+
+	/*
+	 * check again make sure it didn't become free while
+	 * we weren't looking.
+	 */
+	if (ACCESS_ONCE(lock->tickets.head) == want) {
+		add_stats(TAKEN_SLOW_PICKUP, 1);
+		goto out;
+	}
+
+	/*

arch/x86/kernel/kvm.c +9 -0

+	cpumask_clear_cpu(cpu, &waiting_cpus);
+	w->lock = NULL;
+	local_irq_restore(flags);
+	spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)

arch/x86/kernel/kvm.c +14 -0

+			break;
+		}
+	}
+}
+
+/*
+ * Setup pv_lock_ops to exploit KVM_FEATURE_PV_UNHALT if present.
+ */
+void __init kvm_spinlock_init(void)
+{
+	if (!kvm_para_available())
+		return;
+	/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
+	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))

Review

Quoted 7 lines · Raghavendra K T

(quoted diff) +7 -0

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)
+{
+	struct kvm_lock_waiting *w;
+	int cpu;
+	u64 start;
+	unsigned long flags;
+

Why don't you bailout if in nmi here like we discussed?

Quoted 1 line · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

Quoted 7 lines

arch/x86/kernel/kvm.c +7 -0

+
+	/*
+	 * halt until it's our turn and kicked. Note that we do safe halt
+	 * for irq enabled case to avoid hang when lock info is overwritten
+	 * in irq spinlock slowpath and no spurious interrupt occur to save us.
+	 */
+	if (arch_irqs_disabled_flags(flags))

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 741

Quoted 5 lines

arch/x86/kernel/kvm.c +5 -0

+		halt();
+	else
+		safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them enabled at this point? I do not see any problem yet, will keep thinking.

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 742

Quoted 1 line

arch/x86/kernel/kvm.c +1 -0

+	cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 12 lines

arch/x86/kernel/kvm.c +12 -0

+	w->lock = NULL;
+	local_irq_restore(flags);
+	spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)
+{
+	int cpu;
+
+	add_stats(RELEASED_SLOW, 1);

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 760

Quoted 6 lines

arch/x86/kernel/kvm.c +6 -0

+	for_each_cpu(cpu, &waiting_cpus) {
+		const struct kvm_lock_waiting *w = &per_cpu(lock_waiting, cpu);
+		if (ACCESS_ONCE(w->lock) == lock &&
+		    ACCESS_ONCE(w->want) == ticket) {
+			add_stats(RELEASED_SLOW_KICKED, 1);
+			kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 2 lines · Gleb Natapov

Quoted 1 line · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 7 lines

Quoted 7 lines

arch/x86/kernel/kvm.c +7 -0

+
+	/*
+	 * halt until it's our turn and kicked. Note that we do safe halt
+	 * for irq enabled case to avoid hang when lock info is overwritten
+	 * in irq spinlock slowpath and no spurious interrupt occur to save us.
+	 */
+	if (arch_irqs_disabled_flags(flags))

Quoted 8 lines

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 741

Quoted 5 lines

arch/x86/kernel/kvm.c +5 -0

+		halt();
+	else
+		safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 1 line

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 742

Quoted 1 line

arch/x86/kernel/kvm.c +1 -0

+	cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Quoted 12 lines

Quoted 12 lines

arch/x86/kernel/kvm.c +12 -0

+	w->lock = NULL;
+	local_irq_restore(flags);
+	spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)
+{
+	int cpu;
+
+	add_stats(RELEASED_SLOW, 1);

Quoted 8 lines

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 760

Quoted 6 lines

arch/x86/kernel/kvm.c +6 -0

+	for_each_cpu(cpu, &waiting_cpus) {
+		const struct kvm_lock_waiting *w = &per_cpu(lock_waiting, cpu);
+		if (ACCESS_ONCE(w->lock) == lock &&
+		    ACCESS_ONCE(w->want) == ticket) {
+			add_stats(RELEASED_SLOW_KICKED, 1);
+			kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 23 lines · Raghavendra K T

Quoted 2 lines · Gleb Natapov

Quoted 1 line · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 7 lines

Quoted 7 lines

arch/x86/kernel/kvm.c +7 -0

+
+	/*
+	 * halt until it's our turn and kicked. Note that we do safe halt
+	 * for irq enabled case to avoid hang when lock info is overwritten
+	 * in irq spinlock slowpath and no spurious interrupt occur to save us.
+	 */
+	if (arch_irqs_disabled_flags(flags))

Quoted 8 lines

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 741

Quoted 5 lines

arch/x86/kernel/kvm.c +5 -0

+		halt();
+	else
+		safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 9 lines

Quoted 1 line

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 742

Quoted 1 line

arch/x86/kernel/kvm.c +1 -0

+	cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

arch/x86/kernel/kvm.c +8 -0

+	w->lock = NULL;
+	local_irq_restore(flags);
+	spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 760

Quoted 10 lines

arch/x86/kernel/kvm.c +10 -0

+{
+	int cpu;
+
+	add_stats(RELEASED_SLOW, 1);
+	for_each_cpu(cpu, &waiting_cpus) {
+		const struct kvm_lock_waiting *w = &per_cpu(lock_waiting, cpu);
+		if (ACCESS_ONCE(w->lock) == lock &&
+		    ACCESS_ONCE(w->want) == ticket) {
+			add_stats(RELEASED_SLOW_KICKED, 1);
+			kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting to see the numbers.

Quoted 57 lines · Gleb Natapov

Quoted 23 lines · Raghavendra K T

Quoted 2 lines · Gleb Natapov

Quoted 1 line · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 7 lines

Quoted 7 lines

arch/x86/kernel/kvm.c +7 -0

+
+	/*
+	 * halt until it's our turn and kicked. Note that we do safe halt
+	 * for irq enabled case to avoid hang when lock info is overwritten
+	 * in irq spinlock slowpath and no spurious interrupt occur to save us.
+	 */
+	if (arch_irqs_disabled_flags(flags))

Quoted 8 lines

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 741

Quoted 5 lines

arch/x86/kernel/kvm.c +5 -0

+		halt();
+	else
+		safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 9 lines

Quoted 1 line

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 742

Quoted 1 line

arch/x86/kernel/kvm.c +1 -0

+	cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

arch/x86/kernel/kvm.c +8 -0

+	w->lock = NULL;
+	local_irq_restore(flags);
+	spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 760

Quoted 10 lines

arch/x86/kernel/kvm.c +10 -0

+{
+	int cpu;
+
+	add_stats(RELEASED_SLOW, 1);
+	for_each_cpu(cpu, &waiting_cpus) {
+		const struct kvm_lock_waiting *w = &per_cpu(lock_waiting, cpu);
+		if (ACCESS_ONCE(w->lock) == lock &&
+		    ACCESS_ONCE(w->want) == ticket) {
+			add_stats(RELEASED_SLOW_KICKED, 1);
+			kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 8 lines

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

Quoted 72 lines · Raghavendra K T

Quoted 57 lines · Gleb Natapov

Quoted 23 lines · Raghavendra K T

Quoted 2 lines · Gleb Natapov

Quoted 1 line · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 7 lines

Quoted 7 lines

arch/x86/kernel/kvm.c +7 -0

+
+	/*
+	 * halt until it's our turn and kicked. Note that we do safe halt
+	 * for irq enabled case to avoid hang when lock info is overwritten
+	 * in irq spinlock slowpath and no spurious interrupt occur to save us.
+	 */
+	if (arch_irqs_disabled_flags(flags))

Quoted 8 lines

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 741

Quoted 5 lines

arch/x86/kernel/kvm.c +5 -0

+		halt();
+	else
+		safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 9 lines

Quoted 1 line

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 742

Quoted 1 line

arch/x86/kernel/kvm.c +1 -0

+	cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

arch/x86/kernel/kvm.c +8 -0

+	w->lock = NULL;
+	local_irq_restore(flags);
+	spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)

arch/x86/kernel/kvm.c static __init int activate_jump_labels(void) · line 760

Quoted 10 lines

arch/x86/kernel/kvm.c +10 -0

+{
+	int cpu;
+
+	add_stats(RELEASED_SLOW, 1);
+	for_each_cpu(cpu, &waiting_cpus) {
+		const struct kvm_lock_waiting *w = &per_cpu(lock_waiting, cpu);
+		if (ACCESS_ONCE(w->lock) == lock &&
+		    ACCESS_ONCE(w->want) == ticket) {
+			add_stats(RELEASED_SLOW_KICKED, 1);
+			kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 8 lines

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 82 lines · Gleb Natapov

Quoted 81 lines · Raghavendra K T

Quoted 65 lines · Gleb Natapov

Quoted 29 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

(quoted diff) +3 -0

+
+    /*
+     * halt until it's our turn and kicked. Note that we do safe

halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.

(quoted diff) +4 -0

+     */
+    if (arch_irqs_disabled_flags(flags))
+        halt();
+    else

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+        safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 9 lines

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

(quoted diff) +8 -0

+    w->lock = NULL;
+    local_irq_restore(flags);
+    spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,

__ticket_t ticket)

(quoted diff) +6 -0

+{
+    int cpu;
+
+    add_stats(RELEASED_SLOW, 1);
+    for_each_cpu(cpu, &waiting_cpus) {
+        const struct kvm_lock_waiting *w =

&per_cpu(lock_waiting, cpu);

Review

Quoted 4 lines

(quoted diff) +4 -0

+        if (ACCESS_ONCE(w->lock) == lock &&
+            ACCESS_ONCE(w->want) == ticket) {
+            add_stats(RELEASED_SLOW_KICKED, 1);
+            kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 9 lines

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

Quoted 94 lines · Raghavendra K T

Quoted 82 lines · Gleb Natapov

Quoted 81 lines · Raghavendra K T

Quoted 65 lines · Gleb Natapov

Quoted 29 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

(quoted diff) +3 -0

+
+    /*
+     * halt until it's our turn and kicked. Note that we do safe

halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.

(quoted diff) +4 -0

+     */
+    if (arch_irqs_disabled_flags(flags))
+        halt();
+    else

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+        safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 9 lines

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

(quoted diff) +8 -0

+    w->lock = NULL;
+    local_irq_restore(flags);
+    spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,

__ticket_t ticket)

(quoted diff) +6 -0

+{
+    int cpu;
+
+    add_stats(RELEASED_SLOW, 1);
+    for_each_cpu(cpu, &waiting_cpus) {
+        const struct kvm_lock_waiting *w =

&per_cpu(lock_waiting, cpu);

Review

Quoted 4 lines

(quoted diff) +4 -0

+        if (ACCESS_ONCE(w->lock) == lock &&
+            ACCESS_ONCE(w->want) == ticket) {
+            add_stats(RELEASED_SLOW_KICKED, 1);
+            kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 9 lines

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

Quoted 1 line

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

(note: I tried sending APIC_DM_REMRD IPI directly, which worked fine but hypercall way of handling still performed well from the results I saw).

Quoted 95 lines · Raghavendra K T

Quoted 94 lines · Raghavendra K T

Quoted 82 lines · Gleb Natapov

Quoted 81 lines · Raghavendra K T

Quoted 65 lines · Gleb Natapov

Quoted 29 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

(quoted diff) +3 -0

+
+    /*
+     * halt until it's our turn and kicked. Note that we do safe

halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.

(quoted diff) +4 -0

+     */
+    if (arch_irqs_disabled_flags(flags))
+        halt();
+    else

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+        safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 9 lines

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

(quoted diff) +8 -0

+    w->lock = NULL;
+    local_irq_restore(flags);
+    spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,

__ticket_t ticket)

(quoted diff) +6 -0

+{
+    int cpu;
+
+    add_stats(RELEASED_SLOW, 1);
+    for_each_cpu(cpu, &waiting_cpus) {
+        const struct kvm_lock_waiting *w =

&per_cpu(lock_waiting, cpu);

Review

Quoted 4 lines

(quoted diff) +4 -0

+        if (ACCESS_ONCE(w->lock) == lock &&
+            ACCESS_ONCE(w->want) == ticket) {
+            add_stats(RELEASED_SLOW_KICKED, 1);
+            kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but forgot why it was dismissed.

Quoted 9 lines

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

We can of course. IIRC the objection was that NMI handling path is very fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

Quoted 8 lines

Quoted 1 line

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

Quoted 5 lines

(note: I tried sending APIC_DM_REMRD IPI directly, which worked fine but hypercall way of handling still performed well from the results I saw).

You mean better? This is strange. Have you ran guest with x2apic?

Quoted 109 lines · Gleb Natapov

Quoted 108 lines · Raghavendra K T

Quoted 95 lines · Raghavendra K T

Quoted 86 lines · Gleb Natapov

Quoted 85 lines · Raghavendra K T

Quoted 70 lines · Gleb Natapov

Quoted 31 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

(quoted diff) +3 -0

+
+    /*
+     * halt until it's our turn and kicked. Note that we do safe

halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.

(quoted diff) +4 -0

+     */
+    if (arch_irqs_disabled_flags(flags))
+        halt();
+    else

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+        safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 8 lines

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value
for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

(quoted diff) +8 -0

+    w->lock = NULL;
+    local_irq_restore(flags);
+    spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,

__ticket_t ticket)

(quoted diff) +6 -0

+{
+    int cpu;
+
+    add_stats(RELEASED_SLOW, 1);
+    for_each_cpu(cpu, &waiting_cpus) {
+        const struct kvm_lock_waiting *w =

Quoted 8 lines

Review

Quoted 5 lines

&per_cpu(lock_waiting, cpu);

(quoted diff) +4 -0

+        if (ACCESS_ONCE(w->lock) == lock &&
+            ACCESS_ONCE(w->want) == ticket) {
+            add_stats(RELEASED_SLOW_KICKED, 1);
+            kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but
forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 7 lines

We can of course. IIRC the objection was that NMI handling path is very
fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some
complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 5 lines

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

Quoted 4 lines

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

aha.. Okay. will check that.

Quoted 110 lines · Raghavendra K T

Quoted 109 lines · Gleb Natapov

Quoted 108 lines · Raghavendra K T

Quoted 95 lines · Raghavendra K T

Quoted 86 lines · Gleb Natapov

Quoted 85 lines · Raghavendra K T

Quoted 70 lines · Gleb Natapov

Quoted 31 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

(quoted diff) +3 -0

+
+    /*
+     * halt until it's our turn and kicked. Note that we do safe

halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.

(quoted diff) +4 -0

+     */
+    if (arch_irqs_disabled_flags(flags))
+        halt();
+    else

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+        safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 8 lines

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value
for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

(quoted diff) +8 -0

+    w->lock = NULL;
+    local_irq_restore(flags);
+    spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,

__ticket_t ticket)

(quoted diff) +6 -0

+{
+    int cpu;
+
+    add_stats(RELEASED_SLOW, 1);
+    for_each_cpu(cpu, &waiting_cpus) {
+        const struct kvm_lock_waiting *w =

Quoted 8 lines

Review

Quoted 5 lines

&per_cpu(lock_waiting, cpu);

(quoted diff) +4 -0

+        if (ACCESS_ONCE(w->lock) == lock &&
+            ACCESS_ONCE(w->want) == ticket) {
+            add_stats(RELEASED_SLOW_KICKED, 1);
+            kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but
forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 7 lines

We can of course. IIRC the objection was that NMI handling path is very
fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some
complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 5 lines

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

Quoted 6 lines

Quoted 4 lines

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

aha.. Okay. will check that.

yes. Thanks.. that did the trick.

I did like below in unknown_nmi_error():
if (cpumask_test_cpu(smp_processor_id(), &waiting_cpus))
return;

But I believe you asked NMI method only for experimental purpose to check the upperbound. because as I doubted above, for spurious NMI (i.e. when unlocker kicks when waiter already got the lock), we would still hit unknown NMI error.

I had hit spurious NMI over 1656 times over entire benchmark run. along with
INFO: NMI handler (arch_trigger_all_cpu_backtrace_handler) took too long to run: 24.886 msecs etc...

(and we cannot get away with that too because it means we bypass the unknown NMI error even in genuine cases too)

Here was the result for the my dbench test( 32 core machine with 32 vcpu guest HT off)

---------- % improvement --------------
pvspinlock pvspin_ipi pvpsin_nmi

dbench_1x	0.9016    	0.7442    	0.7522
dbench_2x	14.7513   	18.0164   	15.9421
dbench_3x	14.7571   	17.0793   	13.3572
dbench_4x	6.3625    	8.7897    	5.3800

So I am seeing over 2-4% improvement with IPI method.

Gleb,
do you think the current series looks good to you? [one patch I
have resent with in_nmi() check] or do you think I have to respin the series with IPI method etc. or is there any concerns that I have to address. Please let me know..

Quoted 129 lines · Raghavendra K T

Quoted 110 lines · Raghavendra K T

Quoted 109 lines · Gleb Natapov

Quoted 108 lines · Raghavendra K T

Quoted 95 lines · Raghavendra K T

Quoted 86 lines · Gleb Natapov

Quoted 85 lines · Raghavendra K T

Quoted 70 lines · Gleb Natapov

Quoted 31 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock, __ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

(quoted diff) +3 -0

+
+    /*
+     * halt until it's our turn and kicked. Note that we do safe

halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.

(quoted diff) +4 -0

+     */
+    if (arch_irqs_disabled_flags(flags))
+        halt();
+    else

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+        safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 8 lines

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value
for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

(quoted diff) +8 -0

+    w->lock = NULL;
+    local_irq_restore(flags);
+    spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,

__ticket_t ticket)

(quoted diff) +6 -0

+{
+    int cpu;
+
+    add_stats(RELEASED_SLOW, 1);
+    for_each_cpu(cpu, &waiting_cpus) {
+        const struct kvm_lock_waiting *w =

Quoted 8 lines

Review

Quoted 5 lines

&per_cpu(lock_waiting, cpu);

(quoted diff) +4 -0

+        if (ACCESS_ONCE(w->lock) == lock &&
+            ACCESS_ONCE(w->want) == ticket) {
+            add_stats(RELEASED_SLOW_KICKED, 1);
+            kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but
forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 7 lines

We can of course. IIRC the objection was that NMI handling path is very
fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some
complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 5 lines

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

Quoted 6 lines

Quoted 4 lines

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

aha.. Okay. will check that.

yes. Thanks.. that did the trick.

I did like below in unknown_nmi_error():
if (cpumask_test_cpu(smp_processor_id(), &waiting_cpus))

    return;

But I believe you asked NMI method only for experimental purpose to check the upperbound. because as I doubted above, for spurious NMI (i.e. when unlocker kicks when waiter already got the lock), we would still hit unknown NMI error.

Quoted 5 lines

I had hit spurious NMI over 1656 times over entire benchmark run. along with
INFO: NMI handler (arch_trigger_all_cpu_backtrace_handler) took too long to run: 24.886 msecs etc...

I wonder why this happens.

Quoted 6 lines

(and we cannot get away with that too because it means we bypass the unknown NMI error even in genuine cases too)

Here was the result for the my dbench test( 32 core machine with 32 vcpu guest HT off)

Quoted 9 lines

---------- % improvement --------------
pvspinlock pvspin_ipi pvpsin_nmi

dbench_1x	0.9016    	0.7442    	0.7522
dbench_2x	14.7513   	18.0164   	15.9421
dbench_3x	14.7571   	17.0793   	13.3572
dbench_4x	6.3625    	8.7897    	5.3800

So I am seeing over 2-4% improvement with IPI method.

Yeah, this was expected.

Quoted 6 lines

Gleb,
do you think the current series looks good to you? [one patch I
have resent with in_nmi() check] or do you think I have to respin the series with IPI method etc. or is there any concerns that I have to address. Please let me know..

The current code looks fine to me.

Quoted 160 lines · Gleb Natapov

Quoted 129 lines · Raghavendra K T

Quoted 110 lines · Raghavendra K T

Quoted 109 lines · Gleb Natapov

Quoted 108 lines · Raghavendra K T

Quoted 95 lines · Raghavendra K T

Quoted 86 lines · Gleb Natapov

Quoted 85 lines · Raghavendra K T

Quoted 70 lines · Gleb Natapov

Quoted 31 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

+static void kvm_lock_spinning(struct arch_spinlock *lock,
__ticket_t want)

[...]

Quoted 12 lines

Quoted 12 lines

+
+ /*
+ * halt until it's our turn and kicked. Note that we do safe
halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.
+ */
+ if (arch_irqs_disabled_flags(flags))
+ halt();
+ else

Quoted 8 lines

Quoted 3 lines

+ safe_halt();
+
+out:

So here now interrupts can be either disabled or enabled. Previous version disabled interrupts here, so are we sure it is safe to have them
enabled at this point? I do not see any problem yet, will keep thinking.

If we enable interrupt here, then

Quoted 1 line

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 8 lines

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value
for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

+ w->lock = NULL;
+ local_irq_restore(flags);
+ spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,
__ticket_t ticket)
+{
+ int cpu;
+
+ add_stats(RELEASED_SLOW, 1);
+ for_each_cpu(cpu, &waiting_cpus) {
+ const struct kvm_lock_waiting *w =

Quoted 8 lines

Quoted 5 lines

&per_cpu(lock_waiting, cpu);
+ if (ACCESS_ONCE(w->lock) == lock &&
+ ACCESS_ONCE(w->want) == ticket) {
+ add_stats(RELEASED_SLOW_KICKED, 1);
+ kvm_kick_cpu(cpu);

What about using NMI to wake sleepers? I think it was discussed, but
forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 7 lines

We can of course. IIRC the objection was that NMI handling path is very
fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some
complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 5 lines

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

Quoted 6 lines

Quoted 4 lines

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

aha.. Okay. will check that.

yes. Thanks.. that did the trick.

I did like below in unknown_nmi_error():
if (cpumask_test_cpu(smp_processor_id(), &waiting_cpus))
return;

But I believe you asked NMI method only for experimental purpose to check the upperbound. because as I doubted above, for spurious NMI (i.e. when unlocker kicks when waiter already got the lock), we would still hit unknown NMI error.

Quoted 5 lines

I had hit spurious NMI over 1656 times over entire benchmark run. along with
INFO: NMI handler (arch_trigger_all_cpu_backtrace_handler) took too long to run: 24.886 msecs etc...

I wonder why this happens.

Quoted 6 lines

(and we cannot get away with that too because it means we bypass the unknown NMI error even in genuine cases too)

Here was the result for the my dbench test( 32 core machine with 32 vcpu guest HT off)

Quoted 9 lines

---------- % improvement --------------
pvspinlock pvspin_ipi pvpsin_nmi

dbench_1x	0.9016    	0.7442    	0.7522
dbench_2x	14.7513   	18.0164   	15.9421
dbench_3x	14.7571   	17.0793   	13.3572
dbench_4x	6.3625    	8.7897    	5.3800

So I am seeing over 2-4% improvement with IPI method.

Yeah, this was expected.

Quoted 6 lines

Gleb,
do you think the current series looks good to you? [one patch I
have resent with in_nmi() check] or do you think I have to respin the series with IPI method etc. or is there any concerns that I have to address. Please let me know..

Quoted 1 line

The current code looks fine to me.

Gleb,

Shall I consider this as an ack for kvm part?

Quoted 163 lines · Raghavendra K T

Quoted 161 lines · Gleb Natapov

Quoted 129 lines · Raghavendra K T

Quoted 110 lines · Raghavendra K T

Quoted 109 lines · Gleb Natapov

Quoted 108 lines · Raghavendra K T

Quoted 95 lines · Raghavendra K T

Quoted 86 lines · Gleb Natapov

Quoted 85 lines · Raghavendra K T

Quoted 70 lines · Gleb Natapov

Quoted 31 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

On Mon, Jul 22, 2013 at 11:50:16AM +0530, Raghavendra K T wrote:
+static void kvm_lock_spinning(struct arch_spinlock *lock,
__ticket_t want)

[...]

Quoted 20 lines

+
+ /*
+ * halt until it's our turn and kicked. Note that we do safe
halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.
+ */
+ if (arch_irqs_disabled_flags(flags))
+ halt();
+ else
+ safe_halt();
+
+out:
So here now interrupts can be either disabled or enabled. Previous
version disabled interrupts here, so are we sure it is safe to
have them
enabled at this point? I do not see any problem yet, will keep
thinking.

If we enable interrupt here, then

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 8 lines

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value
for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 24 lines

Quoted 23 lines

+ w->lock = NULL;
+ local_irq_restore(flags);
+ spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,
__ticket_t ticket)
+{
+ int cpu;
+
+ add_stats(RELEASED_SLOW, 1);
+ for_each_cpu(cpu, &waiting_cpus) {
+ const struct kvm_lock_waiting *w =
&per_cpu(lock_waiting, cpu);
+ if (ACCESS_ONCE(w->lock) == lock &&
+ ACCESS_ONCE(w->want) == ticket) {
+ add_stats(RELEASED_SLOW_KICKED, 1);
+ kvm_kick_cpu(cpu);
What about using NMI to wake sleepers? I think it was
discussed, but
forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 7 lines

We can of course. IIRC the objection was that NMI handling path is very
fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some
complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 5 lines

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

Quoted 6 lines

Quoted 4 lines

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

aha.. Okay. will check that.

yes. Thanks.. that did the trick.

I did like below in unknown_nmi_error():
if (cpumask_test_cpu(smp_processor_id(), &waiting_cpus))

    return;

But I believe you asked NMI method only for experimental purpose to check the upperbound. because as I doubted above, for spurious NMI (i.e. when unlocker kicks when waiter already got the lock), we would still hit unknown NMI error.

Quoted 5 lines

I had hit spurious NMI over 1656 times over entire benchmark run. along with
INFO: NMI handler (arch_trigger_all_cpu_backtrace_handler) took too long to run: 24.886 msecs etc...

I wonder why this happens.

Quoted 6 lines

(and we cannot get away with that too because it means we bypass the unknown NMI error even in genuine cases too)

Here was the result for the my dbench test( 32 core machine with 32 vcpu guest HT off)

Quoted 9 lines

---------- % improvement --------------
pvspinlock pvspin_ipi pvpsin_nmi

dbench_1x	0.9016    	0.7442    	0.7522
dbench_2x	14.7513   	18.0164   	15.9421
dbench_3x	14.7571   	17.0793   	13.3572
dbench_4x	6.3625    	8.7897    	5.3800

So I am seeing over 2-4% improvement with IPI method.

Yeah, this was expected.

Quoted 6 lines

Gleb,
do you think the current series looks good to you? [one patch I
have resent with in_nmi() check] or do you think I have to respin the series with IPI method etc. or is there any concerns that I have to address. Please let me know..

The current code looks fine to me.

Quoted 4 lines

Gleb,

Shall I consider this as an ack for kvm part?

For everything except 18/18. For that I still want to see numbers. But 18/18 is pretty independent from the reset of the series so it should not stop the reset from going in.

Quoted 5 lines · Gleb Natapov

Quoted 2 lines

Shall I consider this as an ack for kvm part?

For everything except 18/18. For that I still want to see numbers. But 18/18 is pretty independent from the reset of the series so it should not stop the reset from going in.

Yes. agreed.
I am going to evaluate patch 18 separately and come with results for that. Now we can consider only 1-17 patches.

Quoted 11 lines · Raghavendra K T

Quoted 5 lines · Gleb Natapov

Quoted 2 lines

Shall I consider this as an ack for kvm part?

For everything except 18/18. For that I still want to see numbers. But 18/18 is pretty independent from the reset of the series so it should not stop the reset from going in.

Yes. agreed.
I am going to evaluate patch 18 separately and come with results for that. Now we can consider only 1-17 patches.

Gleb,

32 core machine with HT off 32 vcpu guests.
base = 3.11-rc + patch 1 -17 pvspinlock_v11
patched = base + patch 18

+-----------+-----------+-----------+------------+-----------+
                  dbench  (Throughput in MB/sec higher is better)
+-----------+-----------+-----------+------------+-----------+
      base      stdev       patched    stdev       %improvement
+-----------+-----------+-----------+------------+-----------+
1x 14584.3800   146.9074   14705.1000   163.1060     0.82773
2x  1713.7300    32.8750    1717.3200    45.5979     0.20948
3x   967.8212    42.0257     971.8855    18.8532     0.41994
4x   685.2764    25.7150     694.5881     8.3907     1.35882
+-----------+-----------+-----------+------------+-----------+

base stdev patched stdev %improvement
3x 967.8212 4.3% 971.8855 1.8% 0.4

Quoted 19 lines · Raghavendra K T

Quoted 11 lines · Raghavendra K T

Quoted 5 lines · Gleb Natapov

Quoted 2 lines

Shall I consider this as an ack for kvm part?

For everything except 18/18. For that I still want to see numbers. But 18/18 is pretty independent from the reset of the series so it should not stop the reset from going in.

Yes. agreed.
I am going to evaluate patch 18 separately and come with results for that. Now we can consider only 1-17 patches.

Gleb,

32 core machine with HT off 32 vcpu guests.
base = 3.11-rc + patch 1 -17 pvspinlock_v11
patched = base + patch 18

Quoted 10 lines

+-----------+-----------+-----------+------------+-----------+
                   dbench  (Throughput in MB/sec higher is better)
+-----------+-----------+-----------+------------+-----------+
       base      stdev       patched    stdev       %improvement
+-----------+-----------+-----------+------------+-----------+
1x 14584.3800   146.9074   14705.1000   163.1060     0.82773
2x  1713.7300    32.8750    1717.3200    45.5979     0.20948
3x   967.8212    42.0257     971.8855    18.8532     0.41994
4x   685.2764    25.7150     694.5881     8.3907     1.35882
+-----------+-----------+-----------+------------+-----------+

Please list stddev in percentage as well ...

a blind stab gave me these figures:

Quoted 2 lines

       base      stdev       patched    stdev       %improvement
3x   967.8212    4.3%     971.8855      1.8%     0.4

That makes the improvement an order of magnitude smaller than the noise of the measurement ... i.e. totally inconclusive.

Also please cut the excessive decimal points: with 2-4% noise what point is there in 5 decimal point results??

Quoted 163 lines · Raghavendra K T

Quoted 161 lines · Gleb Natapov

Quoted 129 lines · Raghavendra K T

Quoted 110 lines · Raghavendra K T

Quoted 109 lines · Gleb Natapov

Quoted 108 lines · Raghavendra K T

Quoted 95 lines · Raghavendra K T

Quoted 86 lines · Gleb Natapov

Quoted 85 lines · Raghavendra K T

Quoted 70 lines · Gleb Natapov

Quoted 31 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

On Mon, Jul 22, 2013 at 11:50:16AM +0530, Raghavendra K T wrote:
+static void kvm_lock_spinning(struct arch_spinlock *lock,
__ticket_t want)

[...]

Quoted 20 lines

+
+ /*
+ * halt until it's our turn and kicked. Note that we do safe
halt
+ * for irq enabled case to avoid hang when lock info is
overwritten
+ * in irq spinlock slowpath and no spurious interrupt occur
to save us.
+ */
+ if (arch_irqs_disabled_flags(flags))
+ halt();
+ else
+ safe_halt();
+
+out:
So here now interrupts can be either disabled or enabled. Previous
version disabled interrupts here, so are we sure it is safe to
have them
enabled at this point? I do not see any problem yet, will keep
thinking.

If we enable interrupt here, then

Quoted 1 line

+ cpumask_clear_cpu(cpu, &waiting_cpus);

Quoted 8 lines

and if we start serving lock for an interrupt that came here, cpumask clear and w->lock=null may not happen atomically. if irq spinlock does not take slow path we would have non null value
for lock, but with no information in waitingcpu.

I am still thinking what would be problem with that.

Exactly, for kicker waiting_cpus and w->lock updates are non atomic anyway.

Quoted 24 lines

Quoted 23 lines

+ w->lock = NULL;
+ local_irq_restore(flags);
+ spin_time_accum_blocked(start);
+}
+PV_CALLEE_SAVE_REGS_THUNK(kvm_lock_spinning);
+
+/* Kick vcpu waiting on @lock->head to reach value @ticket */
+static void kvm_unlock_kick(struct arch_spinlock *lock,
__ticket_t ticket)
+{
+ int cpu;
+
+ add_stats(RELEASED_SLOW, 1);
+ for_each_cpu(cpu, &waiting_cpus) {
+ const struct kvm_lock_waiting *w =
&per_cpu(lock_waiting, cpu);
+ if (ACCESS_ONCE(w->lock) == lock &&
+ ACCESS_ONCE(w->want) == ticket) {
+ add_stats(RELEASED_SLOW_KICKED, 1);
+ kvm_kick_cpu(cpu);
What about using NMI to wake sleepers? I think it was
discussed, but
forgot why it was dismissed.

Quoted 3 lines

I think I have missed that discussion. 'll go back and check. so what is the idea here? we can easily wake up the halted vcpus that have interrupt disabled?

Quoted 7 lines

We can of course. IIRC the objection was that NMI handling path is very
fragile and handling NMI on each wakeup will be more expensive then waking up a guest without injecting an event, but it is still interesting
to see the numbers.

Haam, now I remember, We had tried request based mechanism. (new request like REQ_UNHALT) and process that. It had worked, but had some
complex hacks in vcpu_enter_guest to avoid guest hang in case of request cleared. So had left it there..

Quoted 7 lines

Quoted 3 lines

https://lkml.org/lkml/2012/4/30/67

But I do not remember performance impact though.

No, this is something different. Wakeup with NMI does not need KVM changes at
all. Instead of kvm_kick_cpu(cpu) in kvm_unlock_kick you send NMI IPI.

Quoted 5 lines

True. It was not NMI.
just to confirm, are you talking about something like this to be tried ?

apic->send_IPI_mask(cpumask_of(cpu), APIC_DM_NMI);

When I started benchmark, I started seeing
"Dazed and confused, but trying to continue" from unknown nmi error handling.
Did I miss anything (because we did not register any NMI handler)? or is it that spurious NMIs are trouble because we could get spurious NMIs if next waiter already acquired the lock.

Quoted 6 lines

Quoted 4 lines

There is a default NMI handler that tries to detect the reason why NMI happened (which is no so easy on x86) and prints this message if it fails. You need to add logic to detect spinlock slow path there. Check bit in waiting_cpus for instance.

aha.. Okay. will check that.

yes. Thanks.. that did the trick.

I did like below in unknown_nmi_error():
if (cpumask_test_cpu(smp_processor_id(), &waiting_cpus))

    return;

But I believe you asked NMI method only for experimental purpose to check the upperbound. because as I doubted above, for spurious NMI (i.e. when unlocker kicks when waiter already got the lock), we would still hit unknown NMI error.

Quoted 5 lines

I had hit spurious NMI over 1656 times over entire benchmark run. along with
INFO: NMI handler (arch_trigger_all_cpu_backtrace_handler) took too long to run: 24.886 msecs etc...

I wonder why this happens.

Quoted 6 lines

(and we cannot get away with that too because it means we bypass the unknown NMI error even in genuine cases too)

Here was the result for the my dbench test( 32 core machine with 32 vcpu guest HT off)

Quoted 9 lines

---------- % improvement --------------
pvspinlock pvspin_ipi pvpsin_nmi

dbench_1x	0.9016    	0.7442    	0.7522
dbench_2x	14.7513   	18.0164   	15.9421
dbench_3x	14.7571   	17.0793   	13.3572
dbench_4x	6.3625    	8.7897    	5.3800

So I am seeing over 2-4% improvement with IPI method.

Yeah, this was expected.

Quoted 6 lines

Gleb,
do you think the current series looks good to you? [one patch I
have resent with in_nmi() check] or do you think I have to respin the series with IPI method etc. or is there any concerns that I have to address. Please let me know..

The current code looks fine to me.

Quoted 4 lines

Gleb,

Shall I consider this as an ack for kvm part?

For everything except 18/18. For that I still want to see numbers. But 18/18 is pretty independent from the reset of the series so it should not stop the reset from going in.

Quoted 6 lines · Gleb Natapov

Quoted 5 lines · Raghavendra K T

Quoted 4 lines · Raghavendra K T

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Raghavendra K T

Quoted 1 line · Raghavendra K T

On 07/24/2013 05:36 PM, Gleb Natapov wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines · Raghavendra K T

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Raghavendra K T

Quoted 5 lines · Gleb Natapov

Quoted 3 lines · Raghavendra K T

Quoted 2 lines

+static void kvm_lock_spinning(struct arch_spinlock *lock,
__ticket_t want)

[...]