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

Groups > linux.kernel > #1717793 > unrolled thread

Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of pvclock_gtod_copy in KVM

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 v4 00/10] make L2's kvm-clock stable, get rid of  pvclock_gtod_copy in KVM John Stultz <john.stultz@linaro.org> - 2017-08-22 22:00 +0200
    Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of  pvclock_gtod_copy in KVM Paolo Bonzini <pbonzini@redhat.com> - 2017-08-22 23:10 +0200
      Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of  pvclock_gtod_copy in KVM Thomas Gleixner <tglx@linutronix.de> - 2017-08-23 14:50 +0200
        Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of  pvclock_gtod_copy in KVM Paolo Bonzini <pbonzini@redhat.com> - 2017-08-23 18:10 +0200
          Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of  pvclock_gtod_copy in KVM Paolo Bonzini <pbonzini@redhat.com> - 2017-08-24 10:10 +0200

#1717793 — Re: [PATCH v4 00/10] make L2's kvm-clock stable, get rid of pvclock_gtod_copy in KVM

On Mon, Aug 21, 2017 at 1:40 AM, Denis Plotnikov
<dplotnikov@virtuozzo.com> wrote:
> ping!
>

I still don't feel my questions have been well answered. Its really
not clear to me why, in order to allow the level-2 guest to use a vdso
that the answer is to export more data through the entire stack rather
then to make the kvmclock to be usable from the vsyscall.

So far for a problem statement, all I've got is:
"However, when using nested virtualization you have

        L0: bare-metal hypervisor (uses TSC)
        L1: nested hypervisor (uses kvmclock, can use vsyscall)
        L2: nested guest

and L2 cannot use vsyscall because it is not using the TSC."

Which is a start but doesn't really make it clear why the proposed
solution is best/necessary.

thanks
-john

[toc] | [next] | [standalone]

#1717840

> I still don't feel my questions have been well answered. Its really
> not clear to me why, in order to allow the level-2 guest to use a vdso
> that the answer is to export more data through the entire stack rather
> then to make the kvmclock to be usable from the vsyscall.

Thanks, this helps.

A stable kvmclock is already usable from the vsyscall.  It is however not
yet usable _in the hypervisor_ as a way to provide another stable kvmclock
to the nested guest; right now the only clocksource that a hypervisor can
use to provide a stable kvmclock is the TSC.

So, regarding the "why is it necessary" part.  Even on a modern host with
invariant TSC, kvmclock mediates between TSC and the guest and provides for
example support for live migration, where the TSC frequency may be
different between source and destination.   If the L1 hypervisor could
use the TSC to provide a stable kvmclock, there would be no need for kvmclock
in the first place.  The paravirtualized clock may well disappear in a few
years since Skylake provides TSC scaling.  However, I'm not that optimistic
because people are complaining that I removed support for 2007 processors
and it seems that I'll have to put it back.  So, as more people use nested
virtualization (and we have nested virt migration in the works, too), nested
kvmclock becomes more important too.

Regarding the "why is it best" part.  Right now, the hypervisor makes a
copy of the timekeeper information in order to prepare the stable kvmclock.
This code is very much tied to the TSC.  However, a snapshot of the timekeeper
information is almost entirely the same thing that ktime_get_snapshot returns,
so my suggestion to "untie" the hypervisor code from the TSC was to use
ktime_get_snapshot instead.  This way, the clocksource itself tells KVM
whether it can be the base for a vsyscall-happy kvmclock (which means, it
must be the TSC or a linear transformation of it).

While I am very happy with how the KVM code comes out, it might certainly
be not the best solution---I definitely need help from the clocksource
maintainers here, not just approval!  In particular, it doesn't help that
a lot of code surrounding ktime_get_snapshot is unused, so that may have
sent me off track.

In particular, the return value of the new callback can be defined as "is
it the TSC or a linear transformation of it".  But that's as good a definition
as "is it good for KVM" (i.e., not very good) without some documentation on
the meaning of "cycles" in the struct returned by ktime_get_snapshot. Once I
understand that, I hope I can provide a better explanation for the return
value of the callback.

Paolo

> So far for a problem statement, all I've got is:
> "However, when using nested virtualization you have
> 
>         L0: bare-metal hypervisor (uses TSC)
>         L1: nested hypervisor (uses kvmclock, can use vsyscall)
>         L2: nested guest
> 
> and L2 cannot use vsyscall because it is not using the TSC."
> 
> Which is a start but doesn't really make it clear why the proposed
> solution is best/necessary.
> 
> thanks
> -john
> 

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

#1718329

