kvm: optimize out smp_mb using srcu_read_unlock

RFC PATCH In review Michael S. Tsirkin 4 participants 11 messages Oct 30, 2013 – Nov 1, 2013

Subsystems (4)

x86 KVM KVM x86 Sleepable read-copy update (SRCU)

lore

Share & tools

lore

Michael S. Tsirkin

author

2013-10-30 19:06 UTC #0 lore raw

RFC PATCH 1/1 kvm: optimize out smp_mb using srcu_read_unlock

I noticed that srcu_read_lock/unlock both have a memory barrier, so just by moving srcu_read_unlock earlier we can get rid of one call to smp_mb().

Unsurprisingly, the gain is small but measureable using the unit test microbenchmark:
before

	vmcall 1407
after
	vmcall 1357

Signed-off-by Michael S. Tsirkin <[email protected]>

I didn't stress test this yet, sending out for early review/flames.

Paul, could you review this patch please? Documentation/memory-barriers.txt says that unlock has a weaker uni-directional barrier, but in practice srcu_read_unlock calls smp_mb().

Is it OK to rely on this? If not, can I add smp_mb__after_srcu_read_unlock (making it an empty macro for now) so we can avoid an actual extra smp_mb()?

Thanks.

arch/x86/kvm/x86.c +4 -3

index 8617c9d..a48fb36 100644
@@ -5949,8 +5949,10 @@ restore:
 
 	/* We should set ->mode before check ->requests,
 	 * see the comment in make_all_cpus_request.
+	 *
+	 * srcu_read_unlock below acts as a memory barrier.
 	 */
-	smp_mb();
+	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
 
 	local_irq_disable();

@@ -5960,12 +5962,11 @@ restore:
 		smp_wmb();
 		local_irq_enable();
 		preempt_enable();
+		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 		r = 1;
 		goto cancel_injection;
 	}
 
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
-
 	if (req_immediate_exit)
 		smp_send_reschedule(vcpu->cpu);

Signature · 1 line

MST

Paul E. McKenney

2013-10-30 20:15 UTC #1 lore raw

Quoted 17 lines · Michael S. Tsirkin

I noticed that srcu_read_lock/unlock both have a memory barrier, so just by moving srcu_read_unlock earlier we can get rid of one call to smp_mb().
Unsurprisingly, the gain is small but measureable using the unit test microbenchmark:
before
	vmcall 1407
after
	vmcall 1357
Signed-off-by Michael S. Tsirkin <[email protected]>
--
I didn't stress test this yet, sending out for early review/flames.

Quoted 8 lines

Paul, could you review this patch please? Documentation/memory-barriers.txt says that unlock has a weaker uni-directional barrier, but in practice srcu_read_unlock calls smp_mb().
Is it OK to rely on this? If not, can I add smp_mb__after_srcu_read_unlock (making it an empty macro for now) so we can avoid an actual extra smp_mb()?

Please use smp_mb__after_srcu_read_unlock(). After all, it was not that long ago that srcu_read_unlock() contained no memory barriers, and perhaps some day it won't need to once again.

Thanx, Paul

Quoted 36 lines

Thanks.

arch/x86/kvm/x86.c +4 -3

index 8617c9d..a48fb36 100644
@@ -5949,8 +5949,10 @@ restore:
 
 	/* We should set ->mode before check ->requests,
 	 * see the comment in make_all_cpus_request.
+	 *
+	 * srcu_read_unlock below acts as a memory barrier.
 	 */
-	smp_mb();
+	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
 
 	local_irq_disable();

@@ -5960,12 +5962,11 @@ restore:
 		smp_wmb();
 		local_irq_enable();
 		preempt_enable();
+		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 		r = 1;
 		goto cancel_injection;
 	}
 
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
-
 	if (req_immediate_exit)
 		smp_send_reschedule(vcpu->cpu);

Signature · 2 lines

-- 
MST

Michael S. Tsirkin

author

2013-10-30 23:23 UTC #2 lore raw

Quoted 9 lines

Quoted 8 lines
Paul, could you review this patch please? Documentation/memory-barriers.txt says that unlock has a weaker uni-directional barrier, but in practice srcu_read_unlock calls smp_mb().
Is it OK to rely on this? If not, can I add smp_mb__after_srcu_read_unlock (making it an empty macro for now) so we can avoid an actual extra smp_mb()?

