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

Groups > linux.kernel > #1702179 > unrolled thread

[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

Contents

  [PATCH v4 00/10] make L2's kvm-clock stable, get rid of pvclock_gtod_copy in KVM Denis Plotnikov <dplotnikov@virtuozzo.com> - 2017-08-02 17:00 +0200
    [PATCH v4 10/10] kvmclock: implement the extended reading function Denis Plotnikov <dplotnikov@virtuozzo.com> - 2017-08-02 17:00 +0200
    [PATCH v4 07/10] KVM: x86: remove not used pvclock_gtod_copy Denis Plotnikov <dplotnikov@virtuozzo.com> - 2017-08-02 17:00 +0200
      Re: [PATCH v4 07/10] KVM: x86: remove not used pvclock_gtod_copy Marcelo Tosatti <mtosatti@redhat.com> - 2017-08-03 01:30 +0200
        Re: [PATCH v4 07/10] KVM: x86: remove not used pvclock_gtod_copy Paolo Bonzini <pbonzini@redhat.com> - 2017-08-03 14:40 +0200
    [PATCH v4 09/10] pvclock: add clocksource change notification on changing of tsc stable bit Denis Plotnikov <dplotnikov@virtuozzo.com> - 2017-08-02 17:00 +0200
      Re: [PATCH v4 09/10] pvclock: add clocksource change notification on  changing of tsc stable bit Marcelo Tosatti <mtosatti@redhat.com> - 2017-08-03 01:40 +0200
    [PATCH v4 06/10] timekeeper: add clocksource change notifier Denis Plotnikov <dplotnikov@virtuozzo.com> - 2017-08-02 17: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-02 18:20 +0200
    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-02 18: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-02 19:20 +0200

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

V4:
  * removed "is stable" function with vague definition of stability
    there is the only function which does time with cycle stamp getting
  * some variables renamed
  * some patches split into smaller once
  * atomic64_t usage is replaced with atomic_t
 
V3:
  Changing the timekeeper interface for clocksource reading looks like
  an overkill to achive the goal of getting cycles stamp for KVM.
  Instead extend the timekeeping interface and add functions which provide
  necessary data: read clocksource with cycles stamp, check whether the
  clock source is stable.

  Use those functions and improve existing timekeeper functionality to
  replace pvclock_gtod_copy scheme in masterclock data calculation.

V2:
  The main goal is to make L2 kvm-clock be stable when it's running over L1
  with stable kvm-clock.

  The patch series is for x86 architecture only. If the series is approved
  I'll do changes for other architectures but I don't have an ability to
  compile and check for every single on (help needed)

  The patch series do the following:

        * change timekeeper interface to get cycles stamp value from
          the timekeeper
        * get rid of pvclock copy in KVM by using the changed timekeeper
          interface: get time and cycles right from the timekeeper
        * make KVM recognize a stable kvm-clock as stable clocksource
          and use the KVM masterclock in this case, which means making
          L2 stable when running over stable L1 kvm-clock

Denis Plotnikov (10):
  timekeeper: introduce extended clocksource reading callback
  timekeeper: introduce boot field in system_time_snapshot
  timekeeper: use the extended reading function on snapshot acquiring
  tsc: implement the extended tsc reading function
  KVM: x86: switch to masterclock update using timekeeper functionality
  timekeeper: add clocksource change notifier
  KVM: x86: remove not used pvclock_gtod_copy
  pvclock: add parameters to store stamp data in pvclock reading
    function
  pvclock: add clocksource change notification on changing of tsc stable
    bit
  kvmclock: implement the extended reading function

 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/include/asm/pvclock.h  |   3 +-
 arch/x86/kernel/kvmclock.c      |  19 +++-
 arch/x86/kernel/pvclock.c       |  37 +++++-
 arch/x86/kernel/tsc.c           |  10 ++
 arch/x86/kvm/trace.h            |  31 ++---
 arch/x86/kvm/x86.c              | 242 ++++++++--------------------------------
 arch/x86/xen/time.c             |   2 +-
 include/linux/clocksource.h     |  11 +-
 include/linux/cs_notifier.h     |  17 +++
 include/linux/timekeeping.h     |   5 +
 kernel/time/timekeeping.c       |  68 ++++++++++-
 12 files changed, 219 insertions(+), 228 deletions(-)
 create mode 100644 include/linux/cs_notifier.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1702180 — [PATCH v4 10/10] kvmclock: implement the extended reading function

This allows L2 guests to use masterclock, namely
provide KVM with ability to use masterclock while
running over kvmclock closksource in the cases when
it's possible.

This is the final part of the work of teaching KVM
to use masterclock when over kvmclock clocksource.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 arch/x86/kernel/kvmclock.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index f692579..8c1008f 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -82,7 +82,7 @@ static int kvm_set_wallclock(const struct timespec *now)
 	return -1;
 }
 
-static u64 kvm_clock_read(void)
+static inline u64 __kvm_clock_read(u64 *cycles, u8 *flags)
 {
 	struct pvclock_vcpu_time_info *src;
 	u64 ret;
@@ -91,10 +91,14 @@ static u64 kvm_clock_read(void)
 	preempt_disable_notrace();
 	cpu = smp_processor_id();
 	src = &hv_clock[cpu].pvti;
-	ret = pvclock_clocksource_read(src, NULL, NULL);
+	ret = pvclock_clocksource_read(src, cycles, flags);
 	preempt_enable_notrace();
 	return ret;
 }
+static u64 kvm_clock_read(void)
+{
+	return __kvm_clock_read(NULL, NULL);
+}
 
 static u64 kvm_clock_get_cycles(struct clocksource *cs)
 {
@@ -177,9 +181,20 @@ bool kvm_check_and_clear_guest_paused(void)
 	return ret;
 }
 
