Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301073 > unrolled thread

Re: [PATCH v2 1/4] x86, vdso, pvclock: Simplify and speed up the vdso pvclock reader

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/4] x86, vdso, pvclock: Simplify and speed up the  vdso pvclock reader Marcelo Tosatti <mtosatti@redhat.com> - 2016-01-04 21:50 +0100
    Re: [PATCH v2 1/4] x86, vdso, pvclock: Simplify and speed up the vdso  pvclock reader Andy Lutomirski <luto@amacapital.net> - 2016-01-04 23:40 +0100
      Re: [PATCH v2 1/4] x86, vdso, pvclock: Simplify and speed up the  vdso pvclock reader Marcelo Tosatti <mtosatti@redhat.com> - 2016-01-05 00:00 +0100
        [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Andy Lutomirski <luto@kernel.org> - 2016-01-05 00:20 +0100
          Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Marcelo Tosatti <mtosatti@redhat.com> - 2016-01-07 22:10 +0100
            Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Andy Lutomirski <luto@amacapital.net> - 2016-01-07 22:20 +0100
              Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Paolo Bonzini <pbonzini@redhat.com> - 2016-01-07 22:50 +0100
              Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Marcelo Tosatti <mtosatti@redhat.com> - 2016-01-08 20:50 +0100
                Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Andy Lutomirski <luto@amacapital.net> - 2016-01-12 20:50 +0100
                  Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount Ingo Molnar <mingo@kernel.org> - 2016-01-13 11:50 +0100
          [tip:x86/urgent] x86/vdso/pvclock:   Protect STABLE check with the seqcount tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2016-01-14 10:10 +0100

#1301073 — Re: [PATCH v2 1/4] x86, vdso, pvclock: Simplify and speed up the vdso pvclock reader

On Sun, Dec 20, 2015 at 03:05:41AM -0800, Andy Lutomirski wrote:
> From: Andy Lutomirski <luto@amacapital.net>
> 
> The pvclock vdso code was too abstracted to understand easily and
> excessively paranoid.  Simplify it for a huge speedup.
> 
> This opens the door for additional simplifications, as the vdso no
> longer accesses the pvti for any vcpu other than vcpu 0.
> 
> Before, vclock_gettime using kvm-clock took about 45ns on my machine.
> With this change, it takes 29ns, which is almost as fast as the pure TSC
> implementation.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/entry/vdso/vclock_gettime.c | 81 ++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
> index ca94fa649251..c325ba1bdddf 100644
> --- a/arch/x86/entry/vdso/vclock_gettime.c
> +++ b/arch/x86/entry/vdso/vclock_gettime.c
> @@ -78,47 +78,58 @@ static notrace const struct pvclock_vsyscall_time_info *get_pvti(int cpu)
>  
>  static notrace cycle_t vread_pvclock(int *mode)
>  {
> -	const struct pvclock_vsyscall_time_info *pvti;
> +	const struct pvclock_vcpu_time_info *pvti = &get_pvti(0)->pvti;
>  	cycle_t ret;
> -	u64 last;
> -	u32 version;
> -	u8 flags;
> -	unsigned cpu, cpu1;
> -
> +	u64 tsc, pvti_tsc;
> +	u64 last, delta, pvti_system_time;
> +	u32 version, pvti_tsc_to_system_mul, pvti_tsc_shift;
>  
>  	/*
> -	 * Note: hypervisor must guarantee that:
> -	 * 1. cpu ID number maps 1:1 to per-CPU pvclock time info.
> -	 * 2. that per-CPU pvclock time info is updated if the
> -	 *    underlying CPU changes.
> -	 * 3. that version is increased whenever underlying CPU
> -	 *    changes.
> +	 * Note: The kernel and hypervisor must guarantee that cpu ID
> +	 * number maps 1:1 to per-CPU pvclock time info.
> +	 *
> +	 * Because the hypervisor is entirely unaware of guest userspace
> +	 * preemption, it cannot guarantee that per-CPU pvclock time
> +	 * info is updated if the underlying CPU changes or that that
> +	 * version is increased whenever underlying CPU changes.
>  	 *
> +	 * On KVM, we are guaranteed that pvti updates for any vCPU are
> +	 * atomic as seen by *all* vCPUs.  This is an even stronger
> +	 * guarantee than we get with a normal seqlock.
> +	 *
> +	 * On Xen, we don't appear to have that guarantee, but Xen still
> +	 * supplies a valid seqlock using the version field.
> +
> +	 * We only do pvclock vdso timing at all if
> +	 * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
> +	 * mean that all vCPUs have matching pvti and that the TSC is
> +	 * synced, so we can just look at vCPU 0's pvti.
>  	 */
> -	do {
> -		cpu = __getcpu() & VGETCPU_CPU_MASK;
> -		/* TODO: We can put vcpu id into higher bits of pvti.version.
> -		 * This will save a couple of cycles by getting rid of
> -		 * __getcpu() calls (Gleb).
> -		 */
> -
> -		pvti = get_pvti(cpu);
> -
> -		version = __pvclock_read_cycles(&pvti->pvti, &ret, &flags);
> -
> -		/*
> -		 * Test we're still on the cpu as well as the version.
> -		 * We could have been migrated just after the first
> -		 * vgetcpu but before fetching the version, so we
> -		 * wouldn't notice a version change.
> -		 */
> -		cpu1 = __getcpu() & VGETCPU_CPU_MASK;
> -	} while (unlikely(cpu != cpu1 ||
> -			  (pvti->pvti.version & 1) ||
> -			  pvti->pvti.version != version));
> -
> -	if (unlikely(!(flags & PVCLOCK_TSC_STABLE_BIT)))
> +
> +	if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>  		*mode = VCLOCK_NONE;
> +		return 0;
> +	}
> +
> +	do {
> +		version = pvti->version;
> +
> +		/* This is also a read barrier, so we'll read version first. */
> +		tsc = rdtsc_ordered();
> +
> +		pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
> +		pvti_tsc_shift = pvti->tsc_shift;
> +		pvti_system_time = pvti->system_time;
> +		pvti_tsc = pvti->tsc_timestamp;
> +
> +		/* Make sure that the version double-check is last. */
> +		smp_rmb();
> +	} while (unlikely((version & 1) || version != pvti->version));

Andy,

What happens if PVCLOCK_TSC_STABLE_BIT is disabled here?

> +
> +	delta = tsc - pvti_tsc;
> +	ret = pvti_system_time +
> +		pvclock_scale_delta(delta, pvti_tsc_to_system_mul,
> +				    pvti_tsc_shift);
>  
>  	/* refer to tsc.c read_tsc() comment for rationale */
>  	last = gtod->cycle_last;
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301167 — Re: [PATCH v2 1/4] x86, vdso, pvclock: Simplify and speed up the vdso pvclock reader

On Mon, Jan 4, 2016 at 12:26 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Sun, Dec 20, 2015 at 03:05:41AM -0800, Andy Lutomirski wrote:
>> From: Andy Lutomirski <luto@amacapital.net>
>>
>> The pvclock vdso code was too abstracted to understand easily and
>> excessively paranoid.  Simplify it for a huge speedup.
>>
>> This opens the door for additional simplifications, as the vdso no
>> longer accesses the pvti for any vcpu other than vcpu 0.
>>
>> Before, vclock_gettime using kvm-clock took about 45ns on my machine.
>> With this change, it takes 29ns, which is almost as fast as the pure TSC
>> implementation.
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  arch/x86/entry/vdso/vclock_gettime.c | 81 ++++++++++++++++++++----------------
>>  1 file changed, 46 insertions(+), 35 deletions(-)
>>
>> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
>> index ca94fa649251..c325ba1bdddf 100644
>> --- a/arch/x86/entry/vdso/vclock_gettime.c
>> +++ b/arch/x86/entry/vdso/vclock_gettime.c
>> @@ -78,47 +78,58 @@ static notrace const struct pvclock_vsyscall_time_info *get_pvti(int cpu)
>>
>>  static notrace cycle_t vread_pvclock(int *mode)
>>  {
>> -     const struct pvclock_vsyscall_time_info *pvti;
>> +     const struct pvclock_vcpu_time_info *pvti = &get_pvti(0)->pvti;
>>       cycle_t ret;
>> -     u64 last;
>> -     u32 version;
>> -     u8 flags;
>> -     unsigned cpu, cpu1;
>> -
>> +     u64 tsc, pvti_tsc;
>> +     u64 last, delta, pvti_system_time;
>> +     u32 version, pvti_tsc_to_system_mul, pvti_tsc_shift;
>>
>>       /*
>> -      * Note: hypervisor must guarantee that:
>> -      * 1. cpu ID number maps 1:1 to per-CPU pvclock time info.
>> -      * 2. that per-CPU pvclock time info is updated if the
>> -      *    underlying CPU changes.
>> -      * 3. that version is increased whenever underlying CPU
>> -      *    changes.
>> +      * Note: The kernel and hypervisor must guarantee that cpu ID
>> +      * number maps 1:1 to per-CPU pvclock time info.
>> +      *
>> +      * Because the hypervisor is entirely unaware of guest userspace
>> +      * preemption, it cannot guarantee that per-CPU pvclock time
>> +      * info is updated if the underlying CPU changes or that that
>> +      * version is increased whenever underlying CPU changes.
>>        *
>> +      * On KVM, we are guaranteed that pvti updates for any vCPU are
>> +      * atomic as seen by *all* vCPUs.  This is an even stronger
>> +      * guarantee than we get with a normal seqlock.
>> +      *
>> +      * On Xen, we don't appear to have that guarantee, but Xen still
>> +      * supplies a valid seqlock using the version field.
>> +
>> +      * We only do pvclock vdso timing at all if
>> +      * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>> +      * mean that all vCPUs have matching pvti and that the TSC is
>> +      * synced, so we can just look at vCPU 0's pvti.
>>        */
>> -     do {
>> -             cpu = __getcpu() & VGETCPU_CPU_MASK;
>> -             /* TODO: We can put vcpu id into higher bits of pvti.version.
>> -              * This will save a couple of cycles by getting rid of
>> -              * __getcpu() calls (Gleb).
>> -              */
>> -
>> -             pvti = get_pvti(cpu);
>> -
>> -             version = __pvclock_read_cycles(&pvti->pvti, &ret, &flags);
>> -
>> -             /*
>> -              * Test we're still on the cpu as well as the version.
>> -              * We could have been migrated just after the first
>> -              * vgetcpu but before fetching the version, so we
>> -              * wouldn't notice a version change.
>> -              */
>> -             cpu1 = __getcpu() & VGETCPU_CPU_MASK;
>> -     } while (unlikely(cpu != cpu1 ||
>> -                       (pvti->pvti.version & 1) ||
>> -                       pvti->pvti.version != version));
>> -
>> -     if (unlikely(!(flags & PVCLOCK_TSC_STABLE_BIT)))
>> +
>> +     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>>               *mode = VCLOCK_NONE;
>> +             return 0;
>> +     }
>> +
>> +     do {
>> +             version = pvti->version;
>> +
>> +             /* This is also a read barrier, so we'll read version first. */
>> +             tsc = rdtsc_ordered();
>> +
>> +             pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
>> +             pvti_tsc_shift = pvti->tsc_shift;
>> +             pvti_system_time = pvti->system_time;
>> +             pvti_tsc = pvti->tsc_timestamp;
>> +
>> +             /* Make sure that the version double-check is last. */
>> +             smp_rmb();
>> +     } while (unlikely((version & 1) || version != pvti->version));
>
> Andy,
>
> What happens if PVCLOCK_TSC_STABLE_BIT is disabled here?

Do you mean what happens if it's disabled in the loop part after the
first check?  If that's actually possible, I'll do a follow-up to bail
if that happens by moving the check into the loop.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301175

On Mon, Jan 04, 2016 at 02:33:12PM -0800, Andy Lutomirski wrote:
> On Mon, Jan 4, 2016 at 12:26 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > On Sun, Dec 20, 2015 at 03:05:41AM -0800, Andy Lutomirski wrote:
> >> From: Andy Lutomirski <luto@amacapital.net>
> >>
> >> The pvclock vdso code was too abstracted to understand easily and
> >> excessively paranoid.  Simplify it for a huge speedup.
> >>
> >> This opens the door for additional simplifications, as the vdso no
> >> longer accesses the pvti for any vcpu other than vcpu 0.
> >>
> >> Before, vclock_gettime using kvm-clock took about 45ns on my machine.
> >> With this change, it takes 29ns, which is almost as fast as the pure TSC
> >> implementation.
> >>
> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >> ---
> >>  arch/x86/entry/vdso/vclock_gettime.c | 81 ++++++++++++++++++++----------------
> >>  1 file changed, 46 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
> >> index ca94fa649251..c325ba1bdddf 100644
> >> --- a/arch/x86/entry/vdso/vclock_gettime.c
> >> +++ b/arch/x86/entry/vdso/vclock_gettime.c
> >> @@ -78,47 +78,58 @@ static notrace const struct pvclock_vsyscall_time_info *get_pvti(int cpu)
> >>
> >>  static notrace cycle_t vread_pvclock(int *mode)
> >>  {
> >> -     const struct pvclock_vsyscall_time_info *pvti;
> >> +     const struct pvclock_vcpu_time_info *pvti = &get_pvti(0)->pvti;
> >>       cycle_t ret;
> >> -     u64 last;
> >> -     u32 version;
> >> -     u8 flags;
> >> -     unsigned cpu, cpu1;
> >> -
> >> +     u64 tsc, pvti_tsc;
> >> +     u64 last, delta, pvti_system_time;
> >> +     u32 version, pvti_tsc_to_system_mul, pvti_tsc_shift;
> >>
> >>       /*
> >> -      * Note: hypervisor must guarantee that:
> >> -      * 1. cpu ID number maps 1:1 to per-CPU pvclock time info.
> >> -      * 2. that per-CPU pvclock time info is updated if the
> >> -      *    underlying CPU changes.
> >> -      * 3. that version is increased whenever underlying CPU
> >> -      *    changes.
> >> +      * Note: The kernel and hypervisor must guarantee that cpu ID
> >> +      * number maps 1:1 to per-CPU pvclock time info.
> >> +      *
> >> +      * Because the hypervisor is entirely unaware of guest userspace
> >> +      * preemption, it cannot guarantee that per-CPU pvclock time
> >> +      * info is updated if the underlying CPU changes or that that
> >> +      * version is increased whenever underlying CPU changes.
> >>        *
> >> +      * On KVM, we are guaranteed that pvti updates for any vCPU are
> >> +      * atomic as seen by *all* vCPUs.  This is an even stronger
> >> +      * guarantee than we get with a normal seqlock.
> >> +      *
> >> +      * On Xen, we don't appear to have that guarantee, but Xen still
> >> +      * supplies a valid seqlock using the version field.
> >> +
> >> +      * We only do pvclock vdso timing at all if
> >> +      * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
> >> +      * mean that all vCPUs have matching pvti and that the TSC is
> >> +      * synced, so we can just look at vCPU 0's pvti.
> >>        */
> >> -     do {
> >> -             cpu = __getcpu() & VGETCPU_CPU_MASK;
> >> -             /* TODO: We can put vcpu id into higher bits of pvti.version.
> >> -              * This will save a couple of cycles by getting rid of
> >> -              * __getcpu() calls (Gleb).
> >> -              */
> >> -
> >> -             pvti = get_pvti(cpu);
> >> -
> >> -             version = __pvclock_read_cycles(&pvti->pvti, &ret, &flags);
> >> -
> >> -             /*
> >> -              * Test we're still on the cpu as well as the version.
> >> -              * We could have been migrated just after the first
> >> -              * vgetcpu but before fetching the version, so we
> >> -              * wouldn't notice a version change.
> >> -              */
> >> -             cpu1 = __getcpu() & VGETCPU_CPU_MASK;
> >> -     } while (unlikely(cpu != cpu1 ||
> >> -                       (pvti->pvti.version & 1) ||
> >> -                       pvti->pvti.version != version));
> >> -
> >> -     if (unlikely(!(flags & PVCLOCK_TSC_STABLE_BIT)))
> >> +
> >> +     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
> >>               *mode = VCLOCK_NONE;
> >> +             return 0;
> >> +     }
> >> +
> >> +     do {
> >> +             version = pvti->version;
> >> +
> >> +             /* This is also a read barrier, so we'll read version first. */
> >> +             tsc = rdtsc_ordered();
> >> +
> >> +             pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
> >> +             pvti_tsc_shift = pvti->tsc_shift;
> >> +             pvti_system_time = pvti->system_time;
> >> +             pvti_tsc = pvti->tsc_timestamp;
> >> +
> >> +             /* Make sure that the version double-check is last. */
> >> +             smp_rmb();
> >> +     } while (unlikely((version & 1) || version != pvti->version));
> >
> > Andy,
> >
> > What happens if PVCLOCK_TSC_STABLE_BIT is disabled here?
> 
> Do you mean what happens if it's disabled in the loop part after the
> first check?  If that's actually possible, I'll do a follow-up to bail
> if that happens by moving the check into the loop.
> 
> --Andy

It is possible.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301180 — [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

If the clock becomes unstable while we're reading it, we need to
bail.  We can do this by simply moving the check into the seqcount
loop.

Reported-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Marcelo, how's this?

arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
index 8602f06c759f..1a50e09c945b 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
 	 *
 	 * On Xen, we don't appear to have that guarantee, but Xen still
 	 * supplies a valid seqlock using the version field.
-
+	 *
 	 * We only do pvclock vdso timing at all if
 	 * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
 	 * mean that all vCPUs have matching pvti and that the TSC is
 	 * synced, so we can just look at vCPU 0's pvti.
 	 */
 
-	if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
-		*mode = VCLOCK_NONE;
-		return 0;
-	}
-
 	do {
 		version = pvti->version;
 
 		smp_rmb();
 
+		if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
+			*mode = VCLOCK_NONE;
+			return 0;
+		}
+
 		tsc = rdtsc_ordered();
 		pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
 		pvti_tsc_shift = pvti->tsc_shift;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303910 — Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

On Mon, Jan 04, 2016 at 03:14:28PM -0800, Andy Lutomirski wrote:
> If the clock becomes unstable while we're reading it, we need to
> bail.  We can do this by simply moving the check into the seqcount
> loop.
> 
> Reported-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Marcelo, how's this?
> 
> arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
> index 8602f06c759f..1a50e09c945b 100644
> --- a/arch/x86/entry/vdso/vclock_gettime.c
> +++ b/arch/x86/entry/vdso/vclock_gettime.c
> @@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
>  	 *
>  	 * On Xen, we don't appear to have that guarantee, but Xen still
>  	 * supplies a valid seqlock using the version field.
> -
> +	 *
>  	 * We only do pvclock vdso timing at all if
>  	 * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>  	 * mean that all vCPUs have matching pvti and that the TSC is
>  	 * synced, so we can just look at vCPU 0's pvti.
>  	 */
>  
> -	if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
> -		*mode = VCLOCK_NONE;
> -		return 0;
> -	}
> -
>  	do {
>  		version = pvti->version;
>  
>  		smp_rmb();
>  
> +		if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
> +			*mode = VCLOCK_NONE;
> +			return 0;
> +		}
> +
>  		tsc = rdtsc_ordered();
>  		pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
>  		pvti_tsc_shift = pvti->tsc_shift;
> -- 
> 2.4.3