On Tue, 22 Aug 2017, Paolo Bonzini wrote:
> Regarding the "why is it best" part.  Right now, the hypervisor makes a
> copy of the timekeeper information in order to prepare the stable kvmclock.
> This code is very much tied to the TSC.  However, a snapshot of the timekeeper
> information is almost entirely the same thing that ktime_get_snapshot returns,
> so my suggestion to "untie" the hypervisor code from the TSC was to use
> ktime_get_snapshot instead.  This way, the clocksource itself tells KVM
> whether it can be the base for a vsyscall-happy kvmclock (which means, it
> must be the TSC or a linear transformation of it).
>
> While I am very happy with how the KVM code comes out, it might certainly
> be not the best solution---I definitely need help from the clocksource
> maintainers here, not just approval!  In particular, it doesn't help that
> a lot of code surrounding ktime_get_snapshot is unused, so that may have
> sent me off track.
> 
> In particular, the return value of the new callback can be defined as "is
> it the TSC or a linear transformation of it".  But that's as good a
> definition as "is it good for KVM" (i.e., not very good) without some
> documentation on the meaning of "cycles" in the struct returned by
> ktime_get_snapshot. Once I understand that, I hope I can provide a better
> explanation for the return value of the callback.

This all looks wrong to begin with and you are just bolting and duct taping
stuff together as you see fit.

I understand the idea of providing a clocksource to the core code which
provides direct nano second resolution and do the host -> guest conversion
in the kvmclock implementation. But that's the root cause of all evils.

The reason why you do that is to support live migration of VMs to hosts
with a different TSC frequency. And for exactly that particular corner case
the whole conversion magic is implemented and now you need even more duct
tape to make it work with nested VMs.

That's just wrong. We need to sit back and look at that whole kvm clock
mechanism and redesign it proper.

Let's look at the goals:

   1) Provide the information of host frequency to the guest

   2) Propagate host frequency changes to the guest

      That's solely used for migration purposes.

      It's not there for dealing with non constant frequency TSCs, which
      would be beyond insane.

      Neither to runtime propagate host clocksource adjustments (via NTP &
      friends) to a guest. If you use it that way today, then this needs to
      be fixed, simply because that's the wrong approach. We have proper
      correction mechanisms (NTP,PPS,PTP ...) which can be utilized to do
      so. Aside of that it'd be interesting to see the interaction between
      the host frequency scaling change and NTP in the guest trying to
      adjust as well.

Now lets look at a simple ktime_get() with the current implementation:

   ktime_get()
     do {
	seq = seqcount_begin(tk);
	data = tk->data;
	now = tk->clock->read(tk->clock);
	  kvmclock_read()
            do {
	       seqk = seqcount_begin(kvmclock);
	       nowk = kvmclock_read();
	       datak = kvmclock_data;
	    } while (seqcount_retry(seqk, kvmclock));
	    nsec = convert(nowk, datak);
	    return nsec;
     } while (seqcount_retry(seq, tk));

     nsec = convert(now, data);
     return nsec;

So you need two sequence counters and two conversions for reading the
clock. Sorry, but that's just crap.

Let's look at the normal usecase (no migration) first:

  The TSC frequency can be retrieved at guest boot time and does not ever
  change. For this case the above is bloat and completely useless.

  All you need to do is to register the kvm clocksource with the proper
  frequency and the readout is just the plain TSC read. Nothing
  else.

So now the special case of live migration. You need to make sure that the
change of the TSC frequency is properly propagated and any reader which is
in the middle of a time getter function will retry with the new parameters.

That's not any different from the case where the timekeeper is adjusted by
any of the regular mechanisms: update_wall_time(), ntp, pps, ptp ....

The only thing you need to ensure is that the timekeeping core is properly
informed about the change and code which executes time getter functions
will retry. The core has _ALL_ mechanisms available for that.

So the real question is how to ensure that:

  1) None of the update functions is in progress

  2) The update is propagated via the existing mechanisms

The whole live migration magic is orchestrated by qemu and the kernel. So
it's reasonably simple to ensure that.

Something like the below should work for this:

	Host				Guest
1:	prevent_nmi_delivery_to_guest()
2:	inject_kvmclock_irq()
3:					handle_kvmclock_irq()
4:					  timekeeping_freeze()
5:					  exit_vm()
6:	stop_and_migrate_guest()
7:	update_guest_data()
8:	resume_guest()
9:					  timekeeping_reconfigure();
10:					  timekeeping_unfreeze();
11:	resume_nmi_delivery_to_guest()

#1  Ensures that no NMI is delivered to the guest so that the timekeeper
    core does not have to worry about NMI context calling any of the NMI
    safe time getters.

    If that's not possible then it's simple enough to do something about
    the NMI safe time getters in the time keeping core code, so you don't
    have to worry much about it.