+static bool kvm_clock_read_with_stamp(struct clocksource *cs,
+					u64 *cycles, u64 *cycles_stamp)
+{
+	u8 flags;
+
+	*cycles = __kvm_clock_read(cycles_stamp, &flags);
+
+	return (bool) flags & PVCLOCK_TSC_STABLE_BIT;
+}
+
 struct clocksource kvm_clock = {
 	.name = "kvm-clock",
 	.read = kvm_clock_get_cycles,
+	.read_with_stamp = kvm_clock_read_with_stamp,
 	.rating = 400,
 	.mask = CLOCKSOURCE_MASK(64),
 	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
-- 
2.7.4

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

#1702182 — [PATCH v4 07/10] KVM: x86: remove not used pvclock_gtod_copy

Since, KVM has been switched to getting masterclock related data
right from the timekeeper by the previous patches, now we are able
to remove all the parts related to the old scheme of getting
masterclock data.

This patch removes those parts.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/trace.h            |  31 ++----
 arch/x86/kvm/x86.c              | 216 ++++++----------------------------------
 3 files changed, 42 insertions(+), 207 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 87ac4fb..91465db 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -791,7 +791,7 @@ struct kvm_arch {
 	u64 cur_tsc_generation;
 	int nr_vcpus_matched_tsc;
 
-	spinlock_t pvclock_gtod_sync_lock;
+	spinlock_t masterclock_lock;
 	bool use_master_clock;
 	u64 master_kernel_ns;
 	u64 master_cycle_now;
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 0a6cc67..923ab31 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -807,45 +807,39 @@ TRACE_EVENT(kvm_write_tsc_offset,
 
 #ifdef CONFIG_X86_64
 
-#define host_clocks					\
-	{VCLOCK_NONE, "none"},				\
-	{VCLOCK_TSC,  "tsc"}				\
-
 TRACE_EVENT(kvm_update_master_clock,
-	TP_PROTO(bool use_master_clock, unsigned int host_clock, bool offset_matched),
-	TP_ARGS(use_master_clock, host_clock, offset_matched),
+	TP_PROTO(bool use_master_clock, bool host_clock_stable,
+		bool offset_matched),
+	TP_ARGS(use_master_clock, host_clock_stable, offset_matched),
 
 	TP_STRUCT__entry(
 		__field(		bool,	use_master_clock	)
-		__field(	unsigned int,	host_clock		)
+		__field(		bool,	host_clock_stable	)
 		__field(		bool,	offset_matched		)
 	),
 
 	TP_fast_assign(
 		__entry->use_master_clock	= use_master_clock;
-		__entry->host_clock		= host_clock;
+		__entry->host_clock_stable	= host_clock_stable;
 		__entry->offset_matched		= offset_matched;
 	),
 
-	TP_printk("masterclock %d hostclock %s offsetmatched %u",
+	TP_printk("masterclock %d hostclock stable %u offsetmatched %u",
 		  __entry->use_master_clock,
-		  __print_symbolic(__entry->host_clock, host_clocks),
+		  __entry->host_clock_stable,
 		  __entry->offset_matched)
 );
 
 TRACE_EVENT(kvm_track_tsc,
 	TP_PROTO(unsigned int vcpu_id, unsigned int nr_matched,
-		 unsigned int online_vcpus, bool use_master_clock,
-		 unsigned int host_clock),
-	TP_ARGS(vcpu_id, nr_matched, online_vcpus, use_master_clock,
-		host_clock),
+		 unsigned int online_vcpus, bool use_master_clock),
+	TP_ARGS(vcpu_id, nr_matched, online_vcpus, use_master_clock),
 
 	TP_STRUCT__entry(
 		__field(	unsigned int,	vcpu_id			)
 		__field(	unsigned int,	nr_vcpus_matched_tsc	)
 		__field(	unsigned int,	online_vcpus		)
 		__field(	bool,		use_master_clock	)
-		__field(	unsigned int,	host_clock		)
 	),
 
 	TP_fast_assign(
@@ -853,14 +847,11 @@ TRACE_EVENT(kvm_track_tsc,
 		__entry->nr_vcpus_matched_tsc	= nr_matched;
 		__entry->online_vcpus		= online_vcpus;
 		__entry->use_master_clock	= use_master_clock;
-		__entry->host_clock		= host_clock;
 	),
 
-	TP_printk("vcpu_id %u masterclock %u offsetmatched %u nr_online %u"
-		  " hostclock %s",
+	TP_printk("vcpu_id %u masterclock %u offsetmatched %u nr_online %u",
 		  __entry->vcpu_id, __entry->use_master_clock,
-		  __entry->nr_vcpus_matched_tsc, __entry->online_vcpus,
-		  __print_symbolic(__entry->host_clock, host_clocks))
+		  __entry->nr_vcpus_matched_tsc, __entry->online_vcpus)
 );
 
 #endif /* CONFIG_X86_64 */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d8ec2ca..53754fa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -50,7 +50,7 @@
 #include <linux/hash.h>
 #include <linux/pci.h>
 #include <linux/timekeeper_internal.h>
-#include <linux/pvclock_gtod.h>
+#include <linux/cs_notifier.h>
 #include <linux/kvm_irqfd.h>
 #include <linux/irqbypass.h>
 #include <linux/sched/stat.h>
@@ -1134,50 +1134,6 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 	return kvm_set_msr(vcpu, &msr);
 }
 
-#ifdef CONFIG_X86_64
-struct pvclock_gtod_data {
-	seqcount_t	seq;
-
-	struct { /* extract of a clocksource struct */
-		int vclock_mode;
-		u64	cycle_last;
-		u64	mask;
-		u32	mult;
-		u32	shift;
-	} clock;
-
-	u64		boot_ns;
-	u64		nsec_base;
-	u64		wall_time_sec;
-};
-
-static struct pvclock_gtod_data pvclock_gtod_data;
-
-static void update_pvclock_gtod(struct timekeeper *tk)
-{
-	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
-	u64 boot_ns;
-
-	boot_ns = ktime_to_ns(ktime_add(tk->tkr_mono.base, tk->offs_boot));
-
-	write_seqcount_begin(&vdata->seq);
-
-	/* copy pvclock gtod data */
-	vdata->clock.vclock_mode	= tk->tkr_mono.clock->archdata.vclock_mode;
-	vdata->clock.cycle_last		= tk->tkr_mono.cycle_last;
-	vdata->clock.mask		= tk->tkr_mono.mask;
-	vdata->clock.mult		= tk->tkr_mono.mult;
-	vdata->clock.shift		= tk->tkr_mono.shift;
-
-	vdata->boot_ns			= boot_ns;
-	vdata->nsec_base		= tk->tkr_mono.xtime_nsec;
-
-	vdata->wall_time_sec            = tk->xtime_sec;
-
-	write_seqcount_end(&vdata->seq);
-}
-#endif
-
 void kvm_set_pending_timer(struct kvm_vcpu *vcpu)
 {
 	/*
@@ -1269,10 +1225,6 @@ static void kvm_get_time_scale(uint64_t scaled_hz, uint64_t base_hz,
 		 __func__, base_hz, scaled_hz, shift, *pmultiplier);
 }
 
-#ifdef CONFIG_X86_64
-static atomic_t kvm_guest_has_master_clock = ATOMIC_INIT(0);
-#endif
-
 static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
 static unsigned long max_tsc_khz;
 
@@ -1366,7 +1318,6 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
 #ifdef CONFIG_X86_64
 	bool vcpus_matched;
 	struct kvm_arch *ka = &vcpu->kvm->arch;
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
 
 	vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
 			 atomic_read(&vcpu->kvm->online_vcpus));
@@ -1379,13 +1330,12 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
 	 * and the vcpus need to have matched TSCs.  When that happens,
 	 * perform request to enable masterclock.
 	 */
-	if (ka->use_master_clock ||
-	    (gtod->clock.vclock_mode == VCLOCK_TSC && vcpus_matched))
+	if (ka->use_master_clock || vcpus_matched)
 		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
 
 	trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc,
-			    atomic_read(&vcpu->kvm->online_vcpus),
-		            ka->use_master_clock, gtod->clock.vclock_mode);
+				atomic_read(&vcpu->kvm->online_vcpus),
+				ka->use_master_clock);
 #endif
 }
 
@@ -1538,7 +1488,7 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	kvm_vcpu_write_tsc_offset(vcpu, offset);
 	raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);
 
-	spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
+	spin_lock(&kvm->arch.masterclock_lock);
 	if (!matched) {
 		kvm->arch.nr_vcpus_matched_tsc = 0;
 	} else if (!already_matched) {
@@ -1546,9 +1496,8 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	}
 
 	kvm_track_tsc_matching(vcpu);
-	spin_unlock(&kvm->arch.pvclock_gtod_sync_lock);
+	spin_unlock(&kvm->arch.masterclock_lock);
 }