Check it before returning the value (once cleared, it can't be set back 
to 1), similarly to what was in place before.

[toc] | [prev] | [next] | [standalone]

#1303917 — Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

On Thu, Jan 7, 2016 at 1:02 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Mon, Jan 04, 2016 at 03:14:28PM -0800, Andy Lutomirski wrote:
>> If the clock becomes unstable while we're reading it, we need to
>> bail.  We can do this by simply moving the check into the seqcount
>> loop.
>>
>> Reported-by: Marcelo Tosatti <mtosatti@redhat.com>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>
>> Marcelo, how's this?
>>
>> arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
>> index 8602f06c759f..1a50e09c945b 100644
>> --- a/arch/x86/entry/vdso/vclock_gettime.c
>> +++ b/arch/x86/entry/vdso/vclock_gettime.c
>> @@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
>>        *
>>        * On Xen, we don't appear to have that guarantee, but Xen still
>>        * supplies a valid seqlock using the version field.
>> -
>> +      *
>>        * We only do pvclock vdso timing at all if
>>        * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>>        * mean that all vCPUs have matching pvti and that the TSC is
>>        * synced, so we can just look at vCPU 0's pvti.
>>        */
>>
>> -     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> -             *mode = VCLOCK_NONE;
>> -             return 0;
>> -     }
>> -
>>       do {
>>               version = pvti->version;
>>
>>               smp_rmb();
>>
>> +             if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> +                     *mode = VCLOCK_NONE;
>> +                     return 0;
>> +             }
>> +
>>               tsc = rdtsc_ordered();
>>               pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
>>               pvti_tsc_shift = pvti->tsc_shift;
>> --
>> 2.4.3
>
> Check it before returning the value (once cleared, it can't be set back
> to 1), similarly to what was in place before.
>
>