Quoted 6 lines

Please use smp_mb__after_srcu_read_unlock(). After all, it was not that long ago that srcu_read_unlock() contained no memory barriers, and perhaps some day it won't need to once again.
Thanx, Paul

Thanks!
Something like this will be enough?

include/linux/srcu.h +14 -0

index c114614..9b058ee 100644
@@ -237,4 +237,18 @@ static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__srcu_read_unlock(sp, idx);
 }
 
+/**
+ * smp_mb__after_srcu_read_unlock - ensure full ordering after srcu_read_unlock
+ *
+ * Converts the preceding srcu_read_unlock into a two-way memory barrier.
+ *
+ * Call this after srcu_read_unlock, to guarantee that all memory operations
+ * that occur after smp_mb__after_srcu_read_unlock will appear to happen after
+ * the preceding srcu_read_unlock.
+ */
+static inline void smp_mb__after_srcu_read_unlock(void)
+{
+	/* __srcu_read_unlock has smp_mb() internally so nothing to do here. */
+}
+
 #endif

Gleb Natapov

2013-10-31 06:47 UTC #4 lore raw

Quoted 37 lines · Paul E. McKenney

Quoted 36 lines · Michael S. Tsirkin
Quoted 9 lines
Quoted 8 lines
Paul, could you review this patch please? Documentation/memory-barriers.txt says that unlock has a weaker uni-directional barrier, but in practice srcu_read_unlock calls smp_mb().
Is it OK to rely on this? If not, can I add smp_mb__after_srcu_read_unlock (making it an empty macro for now) so we can avoid an actual extra smp_mb()?
Quoted 6 lines
Please use smp_mb__after_srcu_read_unlock(). After all, it was not that long ago that srcu_read_unlock() contained no memory barriers, and perhaps some day it won't need to once again.
Thanx, Paul
Thanks!
Something like this will be enough?
include/linux/srcu.h +9 -0
index c114614..9b058ee 100644
@@ -237,4 +237,18 @@ static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__srcu_read_unlock(sp, idx);
 }
 
+/**
+ * smp_mb__after_srcu_read_unlock - ensure full ordering after srcu_read_unlock
+ *
+ * Converts the preceding srcu_read_unlock into a two-way memory barrier.
+ *
+ * Call this after srcu_read_unlock, to guarantee that all memory operations
+ * that occur after smp_mb__after_srcu_read_unlock will appear to happen after
+ * the preceding srcu_read_unlock.
+ */

Quoted 9 lines

include/linux/srcu.h static inline void srcu_read_unlock(struct srcu_struct *sp, int idx) · line 254
Quoted 6 lines
include/linux/srcu.h +5 -0
+static inline void smp_mb__after_srcu_read_unlock(void)
+{
+	/* __srcu_read_unlock has smp_mb() internally so nothing to do here. */
+}
+
 #endif
Yep, that should do it!

This looks dubious to me. All other smp_mb__after_* variants are there because some atomic operations have different memory barrier semantics on different arches, but srcu_read_unlock() have the same semantics on all arches, so smp_mb__after_srcu_read_unlock() becomes smp_mb__after_a_function_that_happens_to_have_mb_now_but_may_not_have_in_the_feature(). How likely it is that smp_mb() will disappear from srcu_read_unlock()
(if was added for a reason I guess)? May be we should change documentation to say that srcu_read_unlock() is a memory barrier which will reflect
the reality.

--
Gleb.

Paolo Bonzini

maintainer

2013-10-31 11:11 UTC #5 lore raw

Il 31/10/2013 07:47, Gleb Natapov ha scritto:

Quoted 3 lines

This looks dubious to me. All other smp_mb__after_* variants are there because some atomic operations have different memory barrier semantics on different arches,

It doesn't have to be arches; unlock APIs typically have release semantics only, but SRCU is stronger.

Quoted 7 lines

but srcu_read_unlock() have the same semantics on all
arches, so smp_mb__after_srcu_read_unlock() becomes smp_mb__after_a_function_that_happens_to_have_mb_now_but_may_not_have_in_the_feature(). How likely it is that smp_mb() will disappear from srcu_read_unlock()
(if was added for a reason I guess)? May be we should change documentation to say that srcu_read_unlock() is a memory barrier which will reflect
the reality.