-
 EXPORT_SYMBOL_GPL(kvm_write_tsc);
 
 static inline void adjust_tsc_offset_guest(struct kvm_vcpu *vcpu,
@@ -1567,79 +1516,6 @@ static inline void adjust_tsc_offset_host(struct kvm_vcpu *vcpu, s64 adjustment)
 
 #ifdef CONFIG_X86_64
 
-static u64 read_tsc(void)
-{
-	u64 ret = (u64)rdtsc_ordered();
-	u64 last = pvclock_gtod_data.clock.cycle_last;
-
-	if (likely(ret >= last))
-		return ret;
-
-	/*
-	 * GCC likes to generate cmov here, but this branch is extremely
-	 * predictable (it's just a function of time and the likely is
-	 * very likely) and there's a data dependence, so force GCC
-	 * to generate a branch instead.  I don't barrier() because
-	 * we don't actually need a barrier, and if this function
-	 * ever gets inlined it will generate worse code.
-	 */
-	asm volatile ("");
-	return last;
-}
-
-static inline u64 vgettsc(u64 *cycle_now)
-{
-	long v;
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
-
-	*cycle_now = read_tsc();
-
-	v = (*cycle_now - gtod->clock.cycle_last) & gtod->clock.mask;
-	return v * gtod->clock.mult;
-}
-
-static int do_monotonic_boot(s64 *t, u64 *cycle_now)
-{
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
-	unsigned long seq;
-	int mode;
-	u64 ns;
-
-	do {
-		seq = read_seqcount_begin(&gtod->seq);
-		mode = gtod->clock.vclock_mode;
-		ns = gtod->nsec_base;
-		ns += vgettsc(cycle_now);
-		ns >>= gtod->clock.shift;
-		ns += gtod->boot_ns;
-	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
-	*t = ns;
-
-	return mode;
-}
-
-static int do_realtime(struct timespec *ts, u64 *cycle_now)
-{
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
-	unsigned long seq;
-	int mode;
-	u64 ns;
-
-	do {
-		seq = read_seqcount_begin(&gtod->seq);
-		mode = gtod->clock.vclock_mode;
-		ts->tv_sec = gtod->wall_time_sec;
-		ns = gtod->nsec_base;
-		ns += vgettsc(cycle_now);
-		ns >>= gtod->clock.shift;
-	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
-
-	ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
-	ts->tv_nsec = ns;
-
-	return mode;
-}
-
 /* returns true if host is using tsc clocksource */
 static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *cycle_now)
 {
@@ -1713,34 +1589,28 @@ static bool kvm_get_walltime_and_clockread(struct timespec *ts,
  *
  */
 
-static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
+static void update_masterclock(struct kvm *kvm)
 {
 #ifdef CONFIG_X86_64
 	struct kvm_arch *ka = &kvm->arch;
-	int vclock_mode;
-	bool host_tsc_clocksource, vcpus_matched;
+	bool host_clocksource_stable, vcpus_matched;
 
 	vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
 			atomic_read(&kvm->online_vcpus));
 
 	/*
-	 * If the host uses TSC clock, then passthrough TSC as stable
-	 * to the guest.
+	 * kvm_get_time_and_clockread returns true if clocksource is stable
 	 */
-	host_tsc_clocksource = kvm_get_time_and_clockread(
+	host_clocksource_stable = kvm_get_time_and_clockread(
 					&ka->master_kernel_ns,
 					&ka->master_cycle_now);
 
-	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
+	ka->use_master_clock = host_clocksource_stable && vcpus_matched
 				&& !ka->backwards_tsc_observed
 				&& !ka->boot_vcpu_runs_old_kvmclock;
 
-	if (ka->use_master_clock)
-		atomic_set(&kvm_guest_has_master_clock, 1);
-
-	vclock_mode = pvclock_gtod_data.clock.vclock_mode;
-	trace_kvm_update_master_clock(ka->use_master_clock, vclock_mode,
-					vcpus_matched);
+	trace_kvm_update_master_clock(ka->use_master_clock,
+				host_clocksource_stable, vcpus_matched);
 #endif
 }
 
@@ -1756,10 +1626,10 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	struct kvm_arch *ka = &kvm->arch;
 
-	spin_lock(&ka->pvclock_gtod_sync_lock);
+	spin_lock(&ka->masterclock_lock);
 	kvm_make_mclock_inprogress_request(kvm);
 	/* no guest entries from this point */
-	pvclock_update_vm_gtod_copy(kvm);
+	update_masterclock(kvm);
 
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
@@ -1768,7 +1638,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
 
-	spin_unlock(&ka->pvclock_gtod_sync_lock);
+	spin_unlock(&ka->masterclock_lock);
 #endif
 }
 
@@ -1778,15 +1648,15 @@ u64 get_kvmclock_ns(struct kvm *kvm)
 	struct pvclock_vcpu_time_info hv_clock;
 	u64 ret;
 
-	spin_lock(&ka->pvclock_gtod_sync_lock);
+	spin_lock(&ka->masterclock_lock);
 	if (!ka->use_master_clock) {
-		spin_unlock(&ka->pvclock_gtod_sync_lock);
+		spin_unlock(&ka->masterclock_lock);
 		return ktime_get_boot_ns() + ka->kvmclock_offset;
 	}
 
 	hv_clock.tsc_timestamp = ka->master_cycle_now;
 	hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
-	spin_unlock(&ka->pvclock_gtod_sync_lock);
+	spin_unlock(&ka->masterclock_lock);
 
 	/* both __this_cpu_read() and rdtsc() should be on the same cpu */
 	get_cpu();
@@ -1872,13 +1742,13 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	 * If the host uses TSC clock, then passthrough TSC as stable
 	 * to the guest.
 	 */
-	spin_lock(&ka->pvclock_gtod_sync_lock);
+	spin_lock(&ka->masterclock_lock);
 	use_master_clock = ka->use_master_clock;
 	if (use_master_clock) {
 		host_tsc = ka->master_cycle_now;
 		kernel_ns = ka->master_kernel_ns;
 	}
-	spin_unlock(&ka->pvclock_gtod_sync_lock);
+	spin_unlock(&ka->masterclock_lock);
 
 	/* Keep irq disabled to prevent changes to the clock */
 	local_irq_save(flags);
@@ -4208,11 +4078,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 
 		r = 0;
-		/*
-		 * TODO: userspace has to take care of races with VCPU_RUN, so
-		 * kvm_gen_update_masterclock() can be cut down to locked
-		 * pvclock_update_vm_gtod_copy().
-		 */
+
 		kvm_gen_update_masterclock(kvm);
 		now_ns = get_kvmclock_ns(kvm);
 		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
@@ -6041,7 +5907,8 @@ static void kvm_set_mmio_spte_mask(void)
 }
 
 #ifdef CONFIG_X86_64
-static void pvclock_gtod_update_fn(struct work_struct *work)
+static int process_clocksource_change(struct notifier_block *nb,
+					unsigned long unused0, void *unused1)
 {
 	struct kvm *kvm;
 
@@ -6052,35 +5919,12 @@ static void pvclock_gtod_update_fn(struct work_struct *work)
 	list_for_each_entry(kvm, &vm_list, vm_list)
 		kvm_for_each_vcpu(i, vcpu, kvm)
 			kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
-	atomic_set(&kvm_guest_has_master_clock, 0);
 	spin_unlock(&kvm_lock);
-}
-
-static DECLARE_WORK(pvclock_gtod_work, pvclock_gtod_update_fn);
-
-/*
- * Notification about pvclock gtod data update.
- */
-static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
-			       void *priv)
-{
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
-	struct timekeeper *tk = priv;
-
-	update_pvclock_gtod(tk);
-
-	/* disable master clock if host does not trust, or does not
-	 * use, TSC clocksource
-	 */
-	if (gtod->clock.vclock_mode != VCLOCK_TSC &&
-	    atomic_read(&kvm_guest_has_master_clock) != 0)
-		queue_work(system_long_wq, &pvclock_gtod_work);
-
 	return 0;
 }
 
-static struct notifier_block pvclock_gtod_notifier = {
-	.notifier_call = pvclock_gtod_notify,
+static struct notifier_block clocksource_change_notifier = {
+	.notifier_call = process_clocksource_change,
 };
 #endif
 
@@ -6133,7 +5977,7 @@ int kvm_arch_init(void *opaque)
 
 	kvm_lapic_init();
 #ifdef CONFIG_X86_64
-	pvclock_gtod_register_notifier(&pvclock_gtod_notifier);
+	clocksource_changes_register_notifier(&clocksource_change_notifier);
 #endif
 
 	return 0;
@@ -6154,7 +5998,7 @@ void kvm_arch_exit(void)
 					    CPUFREQ_TRANSITION_NOTIFIER);
 	cpuhp_remove_state_nocalls(CPUHP_AP_X86_KVM_CLK_ONLINE);
 #ifdef CONFIG_X86_64
-	pvclock_gtod_unregister_notifier(&pvclock_gtod_notifier);
+	clocksource_changes_unregister_notifier(&clocksource_change_notifier);
 #endif
 	kvm_x86_ops = NULL;
 	kvm_mmu_module_exit();
@@ -8056,10 +7900,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	raw_spin_lock_init(&kvm->arch.tsc_write_lock);
 	mutex_init(&kvm->arch.apic_map_lock);
 	mutex_init(&kvm->arch.hyperv.hv_lock);
-	spin_lock_init(&kvm->arch.pvclock_gtod_sync_lock);
+	spin_lock_init(&kvm->arch.masterclock_lock);
 
 	kvm->arch.kvmclock_offset = -ktime_get_boot_ns();
-	pvclock_update_vm_gtod_copy(kvm);
+	update_masterclock(kvm);
 
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
-- 
2.7.4

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

#1702565 — Re: [PATCH v4 07/10] KVM: x86: remove not used pvclock_gtod_copy

Hi Denis,

I'm all for this as well, the original submission suggested something
similar, someone said "use a scheme similar to vsyscalls", 
therefore the internal copy of the fields.

More comments below.


On Wed, Aug 02, 2017 at 05:38:07PM +0300, Denis Plotnikov wrote:
> Since, KVM has been switched to getting masterclock related data
> right from the timekeeper by the previous patches, now we are able
> to remove all the parts related to the old scheme of getting
> masterclock data.
> 
> This patch removes those parts.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  arch/x86/include/asm/kvm_host.h |   2 +-
>  arch/x86/kvm/trace.h            |  31 ++----
>  arch/x86/kvm/x86.c              | 216 ++++++----------------------------------
>  3 files changed, 42 insertions(+), 207 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 87ac4fb..91465db 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -791,7 +791,7 @@ struct kvm_arch {
>  	u64 cur_tsc_generation;
>  	int nr_vcpus_matched_tsc;
>  
> -	spinlock_t pvclock_gtod_sync_lock;
> +	spinlock_t masterclock_lock;
>  	bool use_master_clock;
>  	u64 master_kernel_ns;
>  	u64 master_cycle_now;
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 0a6cc67..923ab31 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -807,45 +807,39 @@ TRACE_EVENT(kvm_write_tsc_offset,
>  
>  #ifdef CONFIG_X86_64
>  
> -#define host_clocks					\
> -	{VCLOCK_NONE, "none"},				\
> -	{VCLOCK_TSC,  "tsc"}				\
> -
>  TRACE_EVENT(kvm_update_master_clock,
> -	TP_PROTO(bool use_master_clock, unsigned int host_clock, bool offset_matched),
> -	TP_ARGS(use_master_clock, host_clock, offset_matched),
> +	TP_PROTO(bool use_master_clock, bool host_clock_stable,
> +		bool offset_matched),
> +	TP_ARGS(use_master_clock, host_clock_stable, offset_matched),
>  
>  	TP_STRUCT__entry(
>  		__field(		bool,	use_master_clock	)
> -		__field(	unsigned int,	host_clock		)
> +		__field(		bool,	host_clock_stable	)
>  		__field(		bool,	offset_matched		)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->use_master_clock	= use_master_clock;
> -		__entry->host_clock		= host_clock;
> +		__entry->host_clock_stable	= host_clock_stable;
>  		__entry->offset_matched		= offset_matched;
>  	),
>  
> -	TP_printk("masterclock %d hostclock %s offsetmatched %u",
> +	TP_printk("masterclock %d hostclock stable %u offsetmatched %u",
>  		  __entry->use_master_clock,
> -		  __print_symbolic(__entry->host_clock, host_clocks),
> +		  __entry->host_clock_stable,
>  		  __entry->offset_matched)
>  );
>  
>  TRACE_EVENT(kvm_track_tsc,
>  	TP_PROTO(unsigned int vcpu_id, unsigned int nr_matched,
> -		 unsigned int online_vcpus, bool use_master_clock,
> -		 unsigned int host_clock),
> -	TP_ARGS(vcpu_id, nr_matched, online_vcpus, use_master_clock,
> -		host_clock),
> +		 unsigned int online_vcpus, bool use_master_clock),
> +	TP_ARGS(vcpu_id, nr_matched, online_vcpus, use_master_clock),
>  
>  	TP_STRUCT__entry(
>  		__field(	unsigned int,	vcpu_id			)
>  		__field(	unsigned int,	nr_vcpus_matched_tsc	)
>  		__field(	unsigned int,	online_vcpus		)
>  		__field(	bool,		use_master_clock	)
> -		__field(	unsigned int,	host_clock		)
>  	),
>  
>  	TP_fast_assign(
> @@ -853,14 +847,11 @@ TRACE_EVENT(kvm_track_tsc,
>  		__entry->nr_vcpus_matched_tsc	= nr_matched;
>  		__entry->online_vcpus		= online_vcpus;
>  		__entry->use_master_clock	= use_master_clock;
> -		__entry->host_clock		= host_clock;
>  	),
>  
> -	TP_printk("vcpu_id %u masterclock %u offsetmatched %u nr_online %u"
> -		  " hostclock %s",
> +	TP_printk("vcpu_id %u masterclock %u offsetmatched %u nr_online %u",
>  		  __entry->vcpu_id, __entry->use_master_clock,
> -		  __entry->nr_vcpus_matched_tsc, __entry->online_vcpus,
> -		  __print_symbolic(__entry->host_clock, host_clocks))
> +		  __entry->nr_vcpus_matched_tsc, __entry->online_vcpus)
>  );
>  
>  #endif /* CONFIG_X86_64 */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d8ec2ca..53754fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -50,7 +50,7 @@
>  #include <linux/hash.h>
>  #include <linux/pci.h>
>  #include <linux/timekeeper_internal.h>
> -#include <linux/pvclock_gtod.h>
> +#include <linux/cs_notifier.h>
>  #include <linux/kvm_irqfd.h>
>  #include <linux/irqbypass.h>
>  #include <linux/sched/stat.h>
> @@ -1134,50 +1134,6 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>  	return kvm_set_msr(vcpu, &msr);
>  }
>  
> -#ifdef CONFIG_X86_64
> -struct pvclock_gtod_data {
> -	seqcount_t	seq;
> -
> -	struct { /* extract of a clocksource struct */
> -		int vclock_mode;
> -		u64	cycle_last;
> -		u64	mask;
> -		u32	mult;
> -		u32	shift;
> -	} clock;
> -
> -	u64		boot_ns;
> -	u64		nsec_base;
> -	u64		wall_time_sec;
> -};
> -
> -static struct pvclock_gtod_data pvclock_gtod_data;
> -
> -static void update_pvclock_gtod(struct timekeeper *tk)
> -{
> -	struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
> -	u64 boot_ns;
> -
> -	boot_ns = ktime_to_ns(ktime_add(tk->tkr_mono.base, tk->offs_boot));
> -
> -	write_seqcount_begin(&vdata->seq);
> -
> -	/* copy pvclock gtod data */
> -	vdata->clock.vclock_mode	= tk->tkr_mono.clock->archdata.vclock_mode;
> -	vdata->clock.cycle_last		= tk->tkr_mono.cycle_last;
> -	vdata->clock.mask		= tk->tkr_mono.mask;
> -	vdata->clock.mult		= tk->tkr_mono.mult;
> -	vdata->clock.shift		= tk->tkr_mono.shift;
> -
> -	vdata->boot_ns			= boot_ns;
> -	vdata->nsec_base		= tk->tkr_mono.xtime_nsec;
> -
> -	vdata->wall_time_sec            = tk->xtime_sec;
> -
> -	write_seqcount_end(&vdata->seq);
> -}
> -#endif
> -
>  void kvm_set_pending_timer(struct kvm_vcpu *vcpu)
>  {
>  	/*
> @@ -1269,10 +1225,6 @@ static void kvm_get_time_scale(uint64_t scaled_hz, uint64_t base_hz,
>  		 __func__, base_hz, scaled_hz, shift, *pmultiplier);
>  }
>  
> -#ifdef CONFIG_X86_64
> -static atomic_t kvm_guest_has_master_clock = ATOMIC_INIT(0);
> -#endif
> -
>  static DEFINE_PER_CPU(unsigned long, cpu_tsc_khz);
>  static unsigned long max_tsc_khz;
>  
> @@ -1366,7 +1318,6 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
>  #ifdef CONFIG_X86_64
>  	bool vcpus_matched;
>  	struct kvm_arch *ka = &vcpu->kvm->arch;
> -	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
>  
>  	vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
>  			 atomic_read(&vcpu->kvm->online_vcpus));
> @@ -1379,13 +1330,12 @@ static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu)
>  	 * and the vcpus need to have matched TSCs.  When that happens,
>  	 * perform request to enable masterclock.
>  	 */
> -	if (ka->use_master_clock ||
> -	    (gtod->clock.vclock_mode == VCLOCK_TSC && vcpus_matched))
> +	if (ka->use_master_clock || vcpus_matched)
>  		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);

Don't drop this. The masterclock scheme requires TSC for proper functioning 
(or an analysis why its supposed with different HPET+TSC, for example).

>  
>  	trace_kvm_track_tsc(vcpu->vcpu_id, ka->nr_vcpus_matched_tsc,
> -			    atomic_read(&vcpu->kvm->online_vcpus),
> -		            ka->use_master_clock, gtod->clock.vclock_mode);
> +				atomic_read(&vcpu->kvm->online_vcpus),
> +				ka->use_master_clock);
>  #endif
>  }
>  
> @@ -1538,7 +1488,7 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	kvm_vcpu_write_tsc_offset(vcpu, offset);
>  	raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);
>  
> -	spin_lock(&kvm->arch.pvclock_gtod_sync_lock);
> +	spin_lock(&kvm->arch.masterclock_lock);
>  	if (!matched) {
>  		kvm->arch.nr_vcpus_matched_tsc = 0;
>  	} else if (!already_matched) {
> @@ -1546,9 +1496,8 @@ void kvm_write_tsc(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	}
>  
>  	kvm_track_tsc_matching(vcpu);
> -	spin_unlock(&kvm->arch.pvclock_gtod_sync_lock);
> +	spin_unlock(&kvm->arch.masterclock_lock);
>  }
> -
>  EXPORT_SYMBOL_GPL(kvm_write_tsc);
>  
>  static inline void adjust_tsc_offset_guest(struct kvm_vcpu *vcpu,
> @@ -1567,79 +1516,6 @@ static inline void adjust_tsc_offset_host(struct kvm_vcpu *vcpu, s64 adjustment)
>  
>  #ifdef CONFIG_X86_64
>  
> -static u64 read_tsc(void)
> -{
> -	u64 ret = (u64)rdtsc_ordered();
> -	u64 last = pvclock_gtod_data.clock.cycle_last;
> -
> -	if (likely(ret >= last))
> -		return ret;
> -
> -	/*
> -	 * GCC likes to generate cmov here, but this branch is extremely
> -	 * predictable (it's just a function of time and the likely is
> -	 * very likely) and there's a data dependence, so force GCC
> -	 * to generate a branch instead.  I don't barrier() because
> -	 * we don't actually need a barrier, and if this function
> -	 * ever gets inlined it will generate worse code.
> -	 */
> -	asm volatile ("");
> -	return last;
> -}
> -
> -static inline u64 vgettsc(u64 *cycle_now)
> -{
> -	long v;
> -	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> -
> -	*cycle_now = read_tsc();
> -
> -	v = (*cycle_now - gtod->clock.cycle_last) & gtod->clock.mask;
> -	return v * gtod->clock.mult;
> -}
> -
> -static int do_monotonic_boot(s64 *t, u64 *cycle_now)
> -{
> -	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> -	unsigned long seq;
> -	int mode;
> -	u64 ns;
> -
> -	do {
> -		seq = read_seqcount_begin(&gtod->seq);
> -		mode = gtod->clock.vclock_mode;
> -		ns = gtod->nsec_base;
> -		ns += vgettsc(cycle_now);
> -		ns >>= gtod->clock.shift;
> -		ns += gtod->boot_ns;
> -	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
> -	*t = ns;
> -
> -	return mode;
> -}
> -
> -static int do_realtime(struct timespec *ts, u64 *cycle_now)
> -{
> -	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> -	unsigned long seq;
> -	int mode;
> -	u64 ns;
> -
> -	do {
> -		seq = read_seqcount_begin(&gtod->seq);
> -		mode = gtod->clock.vclock_mode;
> -		ts->tv_sec = gtod->wall_time_sec;
> -		ns = gtod->nsec_base;
> -		ns += vgettsc(cycle_now);
> -		ns >>= gtod->clock.shift;
> -	} while (unlikely(read_seqcount_retry(&gtod->seq, seq)));
> -
> -	ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
> -	ts->tv_nsec = ns;
> -
> -	return mode;
> -}
> -
>  /* returns true if host is using tsc clocksource */
>  static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *cycle_now)
>  {
> @@ -1713,34 +1589,28 @@ static bool kvm_get_walltime_and_clockread(struct timespec *ts,
>   *
>   */
>  
> -static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
> +static void update_masterclock(struct kvm *kvm)
>  {
>  #ifdef CONFIG_X86_64
>  	struct kvm_arch *ka = &kvm->arch;
> -	int vclock_mode;
> -	bool host_tsc_clocksource, vcpus_matched;
> +	bool host_clocksource_stable, vcpus_matched;
>  
>  	vcpus_matched = (ka->nr_vcpus_matched_tsc + 1 ==
>  			atomic_read(&kvm->online_vcpus));
>  
>  	/*
> -	 * If the host uses TSC clock, then passthrough TSC as stable
> -	 * to the guest.
> +	 * kvm_get_time_and_clockread returns true if clocksource is stable
>  	 */
> -	host_tsc_clocksource = kvm_get_time_and_clockread(
> +	host_clocksource_stable = kvm_get_time_and_clockread(
>  					&ka->master_kernel_ns,
>  					&ka->master_cycle_now);
>  
> -	ka->use_master_clock = host_tsc_clocksource && vcpus_matched
> +	ka->use_master_clock = host_clocksource_stable && vcpus_matched
>  				&& !ka->backwards_tsc_observed
>  				&& !ka->boot_vcpu_runs_old_kvmclock;
>  
> -	if (ka->use_master_clock)
> -		atomic_set(&kvm_guest_has_master_clock, 1);
> -
> -	vclock_mode = pvclock_gtod_data.clock.vclock_mode;
> -	trace_kvm_update_master_clock(ka->use_master_clock, vclock_mode,
> -					vcpus_matched);
> +	trace_kvm_update_master_clock(ka->use_master_clock,
> +				host_clocksource_stable, vcpus_matched);
>  #endif
>  }
>  
> @@ -1756,10 +1626,10 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	struct kvm_arch *ka = &kvm->arch;
>  
> -	spin_lock(&ka->pvclock_gtod_sync_lock);
> +	spin_lock(&ka->masterclock_lock);
>  	kvm_make_mclock_inprogress_request(kvm);
>  	/* no guest entries from this point */
> -	pvclock_update_vm_gtod_copy(kvm);
> +	update_masterclock(kvm);
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> @@ -1768,7 +1638,7 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
>  
> -	spin_unlock(&ka->pvclock_gtod_sync_lock);
> +	spin_unlock(&ka->masterclock_lock);
>  #endif
>  }
>  
> @@ -1778,15 +1648,15 @@ u64 get_kvmclock_ns(struct kvm *kvm)
>  	struct pvclock_vcpu_time_info hv_clock;
>  	u64 ret;
>  
> -	spin_lock(&ka->pvclock_gtod_sync_lock);
> +	spin_lock(&ka->masterclock_lock);
>  	if (!ka->use_master_clock) {
> -		spin_unlock(&ka->pvclock_gtod_sync_lock);
> +		spin_unlock(&ka->masterclock_lock);
>  		return ktime_get_boot_ns() + ka->kvmclock_offset;
>  	}
>  
>  	hv_clock.tsc_timestamp = ka->master_cycle_now;
>  	hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
> -	spin_unlock(&ka->pvclock_gtod_sync_lock);
> +	spin_unlock(&ka->masterclock_lock);
>  
>  	/* both __this_cpu_read() and rdtsc() should be on the same cpu */
>  	get_cpu();
> @@ -1872,13 +1742,13 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>  	 * If the host uses TSC clock, then passthrough TSC as stable
>  	 * to the guest.
>  	 */
> -	spin_lock(&ka->pvclock_gtod_sync_lock);
> +	spin_lock(&ka->masterclock_lock);
>  	use_master_clock = ka->use_master_clock;
>  	if (use_master_clock) {
>  		host_tsc = ka->master_cycle_now;
>  		kernel_ns = ka->master_kernel_ns;
>  	}
> -	spin_unlock(&ka->pvclock_gtod_sync_lock);
> +	spin_unlock(&ka->masterclock_lock);
>  
>  	/* Keep irq disabled to prevent changes to the clock */
>  	local_irq_save(flags);
> @@ -4208,11 +4078,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto out;
>  
>  		r = 0;
> -		/*
> -		 * TODO: userspace has to take care of races with VCPU_RUN, so
> -		 * kvm_gen_update_masterclock() can be cut down to locked
> -		 * pvclock_update_vm_gtod_copy().
> -		 */

I have no idea what race is this.. do you know?

> +
>  		kvm_gen_update_masterclock(kvm);
>  		now_ns = get_kvmclock_ns(kvm);
>  		kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> @@ -6041,7 +5907,8 @@ static void kvm_set_mmio_spte_mask(void)
>  }
>  
>  #ifdef CONFIG_X86_64
> -static void pvclock_gtod_update_fn(struct work_struct *work)
> +static int process_clocksource_change(struct notifier_block *nb,
> +					unsigned long unused0, void *unused1)
>  {
>  	struct kvm *kvm;
>  
> @@ -6052,35 +5919,12 @@ static void pvclock_gtod_update_fn(struct work_struct *work)
>  	list_for_each_entry(kvm, &vm_list, vm_list)
>  		kvm_for_each_vcpu(i, vcpu, kvm)
>  			kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> -	atomic_set(&kvm_guest_has_master_clock, 0);
>  	spin_unlock(&kvm_lock);
> -}
> -
> -static DECLARE_WORK(pvclock_gtod_work, pvclock_gtod_update_fn);
> -
> -/*
> - * Notification about pvclock gtod data update.
> - */
> -static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
> -			       void *priv)
> -{
> -	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> -	struct timekeeper *tk = priv;
> -
> -	update_pvclock_gtod(tk);
> -
> -	/* disable master clock if host does not trust, or does not
> -	 * use, TSC clocksource
> -	 */
> -	if (gtod->clock.vclock_mode != VCLOCK_TSC &&
> -	    atomic_read(&kvm_guest_has_master_clock) != 0)
> -		queue_work(system_long_wq, &pvclock_gtod_work);

Don't drop this: TSC is required, and switching to another
clock must disable masterclock scheme.

> -
>  	return 0;
>  }
>  
> -static struct notifier_block pvclock_gtod_notifier = {
> -	.notifier_call = pvclock_gtod_notify,
> +static struct notifier_block clocksource_change_notifier = {
> +	.notifier_call = process_clocksource_change,
>  };
>  #endif
>  
> @@ -6133,7 +5977,7 @@ int kvm_arch_init(void *opaque)
>  
>  	kvm_lapic_init();
>  #ifdef CONFIG_X86_64
> -	pvclock_gtod_register_notifier(&pvclock_gtod_notifier);
> +	clocksource_changes_register_notifier(&clocksource_change_notifier);
>  #endif
>  
>  	return 0;
> @@ -6154,7 +5998,7 @@ void kvm_arch_exit(void)
>  					    CPUFREQ_TRANSITION_NOTIFIER);
>  	cpuhp_remove_state_nocalls(CPUHP_AP_X86_KVM_CLK_ONLINE);
>  #ifdef CONFIG_X86_64
> -	pvclock_gtod_unregister_notifier(&pvclock_gtod_notifier);
> +	clocksource_changes_unregister_notifier(&clocksource_change_notifier);
>  #endif
>  	kvm_x86_ops = NULL;
>  	kvm_mmu_module_exit();
> @@ -8056,10 +7900,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	raw_spin_lock_init(&kvm->arch.tsc_write_lock);
>  	mutex_init(&kvm->arch.apic_map_lock);
>  	mutex_init(&kvm->arch.hyperv.hv_lock);
> -	spin_lock_init(&kvm->arch.pvclock_gtod_sync_lock);
> +	spin_lock_init(&kvm->arch.masterclock_lock);
>  
>  	kvm->arch.kvmclock_offset = -ktime_get_boot_ns();
> -	pvclock_update_vm_gtod_copy(kvm);
> +	update_masterclock(kvm);
>  
>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
> -- 
> 2.7.4

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

#1703059 — Re: [PATCH v4 07/10] KVM: x86: remove not used pvclock_gtod_copy

On 03/08/2017 01:21, Marcelo Tosatti wrote:
>> -	if (ka->use_master_clock ||
>> -	    (gtod->clock.vclock_mode == VCLOCK_TSC && vcpus_matched))
>> +	if (ka->use_master_clock || vcpus_matched)
>>  		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> Don't drop this. The masterclock scheme requires TSC for proper functioning 
> (or an analysis why its supposed with different HPET+TSC, for example).

I think testing gtod->clock.vclock_mode is just an optimization?
kvm_get_time_and_clockread would return false anyway and masterclock
would not be enabled.

Paolo

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

#1702183 — [PATCH v4 09/10] pvclock: add clocksource change notification on changing of tsc stable bit

It's needed to notify the KVM guest about critical changes in pvclock
and make it to update its masterclock.

This is a part of the work aiming to make kvmclock be a clocksource
providing valid cycles value for KVM masterclock, another words
make possible to use KVM masterclock over kvmclock clocksource.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 arch/x86/kernel/pvclock.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index bece384..5898f20 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -22,6 +22,7 @@
 #include <linux/gfp.h>
 #include <linux/bootmem.h>
 #include <linux/nmi.h>
+#include <linux/cs_notifier.h>
 
 #include <asm/fixmap.h>
 #include <asm/pvclock.h>
@@ -73,6 +74,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 	return flags & valid_flags;
 }
 
