x86: KVM vdso and clock improvements

From: Andy Lutomirski <[email protected]>

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/vdso/vclock_gettime.c | 81 ++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 35 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +31 -35

index ca94fa649251..c325ba1bdddf 100644
@@ -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. */

arch/x86/entry/vdso/vclock_gettime.c +5 -0

+
+	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;

Signature · 6 lines

2.5.0

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 98 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/vdso/vclock_gettime.c | 81 ++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 35 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +31 -35

index ca94fa649251..c325ba1bdddf 100644
@@ -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. */

arch/x86/entry/vdso/vclock_gettime.c static notrace const struct pvclock_vsyscall_time_info *get_pvti(int cpu) · line 127

Quoted 10 lines

arch/x86/entry/vdso/vclock_gettime.c +10 -0

+             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?

Quoted 112 lines · Marcelo Tosatti

Quoted 98 lines · Andy Lutomirski

From: Andy Lutomirski <[email protected]>

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/vdso/vclock_gettime.c | 81 ++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 35 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +31 -35

index ca94fa649251..c325ba1bdddf 100644
@@ -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. */

arch/x86/entry/vdso/vclock_gettime.c static notrace const struct pvclock_vsyscall_time_info *get_pvti(int cpu) · line 127

Quoted 10 lines

arch/x86/entry/vdso/vclock_gettime.c +10 -0

+             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,

Quoted 1 line

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

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

Marcelo, how's this?

arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------

 1 file changed, 6 insertions(+), 6 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +6 -6

index 8602f06c759f..1a50e09c945b 100644
@@ -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;

Signature · 1 line

2.4.3

Quoted 39 lines · Andy Lutomirski

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

Marcelo, how's this?

arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------

 1 file changed, 6 insertions(+), 6 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +1 -6

index 8602f06c759f..1a50e09c945b 100644
@@ -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();
 

Quoted 10 lines

arch/x86/entry/vdso/vclock_gettime.c +5 -1

+             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?

Quoted 40 lines · Marcelo Tosatti

Quoted 39 lines · Andy Lutomirski

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

Marcelo, how's this?

arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------

 1 file changed, 6 insertions(+), 6 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +1 -6

index 8602f06c759f..1a50e09c945b 100644
@@ -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();
 

Quoted 15 lines

Quoted 10 lines

arch/x86/entry/vdso/vclock_gettime.c +5 -1

+             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

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 71 lines · Andy Lutomirski

Quoted 40 lines · Marcelo Tosatti

Quoted 39 lines · Andy Lutomirski

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 <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

Marcelo, how's this?

arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------

 1 file changed, 6 insertions(+), 6 deletions(-)

arch/x86/entry/vdso/vclock_gettime.c +1 -6

index 8602f06c759f..1a50e09c945b 100644
@@ -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();
 

Quoted 15 lines

Quoted 10 lines

arch/x86/entry/vdso/vclock_gettime.c +5 -1

+             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?

Quoted 5 lines

--Andy

--
Andy Lutomirski
AMA Capital Management, LLC

Right, its OK due to version check, thanks.

Hi Ingo-

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