That would be different from all other unlock APIs.

Paolo

Paolo Bonzini

maintainer

2013-10-31 11:14 UTC #9 lore raw

In reply to Michael S. Tsirkin

Il 30/10/2013 20:09, Michael S. Tsirkin ha scritto:

Quoted 4 lines

I noticed that srcu_read_lock/unlock both have a memory barrier, so just by moving srcu_read_unlock earlier we can get rid of one call to smp_mb().

Quoted 8 lines

Unsurprisingly, the gain is small but measureable using the unit test microbenchmark:
before
	vmcall 1407
after
	vmcall 1357
Signed-off-by Michael S. Tsirkin <[email protected]>

Across how many runs? Best or average or "all runs were in that ballpark", :) and what's the minimum/maximum before and after the patch?

As you say the benefit is not surprising, but the experiments should be documented properly.

Paolo

Michael S. Tsirkin

author

2013-10-31 11:30 UTC #10 lore raw

Quoted 12 lines · Paolo Bonzini

Il 30/10/2013 20:09, Michael S. Tsirkin ha scritto:
Quoted 11 lines
I noticed that srcu_read_lock/unlock both have a memory barrier, so just by moving srcu_read_unlock earlier we can get rid of one call to smp_mb().
Unsurprisingly, the gain is small but measureable using the unit test microbenchmark:
before
	vmcall 1407
after
	vmcall 1357

Quoted 3 lines

Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>
Across how many runs?

It's the experiment that vmexit test does:
it runs for 2^30 cycles, then divides the number of cycles by the number of iterations.
You get in the ballpark of 1300000 iterations normally.

Quoted 7 lines

Best or average or "all runs were in that
ballpark", :) and what's the minimum/maximum before and after the patch?
As you say the benefit is not surprising, but the experiments should be documented properly.
Paolo

"All runs in that ballpark".

Signature · 1 line

MST

Gleb Natapov

2013-10-31 12:28 UTC #6 lore raw

In reply to Paolo Bonzini

Quoted 6 lines · Paolo Bonzini

Il 31/10/2013 07:47, Gleb Natapov ha scritto:
Quoted 3 lines
This looks dubious to me. All other smp_mb__after_* variants are there because some atomic operations have different memory barrier semantics on different arches,
It doesn't have to be arches;

Of course it doesn't, but it is now :)

Quoted 3 lines

unlock APIs typically have release
semantics only, but SRCU is stronger.

Yes the question is if it is by design or implementation detail we should not rely on.

Quoted 10 lines

Quoted 7 lines
but srcu_read_unlock() have the same semantics on all
arches, so smp_mb__after_srcu_read_unlock() becomes smp_mb__after_a_function_that_happens_to_have_mb_now_but_may_not_have_in_the_feature(). How likely it is that smp_mb() will disappear from srcu_read_unlock()
(if was added for a reason I guess)? May be we should change documentation to say that srcu_read_unlock() is a memory barrier which will reflect
the reality.
That would be different from all other unlock APIs.

As long as it is documented... smp_mb__after_srcu_read_unlock() is just a form of documentation anyway right now. I do not have strong objection to smp_mb__after_srcu_read_unlock() though, the improvement is impressive for such a small change.

--
Gleb.

Michael S. Tsirkin

author

2013-10-31 13:54 UTC #7 lore raw

In reply to Paul E. McKenney

Quoted 46 lines · Paul E. McKenney

Quoted 32 lines · Michael S. Tsirkin
Quoted 9 lines
Quoted 8 lines
Paul, could you review this patch please? Documentation/memory-barriers.txt says that unlock has a weaker uni-directional barrier, but in practice srcu_read_unlock calls smp_mb().
Is it OK to rely on this? If not, can I add smp_mb__after_srcu_read_unlock (making it an empty macro for now) so we can avoid an actual extra smp_mb()?
Quoted 6 lines
Please use smp_mb__after_srcu_read_unlock(). After all, it was not that long ago that srcu_read_unlock() contained no memory barriers, and perhaps some day it won't need to once again.
Thanx, Paul
Thanks!
Something like this will be enough?
include/linux/srcu.h +5 -0
index c114614..9b058ee 100644
@@ -237,4 +237,18 @@ static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__srcu_read_unlock(sp, idx);
 }
 