+static atomic_t clocksource_stable = ATOMIC_INIT(0);
+
 u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src,
 				u64 *cycles_stamp, u8 *flags_stamp)
 {
@@ -102,10 +105,20 @@ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src,
 		pvclock_touch_watchdogs();
 	}
 
-	if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) &&
-		(flags & PVCLOCK_TSC_STABLE_BIT))
-		return ret;
+	if (likely(valid_flags & PVCLOCK_TSC_STABLE_BIT)) {
+		bool stable_now = !!(flags & PVCLOCK_TSC_STABLE_BIT);
+		bool stable_last = (bool) atomic_read(&clocksource_stable);
+
+		if (unlikely(stable_now != stable_last)) {
+			/* send notification once */
+			if (stable_last == atomic_cmpxchg(
+				&clocksource_stable, stable_last, stable_now))
+				clocksource_changes_notify();
+		}
 
+		if (stable_now)
+			return ret;
+	}
 	/*
 	 * Assumption here is that last_value, a global accumulator, always goes
 	 * forward. If we are less than that, we should not be much smaller.
-- 
2.7.4

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

#1702570 — Re: [PATCH v4 09/10] pvclock: add clocksource change notification on changing of tsc stable bit

On Wed, Aug 02, 2017 at 05:38:09PM +0300, Denis Plotnikov wrote:
> It's needed to notify the KVM guest about critical changes in pvclock
> and make it to update its masterclock.
> 
> This is a part of the work aiming to make kvmclock be a clocksource
> providing valid cycles value for KVM masterclock, another words
> make possible to use KVM masterclock over kvmclock clocksource.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  arch/x86/kernel/pvclock.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

Please do an analysis similar to the comment which starts at 

"* Assuming a stable TSC across physical CPUS, and a stable TSC
 * across virtual CPUs, the following condition is possible.
 * Each numbered line represents an event visible to both
 * CPUs at the next numbered event."

Describing why its safe to use kvmclock as source for the masterclock
(honestly i haven't gone through the details, but someone should 
before this patch is merged).

For one thing, its only safe to use kvmclock masterclock if
the TSCs are synchronized in the host (so that you can read offset
on vcpu-0 using a TSC that has been initialized on vcpu-1). 
So masterclock in the L1 guest is necessary. Do you enforce that?

Also L2 guest TSCs must be synchronized. Where is that enforced?

Also why its safe to use non-TSC-clocksource (clock_read, tsc_read) + 
tsc offsets from that point.



> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index bece384..5898f20 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -22,6 +22,7 @@
>  #include <linux/gfp.h>
>  #include <linux/bootmem.h>
>  #include <linux/nmi.h>
> +#include <linux/cs_notifier.h>
>  
>  #include <asm/fixmap.h>
>  #include <asm/pvclock.h>
> @@ -73,6 +74,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  	return flags & valid_flags;
>  }
>  
> +static atomic_t clocksource_stable = ATOMIC_INIT(0);
> +
>  u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src,
>  				u64 *cycles_stamp, u8 *flags_stamp)
>  {
> @@ -102,10 +105,20 @@ u64 pvclock_clocksource_read(struct pvclock_vcpu_time_info *src,
>  		pvclock_touch_watchdogs();
>  	}
>  
> -	if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) &&
> -		(flags & PVCLOCK_TSC_STABLE_BIT))
> -		return ret;
> +	if (likely(valid_flags & PVCLOCK_TSC_STABLE_BIT)) {
> +		bool stable_now = !!(flags & PVCLOCK_TSC_STABLE_BIT);
> +		bool stable_last = (bool) atomic_read(&clocksource_stable);
> +
> +		if (unlikely(stable_now != stable_last)) {
> +			/* send notification once */
> +			if (stable_last == atomic_cmpxchg(
> +				&clocksource_stable, stable_last, stable_now))
> +				clocksource_changes_notify();
> +		}
>  
> +		if (stable_now)
> +			return ret;
> +	}
>  	/*
>  	 * Assumption here is that last_value, a global accumulator, always goes
>  	 * forward. If we are less than that, we should not be much smaller.
> -- 
> 2.7.4

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

#1702184 — [PATCH v4 06/10] timekeeper: add clocksource change notifier

This notifier will fire when clocksource is changed or
any properties of the clocksource are changed which alter
the clocksource critical properties, e.g clocksource stability.

It will be used in updating the KVM masterclock with the help
of timekeeper because it's the very moment to notify KVM about
critical changes happened in the underlying timekeeper.

This is a final related to the timekeeper patch of the work aiming
to move to a more simple scheme of masterclock related values
calculation in KVM.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 include/linux/cs_notifier.h | 17 +++++++++++++++
 kernel/time/timekeeping.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 include/linux/cs_notifier.h

diff --git a/include/linux/cs_notifier.h b/include/linux/cs_notifier.h
new file mode 100644
index 0000000..2b1b4e6
--- /dev/null
+++ b/include/linux/cs_notifier.h
@@ -0,0 +1,17 @@
+#ifndef _CS_CHANGES_H
+#define _CS_CHANGES_H
+
+#include <linux/notifier.h>
+
+/*
+ * The clocksource changes notifier is called when the system
+ * clocksource is changed or some properties of the current
+ * system clocksource is changed that can affect other parts of the system,
+ * for example KVM guests
+ */
+
+extern void clocksource_changes_notify(void);
+extern int clocksource_changes_register_notifier(struct notifier_block *nb);
+extern int clocksource_changes_unregister_notifier(struct notifier_block *nb);
+
+#endif /* _CS_CHANGES_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d1aa575..c9dad8a 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -596,6 +596,55 @@ int pvclock_gtod_unregister_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(pvclock_gtod_unregister_notifier);
 
