linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] pvclock: Add CPU barries to get correct version value
@ 2016-05-27  6:17 Minfei Huang
  2016-05-27  6:17 ` [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset Minfei Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Minfei Huang @ 2016-05-27  6:17 UTC (permalink / raw)
  To: bp, pbonzini, luto, hpa, mingo, tglx; +Cc: x86, linux-kernel, Minfei Huang

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.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 2 ++
 arch/x86/kernel/pvclock.c      | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..538ae94 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -85,6 +85,8 @@ unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
 	u8 ret_flags;
 
 	version = src->version;
+	/* Make the latest version visible */
+	smp_rmb();
 
 	offset = pvclock_get_nsec_offset(src);
 	ret = src->system_time + offset;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 99bfc02..7f82fe0 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -66,6 +66,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 
 	do {
 		version = __pvclock_read_cycles(src, &ret, &flags);
+		/* Make sure that the version double-check is last. */
+		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
 
 	return flags & valid_flags;
@@ -80,6 +82,8 @@ cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 
 	do {
 		version = __pvclock_read_cycles(src, &ret, &flags);
+		/* Make sure that the version double-check is last. */
+		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
 
 	if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-- 
2.6.3

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset
  2016-05-27  6:17 [PATCH 1/3] pvclock: Add CPU barries to get correct version value Minfei Huang
@ 2016-05-27  6:17 ` Minfei Huang
  2016-05-27 15:40   ` Andy Lutomirski
  2016-05-27  6:17 ` [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags Minfei Huang
  2016-05-27 15:39 ` [PATCH 1/3] pvclock: Add CPU barries to get correct version value Andy Lutomirski
  2 siblings, 1 reply; 20+ messages in thread
From: Minfei Huang @ 2016-05-27  6:17 UTC (permalink / raw)
  To: bp, pbonzini, luto, hpa, mingo, tglx; +Cc: x86, linux-kernel, Minfei Huang

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.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 538ae94..7c1c895 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -69,31 +69,22 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
 }
 
 static __always_inline
-u64 pvclock_get_nsec_offset(const struct pvclock_vcpu_time_info *src)
-{
-	u64 delta = rdtsc_ordered() - src->tsc_timestamp;
-	return pvclock_scale_delta(delta, src->tsc_to_system_mul,
-				   src->tsc_shift);
-}
-
-static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
 			       cycle_t *cycles, u8 *flags)
 {
 	unsigned version;
-	cycle_t ret, offset;
-	u8 ret_flags;
+	cycle_t offset;
+	u64 delta;
 
 	version = src->version;
 	/* Make the latest version visible */
 	smp_rmb();
 
-	offset = pvclock_get_nsec_offset(src);
-	ret = src->system_time + offset;
-	ret_flags = src->flags;
-
-	*cycles = ret;
-	*flags = ret_flags;
+	delta = rdtsc_ordered() - src->tsc_timestamp;
+	offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
+				   src->tsc_shift);
+	*cycles = src->system_time + offset;
+	*flags = src->flags;
 	return version;
 }
 
-- 
2.6.3

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags
  2016-05-27  6:17 [PATCH 1/3] pvclock: Add CPU barries to get correct version value Minfei Huang
  2016-05-27  6:17 ` [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset Minfei Huang
@ 2016-05-27  6:17 ` Minfei Huang
  2016-05-27 15:40   ` Andy Lutomirski
  2016-05-28 12:27   ` [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags Minfei Huang
  2016-05-27 15:39 ` [PATCH 1/3] pvclock: Add CPU barries to get correct version value Andy Lutomirski
  2 siblings, 2 replies; 20+ messages in thread
From: Minfei Huang @ 2016-05-27  6:17 UTC (permalink / raw)
  To: bp, pbonzini, luto, hpa, mingo, tglx; +Cc: x86, linux-kernel, 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.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 12 ++++++++++++
 arch/x86/kernel/pvclock.c      |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7c1c895..3c4e5b7 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -88,6 +88,18 @@ unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
 	return version;
 }
 
+static __always_inline unsigned int __pvclock_read_flags(
+		const struct pvclock_vcpu_time_info *src, u8 *flags)
+{
+	unsigned int version;
+
+	version = src->version;
+	/* Make the latest version visible */
+	smp_rmb();
+	*flags = src->flags;
+	return version;
+}
+
 struct pvclock_vsyscall_time_info {
 	struct pvclock_vcpu_time_info pvti;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..138772c 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,10 @@ 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 = __pvclock_read_flags(src, &flags);
 		/* Make sure that the version double-check is last. */
 		smp_rmb();
 	} while ((src->version & 1) || version != src->version);
-- 
2.6.3

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] pvclock: Add CPU barries to get correct version value
  2016-05-27  6:17 [PATCH 1/3] pvclock: Add CPU barries to get correct version value Minfei Huang
  2016-05-27  6:17 ` [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset Minfei Huang
  2016-05-27  6:17 ` [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags Minfei Huang
@ 2016-05-27 15:39 ` Andy Lutomirski
  2 siblings, 0 replies; 20+ messages in thread
From: Andy Lutomirski @ 2016-05-27 15:39 UTC (permalink / raw)
  To: Minfei Huang
  Cc: Borislav Petkov, Paolo Bonzini, Andrew Lutomirski,
	H. Peter Anvin, Ingo Molnar, Thomas Gleixner, X86 ML,
	linux-kernel

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> 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.

LGTM.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset
  2016-05-27  6:17 ` [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset Minfei Huang
@ 2016-05-27 15:40   ` Andy Lutomirski
  0 siblings, 0 replies; 20+ messages in thread
From: Andy Lutomirski @ 2016-05-27 15:40 UTC (permalink / raw)
  To: Minfei Huang
  Cc: Borislav Petkov, Paolo Bonzini, Andrew Lutomirski,
	H. Peter Anvin, Ingo Molnar, Thomas Gleixner, X86 ML,
	linux-kernel

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> 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.

LGTM.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags
  2016-05-27  6:17 ` [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags Minfei Huang
@ 2016-05-27 15:40   ` Andy Lutomirski
  2016-05-27 16:06     ` Paolo Bonzini
  2016-05-28 12:27   ` [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags Minfei Huang
  1 sibling, 1 reply; 20+ messages in thread
From: Andy Lutomirski @ 2016-05-27 15:40 UTC (permalink / raw)
  To: Minfei Huang
  Cc: Borislav Petkov, Paolo Bonzini, Andrew Lutomirski,
	H. Peter Anvin, Ingo Molnar, Thomas Gleixner, X86 ML,
	linux-kernel

On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> 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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags
  2016-05-27 15:40   ` Andy Lutomirski
@ 2016-05-27 16:06     ` Paolo Bonzini
  2016-05-28 12:02       ` Minfei Huang
  0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2016-05-27 16:06 UTC (permalink / raw)
  To: Andy Lutomirski, Minfei Huang
  Cc: Borislav Petkov, Andrew Lutomirski, H. Peter Anvin, Ingo Molnar,
	Thomas Gleixner, X86 ML, linux-kernel



On 27/05/2016 17:40, Andy Lutomirski wrote:
> On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
>> 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags
  2016-05-27 16:06     ` Paolo Bonzini
@ 2016-05-28 12:02       ` Minfei Huang
  0 siblings, 0 replies; 20+ messages in thread
From: Minfei Huang @ 2016-05-28 12:02 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Andy Lutomirski, Borislav Petkov, Andrew Lutomirski,
	H. Peter Anvin, Ingo Molnar, Thomas Gleixner, X86 ML,
	linux-kernel

On 05/27/16 at 06:06P, Paolo Bonzini wrote:
> 
> 
> On 27/05/2016 17:40, Andy Lutomirski wrote:
> > On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mnghuan@gmail.com> wrote:
> >> 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

Hmm, I will get rid of pvclock_read_flags, and repost it.

Thanks
Minfei

> pvclock_read_{begin,retry} functions like seqlock_read_{begin,retry}?
> 
> Thanks,
> 
> Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-05-27  6:17 ` [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags Minfei Huang
  2016-05-27 15:40   ` Andy Lutomirski
@ 2016-05-28 12:27   ` Minfei Huang
  2016-06-07 13:16     ` Minfei Huang
                       ` (2 more replies)
  1 sibling, 3 replies; 20+ messages in thread
From: Minfei Huang @ 2016-05-28 12:27 UTC (permalink / raw)
  To: bp, pbonzini, luto, hpa, mingo, tglx; +Cc: x86, linux-kernel, 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, get this variable
flags directly in function.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
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(-)

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..06c58ce 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -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);
-- 
2.6.3

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-05-28 12:27   ` [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags Minfei Huang
@ 2016-06-07 13:16     ` Minfei Huang
  2016-06-09 11:17       ` Paolo Bonzini
  2016-06-08  8:17     ` Borislav Petkov
  2016-06-09 12:16     ` Peter Zijlstra
  2 siblings, 1 reply; 20+ messages in thread
From: Minfei Huang @ 2016-06-07 13:16 UTC (permalink / raw)
  To: bp, pbonzini, luto, hpa, mingo, tglx; +Cc: x86, linux-kernel

ping. Any comment is appreciate.

Thanks
Minfei

On 05/28/16 at 08:27P, Minfei Huang wrote:
> 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 <mnghuan@gmail.com>
> ---
> 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(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -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);
> -- 
> 2.6.3
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-05-28 12:27   ` [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags Minfei Huang
  2016-06-07 13:16     ` Minfei Huang
@ 2016-06-08  8:17     ` Borislav Petkov
  2016-06-09 11:21       ` Paolo Bonzini
  2016-06-09 12:16     ` Peter Zijlstra
  2 siblings, 1 reply; 20+ messages in thread
From: Borislav Petkov @ 2016-06-08  8:17 UTC (permalink / raw)
  To: Minfei Huang; +Cc: pbonzini, luto, hpa, mingo, tglx, x86, linux-kernel

On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> 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 <mnghuan@gmail.com>
> ---
> 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(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -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. */

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

>  		smp_rmb();

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

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

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-07 13:16     ` Minfei Huang
@ 2016-06-09 11:17       ` Paolo Bonzini
  0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2016-06-09 11:17 UTC (permalink / raw)
  To: Minfei Huang, bp, luto, hpa, mingo, tglx; +Cc: x86, linux-kernel



On 07/06/2016 15:16, Minfei Huang wrote:
> ping. Any comment is appreciate.

The patch looks good, I'll follow up with another to introduce the
seqcount-like read_begin/read_retry API.

Thanks,

Paolo

> On 05/28/16 at 08:27P, Minfei Huang wrote:
>> 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 <mnghuan@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
>> index 7f82fe0..06c58ce 100644
>> --- a/arch/x86/kernel/pvclock.c
>> +++ b/arch/x86/kernel/pvclock.c
>> @@ -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);
>> -- 
>> 2.6.3
>>
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-08  8:17     ` Borislav Petkov
@ 2016-06-09 11:21       ` Paolo Bonzini
  2016-06-09 11:28         ` Borislav Petkov
  0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2016-06-09 11:21 UTC (permalink / raw)
  To: Borislav Petkov, Minfei Huang; +Cc: luto, hpa, mingo, tglx, x86, linux-kernel



On 08/06/2016 10:17, Borislav Petkov wrote:
>> > -		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.
> 
>> >  		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.

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-09 11:21       ` Paolo Bonzini
@ 2016-06-09 11:28         ` Borislav Petkov
  2016-06-09 12:31           ` Paolo Bonzini
  2016-06-12 11:46           ` Minfei Huang
  0 siblings, 2 replies; 20+ messages in thread
From: Borislav Petkov @ 2016-06-09 11:28 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Minfei Huang, luto, hpa, mingo, tglx, x86, linux-kernel

On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> This is basically implementing a seqcount.  It needs two barriers and,

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

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

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-05-28 12:27   ` [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags Minfei Huang
  2016-06-07 13:16     ` Minfei Huang
  2016-06-08  8:17     ` Borislav Petkov
@ 2016-06-09 12:16     ` Peter Zijlstra
  2016-06-09 12:24       ` Peter Zijlstra
  2016-06-09 12:26       ` Paolo Bonzini
  2 siblings, 2 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-06-09 12:16 UTC (permalink / raw)
  To: Minfei Huang; +Cc: bp, pbonzini, luto, hpa, mingo, tglx, x86, linux-kernel

On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> +++ b/arch/x86/kernel/pvclock.c
> @@ -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;

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

>  		/* Make sure that the version double-check is last. */
>  		smp_rmb();
>  	} while ((src->version & 1) || version != src->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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-09 12:16     ` Peter Zijlstra
@ 2016-06-09 12:24       ` Peter Zijlstra
  2016-06-09 12:26       ` Paolo Bonzini
  1 sibling, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-06-09 12:24 UTC (permalink / raw)
  To: Minfei Huang; +Cc: bp, pbonzini, luto, hpa, mingo, tglx, x86, linux-kernel

On Thu, Jun 09, 2016 at 02:16:03PM +0200, Peter Zijlstra wrote:
> 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.

Something like so; although given that I've never seen this code before
I can easily have missed an update site.

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 99bfc025111d..7a4bf59aa929 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -60,15 +60,7 @@ 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);
-	} while ((src->version & 1) || version != src->version);
-
-	return flags & valid_flags;
+	return READ_ONCE(src->flags) & valid_flags;
 }
 
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
@@ -83,7 +75,8 @@ cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 	} while ((src->version & 1) || version != src->version);
 
 	if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-		src->flags &= ~PVCLOCK_GUEST_STOPPED;
+		flags &= ~PVCLOCK_GUEST_STOPPED;
+		WRITE_ONCE(src->flags, flags);
 		pvclock_touch_watchdogs();
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1ba3b7d3cae9..54458acce4f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1832,7 +1832,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	if (use_master_clock)
 		pvclock_flags |= PVCLOCK_TSC_STABLE_BIT;
 
-	vcpu->hv_clock.flags = pvclock_flags;
+	WRITE_ONCE(vcpu->hv_clock.flags, pvclock_flags);
 
 	trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock);
 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-09 12:16     ` Peter Zijlstra
  2016-06-09 12:24       ` Peter Zijlstra
@ 2016-06-09 12:26       ` Paolo Bonzini
  2016-06-09 12:35         ` Peter Zijlstra
  1 sibling, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2016-06-09 12:26 UTC (permalink / raw)
  To: Peter Zijlstra, Minfei Huang
  Cc: bp, luto, hpa, mingo, tglx, x86, linux-kernel



On 09/06/2016 14:16, Peter Zijlstra wrote:
> On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
>> +++ b/arch/x86/kernel/pvclock.c
>> @@ -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;
> 
> 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.

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-09 11:28         ` Borislav Petkov
@ 2016-06-09 12:31           ` Paolo Bonzini
  2016-06-12 11:46           ` Minfei Huang
  1 sibling, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2016-06-09 12:31 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Minfei Huang, luto, hpa, mingo, tglx, x86, linux-kernel



On 09/06/2016 13:28, Borislav Petkov wrote:
>> > 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.

The compiler can reorder them, so smp_rmb() and virt_rmb() have to be
barrier(), but the processor won't.

x86 will only move a store after a subsequent load, if you exclude
special cases such as write combining, non-temporal moves and the like.
LFENCE and SFENCE are only needed for those special cases.

Paolo

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-09 12:26       ` Paolo Bonzini
@ 2016-06-09 12:35         ` Peter Zijlstra
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Zijlstra @ 2016-06-09 12:35 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Minfei Huang, bp, luto, hpa, mingo, tglx, x86, linux-kernel

On Thu, Jun 09, 2016 at 02:26:59PM +0200, Paolo Bonzini wrote:
> 
> 
> On 09/06/2016 14:16, Peter Zijlstra wrote:
> > On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> >> +++ b/arch/x86/kernel/pvclock.c
> >> @@ -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;
> > 
> > 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.

Fair enough; just thought it was really silly code.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags
  2016-06-09 11:28         ` Borislav Petkov
  2016-06-09 12:31           ` Paolo Bonzini
@ 2016-06-12 11:46           ` Minfei Huang
  1 sibling, 0 replies; 20+ messages in thread
From: Minfei Huang @ 2016-06-12 11:46 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Paolo Bonzini, luto, hpa, mingo, tglx, x86, linux-kernel

On 06/09/16 at 01:28P, Borislav Petkov wrote:
> On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> > This is basically implementing a seqcount.  It needs two barriers and,
> 
> Why does it need the two barriers? More details please.

Hi, Borislav.

It's a seqcount-like. We should confirm that reading flags occurs
between two reading version. To make complier do not reorder, two
barriers are necessary.

Thanks
Minfei

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2016-06-12 11:46 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-27  6:17 [PATCH 1/3] pvclock: Add CPU barries to get correct version value Minfei Huang
2016-05-27  6:17 ` [PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset Minfei Huang
2016-05-27 15:40   ` Andy Lutomirski
2016-05-27  6:17 ` [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags Minfei Huang
2016-05-27 15:40   ` Andy Lutomirski
2016-05-27 16:06     ` Paolo Bonzini
2016-05-28 12:02       ` Minfei Huang
2016-05-28 12:27   ` [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags Minfei Huang
2016-06-07 13:16     ` Minfei Huang
2016-06-09 11:17       ` Paolo Bonzini
2016-06-08  8:17     ` Borislav Petkov
2016-06-09 11:21       ` Paolo Bonzini
2016-06-09 11:28         ` Borislav Petkov
2016-06-09 12:31           ` Paolo Bonzini
2016-06-12 11:46           ` Minfei Huang
2016-06-09 12:16     ` Peter Zijlstra
2016-06-09 12:24       ` Peter Zijlstra
2016-06-09 12:26       ` Paolo Bonzini
2016-06-09 12:35         ` Peter Zijlstra
2016-05-27 15:39 ` [PATCH 1/3] pvclock: Add CPU barries to get correct version value Andy Lutomirski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).