#2  Inject a special interrupt vector which initiates the timekeeping
    freeze mechanism in the guest

#3  kvm clock interrupt handler gets invoked

#4  Ensures that:

    - All potential timekeeping update mechanisms have left the critical
      region

    - All potential time getters are blocked

    The mechanism for that is simple enough:

    timekeeping_freeze()
    {
    	raw_spin_lock(&timekeeper_lock);
	write_seqcount_begin(&tk_core.seq);
	/* Ensure a consistent state */
	timekeeping_forward_now(tk);
	timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);	
    }

#5  Exit the VM with some magic exit reason so the host side knows that
    timekeeping is frozen

#6  Stop the VM, migrate it

#9  Retrieve the new conversion factor and adjust the timekeeper data
    accordingly

#10 Unfreeze the time keeper with the new configuration

    timekeeping_freeze()
    {
	/* Ensure a consistent state */
	timekeeping_forward_now(tk);
	timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);	
	
	write_seqcount_end(&tk_core.seq);
    	raw_spin_unlock(&timekeeper_lock);
    }

#11 Resume NMI delivery

    As I said in #1 this can be completely handled in the timekeeper core,
    but if we can avoid that then stuff becomes simpler. And simpler is
    preferred ....


So now for the nested KVM case. If you follow the above scheme then this
becomes really simple:

  1) The TSC frequency is merily propagated to the L2 guest. It's the
     same as the L1 guest TSC frequency. No magic voodoo required.

  2) Migration of a L2 guest to a different L1 guest follows the
     same scheme as above

  3) Migration of a L2 guest to a physcial host follows the same scheme as
     above - no idea whether that's supported at all

  4) Migration of a L1 guest with a embedded L2 guest is not rocket science
     either. The above needs some extra code which propagates the time
     keeper freeze to L2 and then when L1 resumes, the updated frequency
     data is propagated to L2 and L2 resumed along with it.

  You don't need any timestamp snapshot magic and voodoo callbacks. All you
  need is a proper mechanism to update the timekeeper.

I might be missing something really important as usual. If so, I'm happy to
be educated.

Thanks,

	tglx

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

#1718485

On 23/08/2017 14:45, Thomas Gleixner wrote:
> So the real question is how to ensure that:
> 
>   1) None of the update functions is in progress
> 
>   2) The update is propagated via the existing mechanisms
> 
> The whole live migration magic is orchestrated by qemu and the kernel. So
> it's reasonably simple to ensure that.

There is no orchestration whatsoever between QEMU and the host kernel,
much less between anything and the guest.  QEMU just sends ioctls.  The
complex part is above QEMU, just because you have to make sure that the
destination sets up networking and everything else just like the source,
but as far as QEMU is concerned live migration is as simple as

  stop_guest()
  get_guest_state()
  write_data_to_socket()

                            read_data_from_socket()
                            set_guest_state()
                            resume_guest()

and as far as KVM is concerned, it's as simple as

  get a signal, KVM_RUN exits
  KVM_GET_REGS and a bunch more ioctls

                            KVM_SET_REGS and a bunch more ioctls
                              KVM_SET_KVMCLOCK triggers kvmclock update
                            ioctl(KVM_RUN)
                              process KVM_REQ_CLOCK_UPDATE
                              enter the guest

Adding interrupts and all that is much, much more complicated than just
reusing code that runs all the time (albeit only on hosts with impaired
TSC) and needs no special case at all.

> The reason why you do that is to support live migration of VMs to hosts
> with a different TSC frequency. And for exactly that particular corner case
> the whole conversion magic is implemented and now you need even more duct
> tape to make it work with nested VMs.

The point of the first part of this series is to _remove_ the duct tape
and actually make KVM use generic timekeeper services, namely
ktime_get_snapshot.  If you look at patches 1-2-3-4-5-7 the delta is
-120 lines of code, without any nested virtualization stuff.

More duct tape would have been just:

-	if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
+	mode = READ_ONCE(pvclock_gtod_data.clock.vclock_mode);
+	if (mode != VCLOCK_TSC &&
+	    (mode != VCLOCK_PVCLOCK || !pvclock_nested_virt_magic())
		return false;

-	return do_realtime(ts, cycle_now) == VCLOCK_TSC;
+	switch (mode) {
+	case VCLOCK_TSC:
+		return do_realtime_tsc(ts, cycle_now);
+	case VCLOCK_PVCLOCK:
+		return do_realtime_pvclock(ts, cycle_now);
+	}

Nested virtualization does need a clocksource change notifier on top,
but we can cross that bridge later.  Maybe Denis can post just those
patches to begin with.

Paolo

> So now for the nested KVM case. If you follow the above scheme then this
> becomes really simple:
> 
>   1) The TSC frequency is merily propagated to the L2 guest. It's the
>      same as the L1 guest TSC frequency. No magic voodoo required.
> 
>   2) Migration of a L2 guest to a different L1 guest follows the
>      same scheme as above
> 
>   3) Migration of a L2 guest to a physcial host follows the same scheme as
>      above - no idea whether that's supported at all
> 
>   4) Migration of a L1 guest with a embedded L2 guest is not rocket science
>      either. The above needs some extra code which propagates the time
>      keeper freeze to L2 and then when L1 resumes, the updated frequency
>      data is propagated to L2 and L2 resumed along with it.
> 
>   You don't need any timestamp snapshot magic and voodoo callbacks. All you
>   need is a proper mechanism to update the timekeeper.
> 
> I might be missing something really important as usual. If so, I'm happy to
> be educated.

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

#1718943

On 23/08/2017 18:02, Paolo Bonzini wrote:
> 
> More duct tape would have been just:
> 
> -	if (pvclock_gtod_data.clock.vclock_mode != VCLOCK_TSC)
> +	mode = READ_ONCE(pvclock_gtod_data.clock.vclock_mode);
> +	if (mode != VCLOCK_TSC &&
> +	    (mode != VCLOCK_PVCLOCK || !pvclock_nested_virt_magic())
> 		return false;
> 
> -	return do_realtime(ts, cycle_now) == VCLOCK_TSC;
> +	switch (mode) {
> +	case VCLOCK_TSC:
> +		return do_realtime_tsc(ts, cycle_now);
> +	case VCLOCK_PVCLOCK:
> +		return do_realtime_pvclock(ts, cycle_now);
> +	}
> 
> Nested virtualization does need a clocksource change notifier on top,
> but we can cross that bridge later.  Maybe Denis can post just those
> patches to begin with.

For what it's worth, this is all that's needed (with patches 1-2-3-4-5-7)
to support kvmclock on top of Hyper-V clock.  It's trivial.

Even if we could add paravirtualization magic to KVM live migration, we
certainly couldn't do that for other hypervisors.

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 5b882cc0c0e9..3bab935b021a 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -46,10 +46,24 @@ static u64 read_hv_clock_tsc(struct clocksource *arg)
 	return current_tick;
 }
 
+static bool read_hv_clock_tsc_with_stamp(struct clocksource *arg,
+					 u64 *cycles, u64 *cycles_stamp)
+{
+	*cycles = __hv_read_tsc_page(tsc_pg, &cycles_stamp);
+
+	if (*cycles == U64_MAX) {
+		*cycles = rdmsrl(HV_X64_MSR_TIME_REF_COUNT);
+		return false;
+	}
+
+	return true;
+}
+
 static struct clocksource hyperv_cs_tsc = {
 		.name		= "hyperv_clocksource_tsc_page",
 		.rating		= 400,
 		.read		= read_hv_clock_tsc,
+		.read_with_stamp = read_hv_clock_tsc_with_stamp,
 		.mask		= CLOCKSOURCE_MASK(64),
 		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 };
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 2b58c8c1eeaa..5aff66e9fff7 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -176,9 +176,9 @@ void hyperv_cleanup(void);
 #endif
 #ifdef CONFIG_HYPERV_TSCPAGE
 struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
-static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
+static inline u64 __hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
 {
-	u64 scale, offset, cur_tsc;
+	u64 scale, offset;
 	u32 sequence;
 
 	/*
@@ -209,7 +209,7 @@ static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
 
 		scale = READ_ONCE(tsc_pg->tsc_scale);
 		offset = READ_ONCE(tsc_pg->tsc_offset);
-		cur_tsc = rdtsc_ordered();
+		*cur_tsc = rdtsc_ordered();
 
 		/*
 		 * Make sure we read sequence after we read all other values
@@ -219,9 +219,14 @@ static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
 
 	} while (READ_ONCE(tsc_pg->tsc_sequence) != sequence);
 
-	return mul_u64_u64_shr(cur_tsc, scale, 64) + offset;
+	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
 }
 
+static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
+{
+	u64 cur_tsc;
+	return __hv_read_tsc_page(tsc_pg, &cur_tsc);
+}
 #else
 static inline struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
 {


Denis, could you try redoing patch 7 to use the pvclock_gtod_notifier
instead of the new one you're adding, and only send that first part?  I
think it's a worthwhile cleanup anyway, so let's start with that.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web