+/* notification chain when there is some changes in the clocksource */
+static RAW_NOTIFIER_HEAD(clocksource_changes_chain);
+
+/**
+ * notify_clocksource_changing - notify all the listeners about changes
+ * happened in the clocksource: changing a clocksource, changing the sensitive
+ * parameters of the clocksource, e.g. stability flag for kvmclock
+ */
+void clocksource_changes_notify(void)
+{
+	raw_notifier_call_chain(&clocksource_changes_chain, 0L, NULL);
+}
+EXPORT_SYMBOL_GPL(clocksource_changes_notify);
+
+/**
+ * clocksource_changes_register_notifier - register
+ * a clocksource changes listener
+ */
+int clocksource_changes_register_notifier(struct notifier_block *nb)
+{
+	unsigned long flags;
+	int ret;
+
+	raw_spin_lock_irqsave(&timekeeper_lock, flags);
+	ret = raw_notifier_chain_register(&clocksource_changes_chain, nb);
+	clocksource_changes_notify();
+	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clocksource_changes_register_notifier);
+
+/**
+ * clocksource_changes_unregister_notifier - unregister
+ * a clocksource changes listener
+ */
+int clocksource_changes_unregister_notifier(struct notifier_block *nb)
+{
+	unsigned long flags;
+	int ret;
+
+	raw_spin_lock_irqsave(&timekeeper_lock, flags);
+	ret = raw_notifier_chain_unregister(&clocksource_changes_chain, nb);
+	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clocksource_changes_unregister_notifier);
+
 /*
  * tk_update_leap_state - helper to update the next_leap_ktime
  */