I don't understand what you mean.

In the old code (4.3 and 4.4), the vdso checks STABLE_BIT at the end,
which is correct as long as STABLE_BIT can never change from 0 to 1.

In the -tip code, it's clearly wrong.

In the code in this patch, it should be correct regardless of how
STABLE_BIT changes as long as the seqcount works.  Given that the
performance cost of doing that is zero, I'd rather keep it that way.
If we're really paranoid, we could move it after the rest of the pvti
reads and add a barrier, but is there really any host on which that
matters?

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1303929 — Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

On 07/01/2016 22:13, Andy Lutomirski wrote:
> I don't understand what you mean.
> 
> In the old code (4.3 and 4.4), the vdso checks STABLE_BIT at the end,
> which is correct as long as STABLE_BIT can never change from 0 to 1.
> 
> In the -tip code, it's clearly wrong.
> 
> In the code in this patch, it should be correct regardless of how
> STABLE_BIT changes as long as the seqcount works.  Given that the
> performance cost of doing that is zero, I'd rather keep it that way.
> If we're really paranoid, we could move it after the rest of the pvti
> reads and add a barrier, but is there really any host on which that
> matters?

I agree that your patch is fine.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

[toc] | [prev] | [next] | [standalone]

#1304961 — Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

On Thu, Jan 07, 2016 at 01:13:41PM -0800, Andy Lutomirski wrote:
> On Thu, Jan 7, 2016 at 1:02 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > On Mon, Jan 04, 2016 at 03:14:28PM -0800, Andy Lutomirski wrote:
> >> If the clock becomes unstable while we're reading it, we need to
> >> bail.  We can do this by simply moving the check into the seqcount
> >> loop.
> >>
> >> Reported-by: Marcelo Tosatti <mtosatti@redhat.com>
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >> ---
> >>
> >> Marcelo, how's this?
> >>
> >> arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
> >> index 8602f06c759f..1a50e09c945b 100644
> >> --- a/arch/x86/entry/vdso/vclock_gettime.c
> >> +++ b/arch/x86/entry/vdso/vclock_gettime.c
> >> @@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
> >>        *
> >>        * On Xen, we don't appear to have that guarantee, but Xen still
> >>        * supplies a valid seqlock using the version field.
> >> -
> >> +      *
> >>        * We only do pvclock vdso timing at all if
> >>        * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
> >>        * mean that all vCPUs have matching pvti and that the TSC is
> >>        * synced, so we can just look at vCPU 0's pvti.
> >>        */
> >>
> >> -     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
> >> -             *mode = VCLOCK_NONE;
> >> -             return 0;
> >> -     }
> >> -
> >>       do {
> >>               version = pvti->version;
> >>
> >>               smp_rmb();
> >>
> >> +             if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
> >> +                     *mode = VCLOCK_NONE;
> >> +                     return 0;
> >> +             }
> >> +
> >>               tsc = rdtsc_ordered();
> >>               pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
> >>               pvti_tsc_shift = pvti->tsc_shift;
> >> --
> >> 2.4.3
> >
> > Check it before returning the value (once cleared, it can't be set back
> > to 1), similarly to what was in place before.
> >
> >
> 
> I don't understand what you mean.
> 
> In the old code (4.3 and 4.4), the vdso checks STABLE_BIT at the end,
> which is correct as long as STABLE_BIT can never change from 0 to 1.
> 
> In the -tip code, it's clearly wrong.
> 
> In the code in this patch, it should be correct regardless of how
> STABLE_BIT changes as long as the seqcount works.  Given that the
> performance cost of doing that is zero, I'd rather keep it that way.
> If we're really paranoid, we could move it after the rest of the pvti
> reads and add a barrier, but is there really any host on which that
> matters?
> 
> --Andy
> 
> -- 
> Andy Lutomirski
> AMA Capital Management, LLC

