lkml.org 
[lkml]   [2019]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH 3/3] Enable ptp_kvm for arm64
On Fri, 06 Sep 2019 12:58:15 +0100,
"Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
>
> Hi Marc,
>
> Very sorry to have missed this comments.
>
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Thursday, August 29, 2019 6:33 PM
> > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> > netdev@vger.kernel.org; pbonzini@redhat.com;
> > sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> > <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> > Poulose <Suzuki.Poulose@arm.com>
> > Cc: linux-kernel@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>;
> > Kaly Xin (Arm Technology China) <Kaly.Xin@arm.com>; Justin He (Arm
> > Technology China) <Justin.He@arm.com>
> > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> >
> > On 29/08/2019 07:39, Jianyong Wu wrote:
> > > Currently in arm64 virtualization environment, there is no mechanism
> > > to keep time sync between guest and host. Time in guest will drift
> > > compared with host after boot up as they may both use third party time
> > > sources to correct their time respectively. The time deviation will be
> > > in order of milliseconds but some scenarios ask for higher time
> > > precision, like in cloud envirenment, we want all the VMs running in
> > > the host aquire the same level accuracy from host clock.
> > >
> > > Use of kvm ptp clock, which choose the host clock source clock as a
> > > reference clock to sync time clock between guest and host has been
> > > adopted by x86 which makes the time sync order from milliseconds to
> > nanoseconds.
> > >
> > > This patch enable kvm ptp on arm64 and we get the similar clock drift
> > > as found with x86 with kvm ptp.
> > >
> > > Test result comparison between with kvm ptp and without it in arm64
> > > are as follows. This test derived from the result of command 'chronyc
> > > sources'. we should take more cure of the last sample column which
> > > shows the offset between the local clock and the source at the last
> > measurement.
> > >
> > > no kvm ptp in guest:
> > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> > >
> > > in host:
> > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> > > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> > > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> > > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> > >
> > > The dns1.synet.edu.cn is the network reference clock for guest and
> > > 120.25.115.20 is the network reference clock for host. we can't get
> > > the clock error between guest and host directly, but a roughly
> > > estimated value will be in order of hundreds of us to ms.
> > >
> > > with kvm ptp in guest:
> > > chrony has been disabled in host to remove the disturb by network clock.
> >
> > Is that a realistic use case? Why should the host not use NTP?
> >
>
> Not really, NTP will change the the host clock which will contaminate the data of sync between
> Host and guest. But in reality, we will keep NTP online.
>
> > >
> > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> > > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> > > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> > > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> > > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> > > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> > > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> > > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> > > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> > > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> > >
> > > The PHC0 is the ptp clock which choose the host clock as its source
> > > clock. So we can be sure to say that the clock error between host and
> > > guest is in order of ns.
> > >
> > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > > ---
> > > arch/arm64/include/asm/arch_timer.h | 3 ++
> > > arch/arm64/kvm/arch_ptp_kvm.c | 76
> > ++++++++++++++++++++++++++++
> > > drivers/clocksource/arm_arch_timer.c | 6 ++-
> > > drivers/ptp/Kconfig | 2 +-
> > > include/linux/arm-smccc.h | 14 +++++
> > > virt/kvm/arm/psci.c | 17 +++++++
> > > 6 files changed, 115 insertions(+), 3 deletions(-) create mode
> > > 100644 arch/arm64/kvm/arch_ptp_kvm.c
> >
> > Please split this patch into two parts: the hypervisor code in a patch and the
> > guest code in another patch. Having both of them together is confusing.
> >
> Ok, really better.
>
> > >
> > > diff --git a/arch/arm64/include/asm/arch_timer.h
> > > b/arch/arm64/include/asm/arch_timer.h
> > > index 6756178c27db..880576a814b6 100644
> > > --- a/arch/arm64/include/asm/arch_timer.h
> > > +++ b/arch/arm64/include/asm/arch_timer.h
> > > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> > > return 0;
> > > }
> > >
> > > +extern struct clocksource clocksource_counter; extern u64
> > > +arch_counter_read(struct clocksource *cs);
> >
> > I'm definitely not keen on exposing the internals of the arch_timer driver to
> > random subsystems. Furthermore, you seem to expect that the guest kernel
> > will only use the arch timer as a clocksource, and nothing really guarantees
> > that (in which case get_device_system_crosststamp will fail).
> >
> The code here is really ugly, I need a better solution to offer a clock source
> For the guest.
>
> > It looks to me that we'd be better off exposing a core timekeeping API that
> > populates a struct system_counterval_t based on the *current* timekeeper
> > monotonic clocksource. This would simplify the split between generic and
> > arch-specific code.
> >
> I think it really necessary.
>
> > Whether or not tglx will be happy with the idea is another problem, but I'm
> > certainly not taking any change to the arch timer code based on this.
> >
> I can have a try, but the detail is not clear for me now.