@@ -1363,6 +1412,7 @@ static int change_clocksource(void *data)
 		}
 	}
 	timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
+	clocksource_changes_notify();
 
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
@@ -1540,6 +1590,7 @@ void __init timekeeping_init(void)
 	tk_set_wall_to_mono(tk, tmp);
 
 	timekeeping_update(tk, TK_MIRROR | TK_CLOCK_WAS_SET);
+	clocksource_changes_notify();
 
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
-- 
2.7.4

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

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

On 02/08/2017 16:38, Denis Plotnikov wrote:
> V4:
>   * removed "is stable" function with vague definition of stability
>     there is the only function which does time with cycle stamp getting
>   * some variables renamed
>   * some patches split into smaller once
>   * atomic64_t usage is replaced with atomic_t

Thanks.  This looks good from the KVM point of view.  Let's see what the
timekeeping guys think.

Paolo

> V3:
>   Changing the timekeeper interface for clocksource reading looks like
>   an overkill to achive the goal of getting cycles stamp for KVM.
>   Instead extend the timekeeping interface and add functions which provide
>   necessary data: read clocksource with cycles stamp, check whether the
>   clock source is stable.
> 
>   Use those functions and improve existing timekeeper functionality to
>   replace pvclock_gtod_copy scheme in masterclock data calculation.
> 
> V2:
>   The main goal is to make L2 kvm-clock be stable when it's running over L1
>   with stable kvm-clock.
> 
>   The patch series is for x86 architecture only. If the series is approved
>   I'll do changes for other architectures but I don't have an ability to
>   compile and check for every single on (help needed)
> 
>   The patch series do the following:
> 
>         * change timekeeper interface to get cycles stamp value from
>           the timekeeper
>         * get rid of pvclock copy in KVM by using the changed timekeeper
>           interface: get time and cycles right from the timekeeper
>         * make KVM recognize a stable kvm-clock as stable clocksource
>           and use the KVM masterclock in this case, which means making
>           L2 stable when running over stable L1 kvm-clock
> 
> Denis Plotnikov (10):
>   timekeeper: introduce extended clocksource reading callback
>   timekeeper: introduce boot field in system_time_snapshot
>   timekeeper: use the extended reading function on snapshot acquiring
>   tsc: implement the extended tsc reading function
>   KVM: x86: switch to masterclock update using timekeeper functionality
>   timekeeper: add clocksource change notifier
>   KVM: x86: remove not used pvclock_gtod_copy
>   pvclock: add parameters to store stamp data in pvclock reading
>     function
>   pvclock: add clocksource change notification on changing of tsc stable
>     bit
>   kvmclock: implement the extended reading function
> 
>  arch/x86/include/asm/kvm_host.h |   2 +-
>  arch/x86/include/asm/pvclock.h  |   3 +-
>  arch/x86/kernel/kvmclock.c      |  19 +++-
>  arch/x86/kernel/pvclock.c       |  37 +++++-
>  arch/x86/kernel/tsc.c           |  10 ++
>  arch/x86/kvm/trace.h            |  31 ++---
>  arch/x86/kvm/x86.c              | 242 ++++++++--------------------------------
>  arch/x86/xen/time.c             |   2 +-
>  include/linux/clocksource.h     |  11 +-
>  include/linux/cs_notifier.h     |  17 +++
>  include/linux/timekeeping.h     |   5 +
>  kernel/time/timekeeping.c       |  68 ++++++++++-
>  12 files changed, 219 insertions(+), 228 deletions(-)
>  create mode 100644 include/linux/cs_notifier.h
> 

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

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