+/**
+ * smp_mb__after_srcu_read_unlock - ensure full ordering after srcu_read_unlock
+ *
+ * Converts the preceding srcu_read_unlock into a two-way memory barrier.
+ *
include/linux/srcu.h static inline void srcu_read_unlock(struct srcu_struct *sp, int idx) · line 254
Quoted 10 lines
include/linux/srcu.h +9 -0
+ * Call this after srcu_read_unlock, to guarantee that all memory operations
+ * that occur after smp_mb__after_srcu_read_unlock will appear to happen after
+ * the preceding srcu_read_unlock.
+ */
+static inline void smp_mb__after_srcu_read_unlock(void)
+{
+	/* __srcu_read_unlock has smp_mb() internally so nothing to do here. */
+}
+
 #endif
Yep, that should do it!

Quoted 1 line

Thanx, Paul

BTW I'm wondering about the smb_mb within srcu_read_lock. If we kept the index in the same memory with the buffer we dereference, could we get rid of it and use a dependency barrier instead? It does appear prominently in the profiles. Thoughts?

Signature · 1 line

MST

Paul E. McKenney

2013-11-01 08:43 UTC #8 lore raw

Quoted 47 lines · Michael S. Tsirkin

Quoted 46 lines · Paul E. McKenney
Quoted 32 lines · Michael S. Tsirkin
Quoted 9 lines
Quoted 8 lines
Paul, could you review this patch please? Documentation/memory-barriers.txt says that unlock has a weaker uni-directional barrier, but in practice srcu_read_unlock calls smp_mb().
Is it OK to rely on this? If not, can I add smp_mb__after_srcu_read_unlock (making it an empty macro for now) so we can avoid an actual extra smp_mb()?
Quoted 6 lines
Please use smp_mb__after_srcu_read_unlock(). After all, it was not that long ago that srcu_read_unlock() contained no memory barriers, and perhaps some day it won't need to once again.
Thanx, Paul
Thanks!
Something like this will be enough?
include/linux/srcu.h +5 -0
index c114614..9b058ee 100644
@@ -237,4 +237,18 @@ static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__srcu_read_unlock(sp, idx);
 }
 
+/**
+ * smp_mb__after_srcu_read_unlock - ensure full ordering after srcu_read_unlock
+ *
+ * Converts the preceding srcu_read_unlock into a two-way memory barrier.
+ *
include/linux/srcu.h static inline void srcu_read_unlock(struct srcu_struct *sp, int idx) · line 254
Quoted 10 lines
include/linux/srcu.h +9 -0
+ * Call this after srcu_read_unlock, to guarantee that all memory operations
+ * that occur after smp_mb__after_srcu_read_unlock will appear to happen after
+ * the preceding srcu_read_unlock.
+ */
+static inline void smp_mb__after_srcu_read_unlock(void)
+{
+	/* __srcu_read_unlock has smp_mb() internally so nothing to do here. */
+}
+
 #endif
Yep, that should do it!

Quoted 7 lines

Quoted 1 line
Thanx, Paul
BTW I'm wondering about the smb_mb within srcu_read_lock. If we kept the index in the same memory with the buffer we dereference, could we get rid of it and use a dependency barrier instead? It does appear prominently in the profiles. Thoughts?

Unfortunately, no go:

	int __srcu_read_lock(struct srcu_struct *sp)
	{
		int idx;

		idx = ACCESS_ONCE(sp->completed) & 0x1;
		preempt_disable();
		ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->c[idx]) += 1;
		smp_mb(); /* B */  /* Avoid leaking the critical section. */
		ACCESS_ONCE(this_cpu_ptr(sp->per_cpu_ref)->seq[idx]) += 1;
		preempt_enable();
		return idx;
	}

The smp_mb() is between the two increments, and there is no dependency between them. There -could- be a dependency between the fetch of idx and the first increment, but there is no ordering required there because the rest of the algorithm will correctly handle any misordering. Which is why there is no memory barrier there.

Thanx, Paul