Something along those lines:

From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Sat, 7 Sep 2019 10:11:49 +0100
Subject: [PATCH] timekeeping: Expose API allowing retrival of current
clocksource and counter value

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
include/linux/timekeeping.h | 5 +++++
kernel/time/timekeeping.c | 12 ++++++++++++
2 files changed, 17 insertions(+)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index b27e2ffa96c1..6df26a913711 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
struct system_time_snapshot *history,
struct system_device_crosststamp *xtstamp);

+/*
+ * Obtain current monotonic clock and its counter value
+ */
+extern void get_current_counterval(struct system_counterval_t *sc);
+
/*
* Simultaneously snapshot realtime and monotonic raw clocks
*/
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d911c8470149..de689bbd3808 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test, u64 after)
return false;
}

+/**
+ * get_current_counterval - Snapshot the current clocksource and counter value
+ * @sc: Pointer to a struct containing the current clocksource and its value
+ */
+void get_current_counterval(struct system_counterval_t *sc)
+{
+ struct timekeeper *tk = &tk_core.timekeeper;
+
+ sc->cs = READ_ONCE(tk->tkr_mono.clock);
+ sc->cycles = sc->cs->read(sc->cs);
+}
+
/**
* get_device_system_crosststamp - Synchronously capture system/device timestamp
* @get_time_fn: Callback to get simultaneous device time and
which should do the right thing.

>
> > > +
> > > #endif
> > > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > > b/arch/arm64/kvm/arch_ptp_kvm.c
> >
> > We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
> > drivers/ptp (as well as its x86 counterpart), and just link the two parts there.
> > This should also allow this to be enabled for 32bit guests.
> >
> Err, sorry, what's mean of "link the two parts there"? should I add
> another two file update driver/ptp/ Both for arm64 and x86 to
> contains these arch-specific code or pack them all into ptp_kvm.c?

What I'm suggesting is that you have 3 files:

drivers/ptp/ptp_kvm.c
drivers/ptp/ptp_kvm_x86.c
drivers/ptp/ptp_kvm_arm.c

and let the Makefile combine them.

[...]

> > Other questions: how does this works with VM migration? Specially when
> > moving from a hypervisor that supports the feature to one that doesn't?
> >
> I think it won't solve the problem generated by VM migration and
> only for VMs in a single machine. Ptp_kvm only works for VMs in the
> same machine. But using ptp (not ptp_kvm) clock, all the machines
> in a low latency network environment can keep time sync in high
> precision, Then VMs move from one machine to another will obtain a
> high precision time sync.

That's a problem. Migration must be possible from one host to another,
even if that means temporarily loosing some (or a lot of)
precision. The service must be discoverable from userspace on the host
so that the MVV can decie whether a migration is possible or not.

Thanks,

M.

--
Jazz is not dead, it just smells funny.

\
 
 \ /
  Last update: 2019-09-07 11:16    [W:0.203 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site