On Wed, Aug 2, 2017 at 7:38 AM, Denis Plotnikov
<dplotnikov@virtuozzo.com> wrote:
> V4:
>   * removed "is stable" function with vague definition of stability
>     there is the only function which does time with cycle stamp getting
>   * some variables renamed
>   * some patches split into smaller once
>   * atomic64_t usage is replaced with atomic_t
>
> V3:
>   Changing the timekeeper interface for clocksource reading looks like
>   an overkill to achive the goal of getting cycles stamp for KVM.
>   Instead extend the timekeeping interface and add functions which provide
>   necessary data: read clocksource with cycles stamp, check whether the
>   clock source is stable.
>
>   Use those functions and improve existing timekeeper functionality to
>   replace pvclock_gtod_copy scheme in masterclock data calculation.
>
> V2:
>   The main goal is to make L2 kvm-clock be stable when it's running over L1
>   with stable kvm-clock.
>
>   The patch series is for x86 architecture only. If the series is approved
>   I'll do changes for other architectures but I don't have an ability to
>   compile and check for every single on (help needed)
>
>   The patch series do the following:
>
>         * change timekeeper interface to get cycles stamp value from
>           the timekeeper
>         * get rid of pvclock copy in KVM by using the changed timekeeper
>           interface: get time and cycles right from the timekeeper
>         * make KVM recognize a stable kvm-clock as stable clocksource
>           and use the KVM masterclock in this case, which means making
>           L2 stable when running over stable L1 kvm-clock

