[PATCH v2 2/2] arm64: Add support for CLOCK_MONOTONIC_RAW in clock_gettime() vDSO

[PATCH v2 2/2] arm64: Add support for CLOCK_MONOTONIC_RAW in clock_gettime() vDSO

[Will Deacon]

Hi Daniel,

[adding John, as I know he's working on patches for this]

On Mon, May 22, 2017 at 04:56:34PM -0700, Daniel Mentz wrote:
> One of the tests in the kselftest suite fails for me on a HiKey board
> (ARM Cortex-A53), and I believe that this change could be related to
> it. The specific test that fails is
> tools/testing/selftests/timers/inconsistency-check.c

Have you got any hints on reproducing this? I've failed to reproduce it
so far on any of my machines running 4.12-rc2.

Which version of the HiKey are you using? The latest one is big.LITTLE
with an A73, which has an unfortunate erratum that ends up disabling the
vDSO.

Will


> Here is a snippet of the test output:
> 
> 285:363928252
> 285:363928252
> 285:363928252
> 285:363928252
> --------------------
> 285:363974918
> 285:363974917
> --------------------
> 285:363974917
> 285:363974917
> 285:363974917
> 285:363974917
> 285:363974917
> 285:363974917
> 285:363974917
> 285:363975751
> 285:363975751
> 285:363975751
> 285:363975751
> 285:363975751
> 285:363975751
> 
> As you can see between the two lines "--------------",
> CLOCK_MONOTONIC_RAW went backwards by one nanosecond.
> 
> From what I understand, the vDSO serves the
> clock_gettime(CLOCK_MONOTONIC_RAW) request as follows: The vDSO has
> copies of tk->tkr_mono.cycle_last, tk->raw_time.tv_nsec,
> tk->tkr_raw.mult and tk->tkr_mono.shift. When the user calls
> clock_gettime(), the vDSO reads the Virtual Timer Count register,
> calculates the difference between cycle_last and then converts the
> difference to nanoseconds. The conversation is done by multiplying
> with tk->tkr_raw.mult followed by a right shift operation by
> tk->tkr_mono.shift bits. The last step is to add the result to
> tk->raw_time.tv_nsec.
> 
> The problem with this approach is that the kernel uses a slightly
> different approach when updating tk->raw_time.tv_nsec: Every time the
> timer count registers advances by tk->cycle_interval, it adds
> tk->raw_interval to tk->raw_time.tv_nsec (see
> logarithmic_accumulation()). Both of these values are pre-calculated
> in tk_setup_internals().
> 
> tk->raw_interval is calculated as follows:
> 
> tk->raw_interval =
> ((u64) interval * clock->mult) >> clock->shift;
> 
> A rounding error is introduced by this last right shift operation. As
> a result, this same rounding error is added to tk->raw_time.tv_nsec
> with every update in logarithmic_accumulation(). Hence, the kernel
> adds (or subtracts) a certain rounding error every cycle_interval.
> 
> The issue is that the vDSO code does not add this very same rounding
> error in the same fashion. Instead, the vDSO code just uses
> clock->mult and clock->shift to convert the counter value difference
> into nanoseconds and then add it to tk->raw_time.tv_nsec. It would not
> introduce this rounding error after every cycle interval. As a result,
> the vDSO and the kernel diverge in a way such that the clock might
> appear to be moving backwards by 1ns after the kernel updates
> tk->raw_time.tv_nsec.
> 
> How should we solve this issue? I suggest exporting tk->cycle_interval
> to the vDSO and falling back to the syscall if the Virtual Timer Count
> register advanced by more than cycle_interval.
> 
> Daniel

[PATCH v2 2/2] arm64: Add support for CLOCK_MONOTONIC_RAW in clock_gettime() vDSO

[John Stultz]

On Thu, May 25, 2017 at 9:29 AM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Daniel,
>
> [adding John, as I know he's working on patches for this]
>
> On Mon, May 22, 2017 at 04:56:34PM -0700, Daniel Mentz wrote:
>> One of the tests in the kselftest suite fails for me on a HiKey board
>> (ARM Cortex-A53), and I believe that this change could be related to
>> it. The specific test that fails is
>> tools/testing/selftests/timers/inconsistency-check.c
>
> Have you got any hints on reproducing this? I've failed to reproduce it
> so far on any of my machines running 4.12-rc2.
>
> Which version of the HiKey are you using? The latest one is big.LITTLE
> with an A73, which has an unfortunate erratum that ends up disabling the
> vDSO.
>
> Will
>
>
>> Here is a snippet of the test output:
>>
>> 285:363928252
>> 285:363928252
>> 285:363928252
>> 285:363928252
>> --------------------
>> 285:363974918
>> 285:363974917
>> --------------------
>> 285:363974917
>> 285:363974917
>> 285:363974917
>> 285:363974917
>> 285:363974917
>> 285:363974917
>> 285:363974917
>> 285:363975751
>> 285:363975751
>> 285:363975751
>> 285:363975751
>> 285:363975751
>> 285:363975751
>>
>> As you can see between the two lines "--------------",
>> CLOCK_MONOTONIC_RAW went backwards by one nanosecond.
>


Hey Daniel,
   Yea. Apologies for the duplicated debugging effort you've done. You
diagnosed the problem properly.

I've already got patches to fix up the core handling and am testing a
patch Will provided to correct the vdso. I'll add you to the cc when I
send to the list.

Will: This was all done on the original HiKey. I can reproduce it with
4.12-rc2 (as well as 4.9). It looks like your vDSO patch is working
properly.

thanks
-john

[PATCH v2 0/2] arm64: Add support for CLOCK_MONOTONIC_RAW in clock_gettime() vDSO

[Kevin Brodsky]

Hi,

This small series adds support for CLOCK_MONOTONIC_RAW to
clock_gettime() (and clock_getres()) in the arm64 vDSO. This should make
calls to clock_gettime() with CLOCK_MONOTONIC_RAW just as fast as
CLOCK_MONOTONIC and suchlike, instead of falling back to the significantly
slower syscall.

Given that gettimeofday.S is already quite difficult to understand, and
directly adding MONOTONIC_RAW support makes it even more so, the first
patch refactors it to make it simpler, and save a few instructions along
the way. The second patch actually adds support for MONOTONIC_RAW, which
is made quite easy by the refactoring.

Thanks,
Kevin

Changelog v1..v2:
* As proposed by Will Deacon, align main labels to be nicer to branch
  predictors. Also replace the "store" exit point of
  __kernel_clock_gettime with a macro, making it slightly easier to read
  and removing a branch.

Kevin Brodsky (2):
  arm64: Refactor vDSO time functions
  arm64: Add support for CLOCK_MONOTONIC_RAW in clock_gettime() vDSO

 arch/arm64/include/asm/vdso_datapage.h |   8 +-
 arch/arm64/kernel/asm-offsets.c        |   6 +-
 arch/arm64/kernel/vdso.c               |   8 +-
 arch/arm64/kernel/vdso/gettimeofday.S  | 331 ++++++++++++++++++++-------------
 4 files changed, 224 insertions(+), 129 deletions(-)

-- 
2.9.0

[PATCH v2 2/2] arm64: Add support for CLOCK_MONOTONIC_RAW in clock_gettime() vDSO

[Kevin Brodsky]

So far the arm64 clock_gettime() vDSO implementation only supported
the following clocks, falling back to the syscall for the others:
- CLOCK_REALTIME{,_COARSE}
- CLOCK_MONOTONIC{,_COARSE}

This patch adds support for the CLOCK_MONOTONIC_RAW clock, taking
advantage of the recent refactoring of the vDSO time functions. Like
the non-_COARSE clocks, this only works when the "arch_sys_counter"
clocksource is in use (allowing us to read the current time from the
virtual counter register), otherwise we also have to fall back to the
syscall.

Most of the data is shared with CLOCK_MONOTONIC, and the algorithm is
similar. The reference implementation in kernel/time/timekeeping.c
shows that:
- CLOCK_MONOTONIC = tk->wall_to_monotonic + tk->xtime_sec +
  timekeeping_get_ns(&tk->tkr_mono)
- CLOCK_MONOTONIC_RAW = tk->raw_time + timekeeping_get_ns(&tk->tkr_raw)
- tkr_mono and tkr_raw are identical (in particular, same
  clocksource), except these members:
  * mult (only mono's multiplier is NTP-adjusted)
  * xtime_nsec (always 0 for raw)

Therefore, tk->raw_time and tkr_raw->mult are now also stored in the
vDSO data page.

Cc: Ali Saidi <ali.saidi@arm.com>
Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
Reviewed-by: Dave Martin <dave.martin@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/vdso_datapage.h |  8 +++--
 arch/arm64/kernel/asm-offsets.c        |  6 +++-
 arch/arm64/kernel/vdso.c               |  8 ++++-
 arch/arm64/kernel/vdso/gettimeofday.S  | 57 +++++++++++++++++++++++++++-------
 4 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/include/asm/vdso_datapage.h b/arch/arm64/include/asm/vdso_datapage.h
index de66199673d7..2b9a63771eda 100644
--- a/arch/arm64/include/asm/vdso_datapage.h
+++ b/arch/arm64/include/asm/vdso_datapage.h
@@ -22,6 +22,8 @@
 
 struct vdso_data {
 	__u64 cs_cycle_last;	/* Timebase at clocksource init */
+	__u64 raw_time_sec;	/* Raw time */
+	__u64 raw_time_nsec;
 	__u64 xtime_clock_sec;	/* Kernel time */
 	__u64 xtime_clock_nsec;
 	__u64 xtime_coarse_sec;	/* Coarse time */
@@ -29,8 +31,10 @@ struct vdso_data {
 	__u64 wtm_clock_sec;	/* Wall to monotonic time */
 	__u64 wtm_clock_nsec;
 	__u32 tb_seq_count;	/* Timebase sequence counter */
-	__u32 cs_mult;		/* Clocksource multiplier */
-	__u32 cs_shift;		/* Clocksource shift */
+	/* cs_* members must be adjacent and in this order (ldp accesses) */
+	__u32 cs_mono_mult;	/* NTP-adjusted clocksource multiplier */
+	__u32 cs_shift;		/* Clocksource shift (mono = raw) */
+	__u32 cs_raw_mult;	/* Raw clocksource multiplier */
 	__u32 tz_minuteswest;	/* Whacky timezone stuff */
 	__u32 tz_dsttime;
 	__u32 use_syscall;
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 2f4ba774488a..c88508b95f87 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -78,6 +78,7 @@ int main(void)
   BLANK();
   DEFINE(CLOCK_REALTIME,	CLOCK_REALTIME);
   DEFINE(CLOCK_MONOTONIC,	CLOCK_MONOTONIC);
+  DEFINE(CLOCK_MONOTONIC_RAW,	CLOCK_MONOTONIC_RAW);
   DEFINE(CLOCK_REALTIME_RES,	MONOTONIC_RES_NSEC);
   DEFINE(CLOCK_REALTIME_COARSE,	CLOCK_REALTIME_COARSE);
   DEFINE(CLOCK_MONOTONIC_COARSE,CLOCK_MONOTONIC_COARSE);
@@ -85,6 +86,8 @@ int main(void)
   DEFINE(NSEC_PER_SEC,		NSEC_PER_SEC);
   BLANK();
   DEFINE(VDSO_CS_CYCLE_LAST,	offsetof(struct vdso_data, cs_cycle_last));
+  DEFINE(VDSO_RAW_TIME_SEC,	offsetof(struct vdso_data, raw_time_sec));
+  DEFINE(VDSO_RAW_TIME_NSEC,	offsetof(struct vdso_data, raw_time_nsec));
   DEFINE(VDSO_XTIME_CLK_SEC,	offsetof(struct vdso_data, xtime_clock_sec));
   DEFINE(VDSO_XTIME_CLK_NSEC,	offsetof(struct vdso_data, xtime_clock_nsec));
   DEFINE(VDSO_XTIME_CRS_SEC,	offsetof(struct vdso_data, xtime_coarse_sec));
@@ -92,7 +95,8 @@ int main(void)
   DEFINE(VDSO_WTM_CLK_SEC,	offsetof(struct vdso_data, wtm_clock_sec));
   DEFINE(VDSO_WTM_CLK_NSEC,	offsetof(struct vdso_data, wtm_clock_nsec));
   DEFINE(VDSO_TB_SEQ_COUNT,	offsetof(struct vdso_data, tb_seq_count));
-  DEFINE(VDSO_CS_MULT,		offsetof(struct vdso_data, cs_mult));
+  DEFINE(VDSO_CS_MONO_MULT,	offsetof(struct vdso_data, cs_mono_mult));
+  DEFINE(VDSO_CS_RAW_MULT,	offsetof(struct vdso_data, cs_raw_mult));
   DEFINE(VDSO_CS_SHIFT,		offsetof(struct vdso_data, cs_shift));
   DEFINE(VDSO_TZ_MINWEST,	offsetof(struct vdso_data, tz_minuteswest));
   DEFINE(VDSO_TZ_DSTTIME,	offsetof(struct vdso_data, tz_dsttime));
diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 9fefb005812a..076312b17d4f 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -214,10 +214,16 @@ void update_vsyscall(struct timekeeper *tk)
 	vdso_data->wtm_clock_nsec		= tk->wall_to_monotonic.tv_nsec;
 
 	if (!use_syscall) {
+		/* tkr_mono.cycle_last == tkr_raw.cycle_last */
 		vdso_data->cs_cycle_last	= tk->tkr_mono.cycle_last;
+		vdso_data->raw_time_sec		= tk->raw_time.tv_sec;
+		vdso_data->raw_time_nsec	= tk->raw_time.tv_nsec;
 		vdso_data->xtime_clock_sec	= tk->xtime_sec;
 		vdso_data->xtime_clock_nsec	= tk->tkr_mono.xtime_nsec;
-		vdso_data->cs_mult		= tk->tkr_mono.mult;
+		/* tkr_raw.xtime_nsec == 0 */
+		vdso_data->cs_mono_mult		= tk->tkr_mono.mult;
+		vdso_data->cs_raw_mult		= tk->tkr_raw.mult;
+		/* tkr_mono.shift == tkr_raw.shift */
 		vdso_data->cs_shift		= tk->tkr_mono.shift;
 	}
 
diff --git a/arch/arm64/kernel/vdso/gettimeofday.S b/arch/arm64/kernel/vdso/gettimeofday.S
index c06919ee29e1..e00b4671bd7c 100644
--- a/arch/arm64/kernel/vdso/gettimeofday.S
+++ b/arch/arm64/kernel/vdso/gettimeofday.S
@@ -87,6 +87,15 @@ x_tmp		.req	x8
 	msub	\res_nsec, x_tmp, \nsec_to_sec, \res_nsec
 	.endm
 
+	/*
+	 * Returns in res_{sec,nsec} the timespec based on the clock_raw delta,
+	 * used for CLOCK_MONOTONIC_RAW.
+	 */
+	.macro	get_ts_clock_raw res_sec, res_nsec, clock_nsec, nsec_to_sec
+	udiv	\res_sec, \clock_nsec, \nsec_to_sec
+	msub	\res_nsec, \res_sec, \nsec_to_sec, \clock_nsec
+	.endm
+
 	/* sec and nsec are modified in place. */
 	.macro add_ts sec, nsec, ts_sec, ts_nsec, nsec_to_sec
 	/* Add timespec. */
@@ -135,7 +144,8 @@ ENTRY(__kernel_gettimeofday)
 1:	seqcnt_acquire
 	syscall_check fail=4f
 	ldr	x10, [vdso_data, #VDSO_CS_CYCLE_LAST]
-	ldp	w11, w12, [vdso_data, #VDSO_CS_MULT]
+	/* w11 = cs_mono_mult, w12 = cs_shift */
+	ldp	w11, w12, [vdso_data, #VDSO_CS_MONO_MULT]
 	ldp	x13, x14, [vdso_data, #VDSO_XTIME_CLK_SEC]
 	seqcnt_check fail=1b
 
@@ -172,20 +182,20 @@ ENDPROC(__kernel_gettimeofday)
 /* int __kernel_clock_gettime(clockid_t clock_id, struct timespec *tp); */
 ENTRY(__kernel_clock_gettime)
 	.cfi_startproc
-	cmp 	w0, #JUMPSLOT_MAX
-	b.hi 	syscall
+	cmp	w0, #JUMPSLOT_MAX
+	b.hi	syscall
 	adr	vdso_data, _vdso_data
-	adr 	x_tmp, jumptable
-	add 	x_tmp, x_tmp, w0, uxtw #2
-	br 	x_tmp
+	adr	x_tmp, jumptable
+	add	x_tmp, x_tmp, w0, uxtw #2
+	br	x_tmp
 
 	ALIGN
 jumptable:
 	jump_slot jumptable, CLOCK_REALTIME, realtime
 	jump_slot jumptable, CLOCK_MONOTONIC, monotonic
-	b 	syscall
-	b 	syscall
-	b 	syscall
+	b	syscall
+	b	syscall
+	jump_slot jumptable, CLOCK_MONOTONIC_RAW, monotonic_raw
 	jump_slot jumptable, CLOCK_REALTIME_COARSE, realtime_coarse
 	jump_slot jumptable, CLOCK_MONOTONIC_COARSE, monotonic_coarse
 
@@ -198,7 +208,8 @@ realtime:
 	seqcnt_acquire
 	syscall_check fail=syscall
 	ldr	x10, [vdso_data, #VDSO_CS_CYCLE_LAST]
-	ldp	w11, w12, [vdso_data, #VDSO_CS_MULT]
+	/* w11 = cs_mono_mult, w12 = cs_shift */
+	ldp	w11, w12, [vdso_data, #VDSO_CS_MONO_MULT]
 	ldp	x13, x14, [vdso_data, #VDSO_XTIME_CLK_SEC]
 	seqcnt_check fail=realtime
 
@@ -216,7 +227,8 @@ monotonic:
 	seqcnt_acquire
 	syscall_check fail=syscall
 	ldr	x10, [vdso_data, #VDSO_CS_CYCLE_LAST]
-	ldp	w11, w12, [vdso_data, #VDSO_CS_MULT]
+	/* w11 = cs_mono_mult, w12 = cs_shift */
+	ldp	w11, w12, [vdso_data, #VDSO_CS_MONO_MULT]
 	ldp	x13, x14, [vdso_data, #VDSO_XTIME_CLK_SEC]
 	ldp	x3, x4, [vdso_data, #VDSO_WTM_CLK_SEC]
 	seqcnt_check fail=monotonic
@@ -234,6 +246,28 @@ monotonic:
 	clock_gettime_return, shift=1
 
 	ALIGN
+monotonic_raw:
+	seqcnt_acquire
+	syscall_check fail=syscall
+	ldr	x10, [vdso_data, #VDSO_CS_CYCLE_LAST]
+	/* w11 = cs_raw_mult, w12 = cs_shift */
+	ldp	w12, w11, [vdso_data, #VDSO_CS_SHIFT]
+	ldp	x13, x14, [vdso_data, #VDSO_RAW_TIME_SEC]
+	seqcnt_check fail=monotonic_raw
+
+	/* All computations are done with left-shifted nsecs. */
+	lsl	x14, x14, x12
+	get_nsec_per_sec res=x9
+	lsl	x9, x9, x12
+
+	get_clock_shifted_nsec res=x15, cycle_last=x10, mult=x11
+	get_ts_clock_raw res_sec=x10, res_nsec=x11, \
+		clock_nsec=x15, nsec_to_sec=x9
+
+	add_ts sec=x10, nsec=x11, ts_sec=x13, ts_nsec=x14, nsec_to_sec=x9
+	clock_gettime_return, shift=1
+
+	ALIGN
 realtime_coarse:
 	seqcnt_acquire
 	ldp	x10, x11, [vdso_data, #VDSO_XTIME_CRS_SEC]
@@ -265,6 +299,7 @@ ENTRY(__kernel_clock_getres)
 	.cfi_startproc
 	cmp	w0, #CLOCK_REALTIME
 	ccmp	w0, #CLOCK_MONOTONIC, #0x4, ne
+	ccmp	w0, #CLOCK_MONOTONIC_RAW, #0x4, ne
 	b.ne	1f
 
 	ldr	x2, 5f
-- 
2.9.0