pvclock: Add CPU barries to get correct version value

Function __pvclock_read_cycles is short enough, so there is no need to have another function pvclock_get_nsec_offset to calculate tsc delta. It's better to combine it into function __pvclock_read_cycles.

Remove useless variables in function __pvclock_read_cycles.

There is a generic function __pvclock_read_cycles to be used to get both flags and cycles. For function pvclock_read_flags, it's useless to get cycles value. To make this function be more effective, add a new wrapper function to only get variable flags.

Quoted 4 lines · Minfei Huang

There is a generic function __pvclock_read_cycles to be used to get both flags and cycles. For function pvclock_read_flags, it's useless to get cycles value. To make this function be more effective, add a new wrapper function to only get variable flags.

Why not just get rid of this function entirely?

Quoted 1 line · Andy Lutomirski

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

There is a generic function __pvclock_read_cycles to be used to get both flags and cycles. For function pvclock_read_flags, it's useless to get cycles value. To make this function be more effective, add a new wrapper function to only get variable flags.

Why not just get rid of this function entirely?

I agree. If you want to abstract the retry logic, perhaps you can add

pvclock_read_{begin,retry} functions like seqlock_read_{begin,retry}?

Thanks,

Paolo

There is a generic function __pvclock_read_cycles to be used to get both flags and cycles. For function pvclock_read_flags, it's useless to get cycles value. To make this function be more effective, get this variable flags directly in function.

Signed-off-by Minfei Huang <[email protected]>

---
v1:
- Get rid of __pvclock_read_cycles according to Andy's suggestion

---
 arch/x86/kernel/pvclock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

arch/x86/kernel/pvclock.c +5 -2

index 7f82fe0..06c58ce 100644
@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;
 	u8 flags;
 
 	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = src->version;
+		/* Make the latest version visible */
+		smp_rmb();
+
+		flags = src->flags;
 		/* Make sure that the version double-check is last. */
 		smp_rmb();
 	} while ((src->version & 1) || version != src->version);

Signature · 1 line

2.6.3

ping. Any comment is appreciate.

Quoted 37 lines

There is a generic function __pvclock_read_cycles to be used to get both flags and cycles. For function pvclock_read_flags, it's useless to get cycles value. To make this function be more effective, get this variable flags directly in function.

Signed-off-by Minfei Huang <[email protected]>

---
v1:
- Get rid of __pvclock_read_cycles according to Andy's suggestion

---
 arch/x86/kernel/pvclock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

arch/x86/kernel/pvclock.c +5 -2

index 7f82fe0..06c58ce 100644
@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;
 	u8 flags;
 
 	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = src->version;
+		/* Make the latest version visible */
+		smp_rmb();
+
+		flags = src->flags;
 		/* Make sure that the version double-check is last. */
 		smp_rmb();
 	} while ((src->version & 1) || version != src->version);

Signature · 1 line

2.6.3

There is a generic function __pvclock_read_cycles to be used to get both flags and cycles. For function pvclock_read_flags, it's useless to get cycles value. To make this function be more effective, get this variable flags directly in function.

Signed-off-by Minfei Huang <[email protected]>

---
v1:
- Get rid of __pvclock_read_cycles according to Andy's suggestion

---
 arch/x86/kernel/pvclock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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

index 7f82fe0..06c58ce 100644
@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;

arch/x86/kernel/pvclock.c +0 -0

 	} while ((src->version & 1) || version != src->version);

Quoted 7 lines

arch/x86/kernel/pvclock.c void pvclock_resume(void) · line 72

Quoted 7 lines

arch/x86/kernel/pvclock.c +5 -1

-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = src->version;
+		/* Make the latest version visible */
+		smp_rmb();
+
+		flags = src->flags;
 		/* Make sure that the version double-check is last. */

What does that comment mean over the barrier? It should be over the "while" line IMO.

Quoted 1 line

Quoted 1 line

smp_rmb();

Why the two barriers back-to-back? Can't have one at the end for all?

This is basically implementing a seqcount. It needs two barriers and,

technically, they should be virt_rmb() -- it really doesn't matter of course because reads are never reordered on x86.

Quoted 2 lines

Quoted 2 lines

technically, they should be virt_rmb() -- it really doesn't matter of course because reads are never reordered on x86.

You mean

	version = src->version;
	flags = src->flags;

are not reordered?

I don't think so.

Quoted 1 line · Paolo Bonzini

This is basically implementing a seqcount. It needs two barriers and,

Why does it need the two barriers? More details please.

+++ b/arch/x86/kernel/pvclock.c

arch/x86/kernel/pvclock.c +0 -0

@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;

What's wrong with:

	u8 flags = READ_ONCE(src->flags);

?

(and have the flags store be done using WRITE_ONCE() of course).

Sure, if your total state is larger than one word you need the seqcount for integrity, but reading _one_ byte, shees.

Quoted 8 lines · Minfei Huang

+++ b/arch/x86/kernel/pvclock.c

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

@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;
 	u8 flags;
 

arch/x86/kernel/pvclock.c void pvclock_resume(void) · line 71

Quoted 7 lines

arch/x86/kernel/pvclock.c +5 -1

 	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = src->version;
+		/* Make the latest version visible */
+		smp_rmb();
+
+		flags = src->flags;

Using a seqcount to load a single byte is insane ;-)

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Minfei Huang

+++ b/arch/x86/kernel/pvclock.c

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

@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
 	unsigned version;
-	cycle_t ret;
 	u8 flags;
 

Quoted 9 lines

arch/x86/kernel/pvclock.c void pvclock_resume(void) · line 71

Quoted 7 lines

arch/x86/kernel/pvclock.c +5 -1

 	do {
-		version = __pvclock_read_cycles(src, &ret, &flags);
+		version = src->version;
+		/* Make the latest version visible */
+		smp_rmb();
+
+		flags = src->flags;

Using a seqcount to load a single byte is insane ;-)

Only if you know that the writer will not write that byte twice within a critical section...

Which I guess we do know in this case because the write side is just a memcpy, but it's still a bit safer when it's not specified by the pvclock API. It's not a fast path anyway, it runs literally twice at startup.

Protocol for the "version" fields is: hypervisor raises it (making it uneven) before it starts updating the fields and raises it again (making it even) when it is done. Thus the guest can make sure the time values it got are consistent by checking the version before and after reading them.

Add CPU barries after getting version value just like what function vread_pvclock does, because all of callees in this function is inline.