So, from a brief skim, I'm not a big fan of this patchset. Though this
is likely in part due to that I haven't seen anything about *why*
these changes are needed.

Can you briefly explain the issue you're trying to solve, and why you
think this approach is the way to go?
(Its usually a good idea to have such rational included in the patchset)

thanks
-john

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

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

On 02/08/2017 18:49, John Stultz wrote:
> On Wed, Aug 2, 2017 at 7:38 AM, Denis Plotnikov
> <dplotnikov@virtuozzo.com> wrote:
>> V4:
>>   * removed "is stable" function with vague definition of stability
>>     there is the only function which does time with cycle stamp getting
>>   * some variables renamed
>>   * some patches split into smaller once
>>   * atomic64_t usage is replaced with atomic_t
>>
>> V3:
>>   Changing the timekeeper interface for clocksource reading looks like
>>   an overkill to achive the goal of getting cycles stamp for KVM.
>>   Instead extend the timekeeping interface and add functions which provide
>>   necessary data: read clocksource with cycles stamp, check whether the
>>   clock source is stable.
>>
>>   Use those functions and improve existing timekeeper functionality to
>>   replace pvclock_gtod_copy scheme in masterclock data calculation.
>>
>> V2:
>>   The main goal is to make L2 kvm-clock be stable when it's running over L1
>>   with stable kvm-clock.
>>
>>   The patch series is for x86 architecture only. If the series is approved
>>   I'll do changes for other architectures but I don't have an ability to
>>   compile and check for every single on (help needed)
>>
>>   The patch series do the following:
>>
>>         * change timekeeper interface to get cycles stamp value from
>>           the timekeeper
>>         * get rid of pvclock copy in KVM by using the changed timekeeper
>>           interface: get time and cycles right from the timekeeper
>>         * make KVM recognize a stable kvm-clock as stable clocksource
>>           and use the KVM masterclock in this case, which means making
>>           L2 stable when running over stable L1 kvm-clock
> 
> So, from a brief skim, I'm not a big fan of this patchset. Though this
> is likely in part due to that I haven't seen anything about *why*
> these changes are needed.

From my selfish KVM maintainer point of view, one advantage is that it
drops knowledge of internal timekeeping functioning from KVM, using
ktime_get_snapshot instead.  These are patches 1-5.  Structuring the
series like this was my idea so I take the blame.

As to patches 6-10, KVM is currently only able to provide vsyscalls if
the host is using the TSC.  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.  This series
lets you use the vsyscall in L2 as long as L1 can.

There is one point where I couldn't help Denis as much as I wanted.
That's a definition of what's a "good" clocksource that can be used by
KVM to provide the vsyscall.  I know why the patch is correct, but I
couldn't really define the concept.

In ktime_get_snapshot and struct system_counterval_t's users, they seem
to use "cycles" to map from TSC to ART; this is not unlike kvmclock's
use of "cycles" to map from TSC to nanoseconds at an origin point.
However, it's not clear to me whether "cycles" may be used by
adjust_historical_crosststamp even for non-TSC clocksources (or
non-kvmclock after this series).  It doesn't help that
adjust_historical_crosststamp is essentially dead code, since
get_device_system_crosststamp is always called with a NULL history argument.

I'm also CCing Marcelo who wrote the KVM vsyscall code.

Paolo

> Can you briefly explain the issue you're trying to solve, and why you
> think this approach is the way to go?
> (Its usually a good idea to have such rational included in the patchset)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web