Right, its OK due to version check, thanks.

[toc] | [prev] | [next] | [standalone]

#1307778 — Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

Hi Ingo-

Can you apply this before the tip:x86/asm pull request goes out?  It
fixes a regression in tip:x86/asm.

--Andy

On Fri, Jan 8, 2016 at 6:04 AM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Thu, Jan 07, 2016 at 01:13:41PM -0800, Andy Lutomirski wrote:
>> On Thu, Jan 7, 2016 at 1:02 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>> > On Mon, Jan 04, 2016 at 03:14:28PM -0800, Andy Lutomirski wrote:
>> >> If the clock becomes unstable while we're reading it, we need to
>> >> bail.  We can do this by simply moving the check into the seqcount
>> >> loop.
>> >>
>> >> Reported-by: Marcelo Tosatti <mtosatti@redhat.com>
>> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> >> ---
>> >>
>> >> Marcelo, how's this?
>> >>
>> >> arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
>> >>  1 file changed, 6 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
>> >> index 8602f06c759f..1a50e09c945b 100644
>> >> --- a/arch/x86/entry/vdso/vclock_gettime.c
>> >> +++ b/arch/x86/entry/vdso/vclock_gettime.c
>> >> @@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
>> >>        *
>> >>        * On Xen, we don't appear to have that guarantee, but Xen still
>> >>        * supplies a valid seqlock using the version field.
>> >> -
>> >> +      *
>> >>        * We only do pvclock vdso timing at all if
>> >>        * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>> >>        * mean that all vCPUs have matching pvti and that the TSC is
>> >>        * synced, so we can just look at vCPU 0's pvti.
>> >>        */
>> >>
>> >> -     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> >> -             *mode = VCLOCK_NONE;
>> >> -             return 0;
>> >> -     }
>> >> -
>> >>       do {
>> >>               version = pvti->version;
>> >>
>> >>               smp_rmb();
>> >>
>> >> +             if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> >> +                     *mode = VCLOCK_NONE;
>> >> +                     return 0;
>> >> +             }
>> >> +
>> >>               tsc = rdtsc_ordered();
>> >>               pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
>> >>               pvti_tsc_shift = pvti->tsc_shift;
>> >> --
>> >> 2.4.3
>> >
>> > Check it before returning the value (once cleared, it can't be set back
>> > to 1), similarly to what was in place before.
>> >
>> >
>>
>> I don't understand what you mean.
>>
>> In the old code (4.3 and 4.4), the vdso checks STABLE_BIT at the end,
>> which is correct as long as STABLE_BIT can never change from 0 to 1.
>>
>> In the -tip code, it's clearly wrong.
>>
>> In the code in this patch, it should be correct regardless of how
>> STABLE_BIT changes as long as the seqcount works.  Given that the
>> performance cost of doing that is zero, I'd rather keep it that way.
>> If we're really paranoid, we could move it after the rest of the pvti
>> reads and add a barrier, but is there really any host on which that
>> matters?
>>
>> --Andy
>>
>> --
>> Andy Lutomirski
>> AMA Capital Management, LLC
>
> Right, its OK due to version check, thanks.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1308278 — Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

* Andy Lutomirski <luto@amacapital.net> wrote:

> Hi Ingo-
> 
> Can you apply this before the tip:x86/asm pull request goes out?  It
> fixes a regression in tip:x86/asm.

Ooops, saw this mail too late - I'll merge this up into x86/urgent right now and 
send all pending fixes to Linus.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1309099 — [tip:x86/urgent] x86/vdso/pvclock: Protect STABLE check with the seqcount

Commit-ID:  78fd8c7288e0a4bba3ad1d69caf9396a6b69cb00
Gitweb:     http://git.kernel.org/tip/78fd8c7288e0a4bba3ad1d69caf9396a6b69cb00
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Mon, 4 Jan 2016 15:14:28 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 13 Jan 2016 11:46:29 +0100

x86/vdso/pvclock: Protect STABLE check with the seqcount

If the clock becomes unstable while we're reading it, we need to
bail.  We can do this by simply moving the check into the
seqcount loop.

Reported-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Alexander Graf <agraf@suse.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Radim Krcmar <rkrcmar@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/755dcedb17269e1d7ce12a9a713dea303835137e.1451949191.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
index 8602f06..1a50e09 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
 	 *
 	 * On Xen, we don't appear to have that guarantee, but Xen still
 	 * supplies a valid seqlock using the version field.
-
+	 *
 	 * We only do pvclock vdso timing at all if
 	 * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
 	 * mean that all vCPUs have matching pvti and that the TSC is
 	 * synced, so we can just look at vCPU 0's pvti.
 	 */
 
-	if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
-		*mode = VCLOCK_NONE;
-		return 0;
-	}
-
 	do {
 		version = pvti->version;
 
 		smp_rmb();
 
+		if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
+			*mode = VCLOCK_NONE;
+			return 0;
+		}
+
 		tsc = rdtsc_ordered();
 		pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
 		pvti_tsc_shift = pvti->tsc_shift;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web