Skip to contentSkip to conversation

x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

PATCH In review Jason Vas Dias 2 participants 4 messages Mar 11, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

Currently the VDSO does not handle
clock_gettime( CLOCK_MONOTONIC_RAW, &ts )
on Intel / AMD - it calls
vdso_fallback_gettime()
for this clock, which issues a syscall, having an unacceptably high latency (minimum measurable time or time between measurements) of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C machines under various versions of Linux.

This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO , by exporting the raw clock calibration, last cycles, last xtime_nsec, and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() .

Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns on average, and the test program:
tools/testing/selftest/timers/inconsistency-check.c
succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value.

The patch is against Linus' latest 4.16-rc4 tree, current HEAD of :
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
.

The patch affects only files:

arch/x86/include/asm/vgtod.h
arch/x86/entry/vdso/vclock_gettime.c
arch/x86/entry/vsyscall/vsyscall_gtod.c

Best Regards,
Jason Vas Dias .

---

arch/x86/entry/vdso/vclock_gettime.c renamed from arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 +69 -0
diff -up linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 linux-4.16-rc4/arch/x86/entry/vdso/vclock_gettime.c
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
 	return last;
 }
 
+notrace static u64 vread_tsc_raw(void)
+{
+        u64 tsc, last=gtod->raw_cycle_last;
+        if( likely( gtod->has_rdtscp ) ) {
+                u32     tsc_lo, tsc_hi,
+                        tsc_cpu __attribute__((unused));
+                asm volatile
+                ( "rdtscp"
+                /* ^- has built-in cancellation point / pipeline stall "barrier" */
+                : "=a" (tsc_lo)
+                , "=d" (tsc_hi)
+                , "=c" (tsc_cpu)
+                ); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx                 
+                tsc      = ((((u64)tsc_hi) & 0xffffffffUL) << 32) | (((u64)tsc_lo) & 0xffffffffUL);
+        } else {
+                tsc      = rdtsc_ordered();
+        }
+	if (likely(tsc >= last))
+		return tsc;        
+        asm volatile ("");                                
+        return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
 	u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *m
 	return v * gtod->mult;
 }
 
+notrace static inline u64 vgetsns_raw(int *mode)
+{
+	u64 v;
+	cycles_t cycles;
+
+	if (gtod->vclock_mode == VCLOCK_TSC)
+		cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+	else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+		cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+	else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+		cycles = vread_hvclock(mode);
+#endif
+	else
+		return 0;
+	v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+	return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_mo
 	return mode;
 }
 
+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+	unsigned long seq;
+	u64 ns;
+	int mode;
+
+	do {
+		seq = gtod_read_begin(gtod);
+		mode = gtod->vclock_mode;
+		ts->tv_sec = gtod->monotonic_time_raw_sec;
+		ns = gtod->monotonic_time_raw_nsec;
+		ns += vgetsns_raw(&mode);
+		ns >>= gtod->raw_shift;
+	} while (unlikely(gtod_read_retry(gtod, seq)));
+
+	ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
+	ts->tv_nsec = ns;
+
+	return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
 	unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid
 		if (do_monotonic(ts) == VCLOCK_NONE)
 			goto fallback;
 		break;
+	case CLOCK_MONOTONIC_RAW:
+		if (do_monotonic_raw(ts) == VCLOCK_NONE)
+			goto fallback;
+		break;
 	case CLOCK_REALTIME_COARSE:
 		do_realtime_coarse(ts);
 		break;
arch/x86/entry/vsyscall/vsyscall_gtod.c renamed from arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 +10 -0
diff -up linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c.4.16-rc4 linux-4.16-rc4/arch/x86/entry/vsyscall/vsyscall_gtod.c
@@ -16,6 +16,7 @@
 #include <linux/timekeeper_internal.h>
 #include <asm/vgtod.h>
 #include <asm/vvar.h>
+#include <asm/cpufeature.h>
 
 int vclocks_used __read_mostly;
 
@@ -45,6 +46,12 @@ void update_vsyscall(struct timekeeper *
 	vdata->mult		= tk->tkr_mono.mult;
 	vdata->shift		= tk->tkr_mono.shift;
 
+	vdata->raw_cycle_last   = tk->tkr_raw.cycle_last;
+	vdata->raw_mask         = tk->tkr_raw.mask;
+	vdata->raw_mult         = tk->tkr_raw.mult;
+	vdata->raw_shift	= tk->tkr_raw.shift;
+	vdata->has_rdtscp	= static_cpu_has(X86_FEATURE_RDTSCP);
+
 	vdata->wall_time_sec		= tk->xtime_sec;
 	vdata->wall_time_snsec		= tk->tkr_mono.xtime_nsec;
 
@@ -74,5 +81,8 @@ void update_vsyscall(struct timekeeper *
 		vdata->monotonic_time_coarse_sec++;
 	}
 
+	vdata->monotonic_time_raw_sec  = tk->raw_sec;
+	vdata->monotonic_time_raw_nsec = tk->tkr_raw.xtime_nsec;
+
 	gtod_write_end(vdata);
 }
arch/x86/include/asm/vgtod.h renamed from arch/x86/include/asm/vgtod.h.4.16-rc4 +7 -0
diff -up linux-4.16-rc4/arch/x86/include/asm/vgtod.h.4.16-rc4 linux-4.16-rc4/arch/x86/include/asm/vgtod.h
@@ -22,6 +22,11 @@ struct vsyscall_gtod_data {
 	u64	mask;
 	u32	mult;
 	u32	shift;
+	u64	raw_cycle_last;
+	u64     raw_mask;
+	u32     raw_mult;
+	u32     raw_shift;
+	u32     has_rdtscp;
 
 	/* open coded 'struct timespec' */
 	u64		wall_time_snsec;
@@ -32,6 +37,8 @@ struct vsyscall_gtod_data {
 	gtod_long_t	wall_time_coarse_nsec;
 	gtod_long_t	monotonic_time_coarse_sec;
 	gtod_long_t	monotonic_time_coarse_nsec;
+	gtod_long_t	monotonic_time_raw_sec;
+	gtod_long_t	monotonic_time_raw_nsec;
 
 	int		tz_minuteswest;
 	int		tz_dsttime;

---

#1 lore raw

On Sun, 11 Mar 2018, Jason Vas Dias wrote:

This looks better now. Though running that patch through checkpatch.pl results in:

total: 28 errors, 20 warnings, 139 lines checked

....

Quoted 1 line

+notrace static u64 vread_tsc_raw(void)

Why do you need a separate function? I asked you to use vread_tsc(). So you might have reasons for doing that, but please then explain WHY and not just throw the stuff in my direction w/o any comment.

arch/x86/entry/vdso/vclock_gettime.c notrace static u64 vread_tsc(void) · line 198

Quoted 13 lines
arch/x86/entry/vdso/vclock_gettime.c renamed from arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 +13 -0
+{
+        u64 tsc, last=gtod->raw_cycle_last;
+        if( likely( gtod->has_rdtscp ) ) {
+                u32     tsc_lo, tsc_hi,
+                        tsc_cpu __attribute__((unused));
+                asm volatile
+                ( "rdtscp"
+                /* ^- has built-in cancellation point / pipeline stall "barrier" */
+                : "=a" (tsc_lo)
+                , "=d" (tsc_hi)
+                , "=c" (tsc_cpu)
+                ); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx                 
+                tsc      = ((((u64)tsc_hi) & 0xffffffffUL) << 32) | (((u64)tsc_lo) & 0xffffffffUL);

This is not required to make the vdso accessor for monotonic raw work.

If at all then the rdtscp support wants to be in a separate patch with a proper explanation.

Aside of that the code for rdtscp wants to be in a proper inline helper in the relevant header file and written according to the coding style the kernel uses for asm inlines.

arch/x86/entry/vdso/vclock_gettime.c notrace static u64 vread_tsc(void) · line 206

Quoted 8 lines
arch/x86/entry/vdso/vclock_gettime.c renamed from arch/x86/entry/vdso/vclock_gettime.c.4.16-rc4 +8 -0
+        } else {
+                tsc      = rdtsc_ordered();
+        }
+	if (likely(tsc >= last))
+		return tsc;        
+        asm volatile ("");                                
+        return last;
+}

The rest looks ok.

Thanks,

tglx

#2 lore raw

Thanks Thomas -

Quoted 7 lines · Thomas Gleixner

On Sun, 11 Mar 2018, Jason Vas Dias wrote:

This looks better now. Though running that patch through checkpatch.pl results in:

total: 28 errors, 20 warnings, 139 lines checked

Hmm, I was unaware of that script, I'll run and find out why - probably because whitespace is not visible in emacs with my monospace font and it is very difficult to see if tabs are used if somehow a '\t\ ' or ' \t' has slipped in .

I'll run the script, fix the errors. and repost.

Quoted 8 lines

....

Quoted 1 line

+notrace static u64 vread_tsc_raw(void)

Why do you need a separate function? I asked you to use vread_tsc(). So you might have reasons for doing that, but please then explain WHY and not just throw the stuff in my direction w/o any comment.

mainly, because vread_tsc() makes its comparison against gtod->cycles_last , a copy of tk->tkr_mono.cycle_last, while vread_tsc_raw() uses gtod->raw_cycle_last, a copy of tk->tkr_raw.cycle_last .

And rdtscp has a built-in "barrier", as the comments explain, making rdtsc_ordered()'s 'barrier()' unnecessary .

Quoted 19 lines
Quoted 13 lines
(quoted diff) +8 -0
+{
+        u64 tsc, last=gtod->raw_cycle_last;
+        if( likely( gtod->has_rdtscp ) ) {
+                u32     tsc_lo, tsc_hi,
+                        tsc_cpu __attribute__((unused));
+                asm volatile
+                ( "rdtscp"
+                /* ^- has built-in cancellation point / pipeline stall

"barrier" */

(quoted diff) +4 -0
+                : "=a" (tsc_lo)
+                , "=d" (tsc_hi)
+                , "=c" (tsc_cpu)
+                ); // since all variables 32-bit, eax, edx, ecx used -
Quoted 3 lines

NOT rax, rdx, rcx
+ tsc = ((((u64)tsc_hi) & 0xffffffffUL) << 32) |
(((u64)tsc_lo) & 0xffffffffUL);

This is not required to make the vdso accessor for monotonic raw work.

Quoted 9 lines

If at all then the rdtscp support wants to be in a separate patch with a proper explanation.

Aside of that the code for rdtscp wants to be in a proper inline helper in the relevant header file and written according to the coding style the kernel uses for asm inlines.

Sorry, I will put the function in the same header as rdtsc_ordered () , in a separate patch.

Quoted 6 lines

The rest looks ok.

Thanks,

tglx

I'll re-generate patches and resend .

A complete patch , against 4.15.9, is attached , that I am using , including a suggested '__vdso_linux_tsc_calibration()' function and arch/x86/include/uapi/asm/vdso_tsc_calibration.h file that does not return any pointers into the VDSO .

Presuming this was split into separate patches as you suggest, and was against the latest HEAD branch (4.16-rcX), would it be OK to include the vdso_linux_tsc_calibration() work ?
It does enable user space code to develop accurate TSC readers which are free to use different structures and pico-second resolution. The actual user-space clock_gettime(CLOCK_MONOTONIC_RAW) replacement I am using for work just reads the TSC , with a latency of < 8ns, and uses the linux_tsc_calibration to convert using floating-point as required.

Thanks & Regards,
Jason

arch/x86/entry/vdso/vclock_gettime.c +90 -1
index f19856d..050aea7 100644
@@ -21,6 +21,7 @@
 #include <linux/math64.h>
 #include <linux/time.h>
 #include <linux/kernel.h>
+#include <uapi/asm/vdso_tsc_calibration.h>
 
 #define gtod (&VVAR(vsyscall_gtod_data))
 
@@ -57,7 +58,6 @@ notrace static long vdso_fallback_gtod(struct timeval *tv, struct timezone *tz)
 	return ret;
 }
 
-
 #else
 
 notrace static long vdso_fallback_gettime(long clock, struct timespec *ts)
@@ -182,6 +182,29 @@ notrace static u64 vread_tsc(void)
 	return last;
 }
 
+notrace static u64 vread_tsc_raw(void)
+{
+        u64 tsc, last=gtod->raw_cycle_last;
+        if( likely( gtod->has_rdtscp ) ) {
+                u32     tsc_lo, tsc_hi,
+                        tsc_cpu __attribute__((unused));
+                asm volatile
+                ( "rdtscp"
+                /* ^- has built-in cancellation point / pipeline stall "barrier" */
+                : "=a" (tsc_lo)
+                , "=d" (tsc_hi)
+                , "=c" (tsc_cpu)
+                ); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx
+                tsc      = ((((u64)tsc_hi) & 0xffffffffUL) << 32) | (((u64)tsc_lo) & 0xffffffffUL);
+        } else {
+                tsc      = rdtsc_ordered();
+        }
+	if (likely(tsc >= last))
+		return tsc;
+        asm volatile ("");
+        return last;
+}
+
 notrace static inline u64 vgetsns(int *mode)
 {
 	u64 v;
@@ -203,6 +226,27 @@ notrace static inline u64 vgetsns(int *mode)
 	return v * gtod->mult;
 }
 
+notrace static inline u64 vgetsns_raw(int *mode)
+{
+	u64 v;
+	cycles_t cycles;
+
+	if (gtod->vclock_mode == VCLOCK_TSC)
+		cycles = vread_tsc_raw();
+#ifdef CONFIG_PARAVIRT_CLOCK
+	else if (gtod->vclock_mode == VCLOCK_PVCLOCK)
+		cycles = vread_pvclock(mode);
+#endif
+#ifdef CONFIG_HYPERV_TSCPAGE
+	else if (gtod->vclock_mode == VCLOCK_HVCLOCK)
+		cycles = vread_hvclock(mode);
+#endif
+	else
+		return 0;
+	v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
+	return v * gtod->raw_mult;
+}
+
 /* Code size doesn't matter (vdso is 4k anyway) and this is faster. */
 notrace static int __always_inline do_realtime(struct timespec *ts)
 {
@@ -246,6 +290,27 @@ notrace static int __always_inline do_monotonic(struct timespec *ts)
 	return mode;
 }
 
+notrace static int __always_inline do_monotonic_raw( struct timespec *ts)
+{
+	unsigned long seq;
+	u64 ns;
+	int mode;
+
+	do {
+		seq = gtod_read_begin(gtod);
+		mode = gtod->vclock_mode;
+		ts->tv_sec = gtod->monotonic_time_raw_sec;
+		ns = gtod->monotonic_time_raw_nsec;
+		ns += vgetsns_raw(&mode);
+		ns >>= gtod->raw_shift;
+	} while (unlikely(gtod_read_retry(gtod, seq)));
+
+	ts->tv_sec += __iter_div_u64_rem(ns, NSEC_PER_SEC, &ns);
+	ts->tv_nsec = ns;
+
+	return mode;
+}
+
 notrace static void do_realtime_coarse(struct timespec *ts)
 {
 	unsigned long seq;
@@ -277,6 +342,10 @@ notrace int __vdso_clock_gettime(clockid_t clock, struct timespec *ts)
 		if (do_monotonic(ts) == VCLOCK_NONE)
 			goto fallback;
 		break;
+	case CLOCK_MONOTONIC_RAW:
+		if (do_monotonic_raw(ts) == VCLOCK_NONE)
+			goto fallback;
+		break;
 	case CLOCK_REALTIME_COARSE:
 		do_realtime_coarse(ts);
 		break;
@@ -326,3 +395,23 @@ notrace time_t __vdso_time(time_t *t)
 }
 time_t time(time_t *t)
 	__attribute__((weak, alias("__vdso_time")));
+
+extern unsigned
+__vdso_linux_tsc_calibration(struct linux_tsc_calibration *);
+
+notrace	unsigned
+__vdso_linux_tsc_calibration(struct linux_tsc_calibration *tsc_cal)
+{
+	if ( (gtod->vclock_mode == VCLOCK_TSC) && (tsc_cal != ((void*)0UL)) )
+	{
+		tsc_cal -> tsc_khz = gtod->tsc_khz;
+		tsc_cal -> mult    = gtod->raw_mult;
+		tsc_cal -> shift   = gtod->raw_shift;
+		return 1;
+	}
+	return 0;
+}
+
+const struct linux_tsc_calibration * linux_tsc_calibration(void)
+	__attribute((weak, alias("__vdso_linux_tsc_calibration")));
+
arch/x86/entry/vdso/vdso.lds.S +2 -0
index d3a2dce..e0b5cce 100644
@@ -25,6 +25,8 @@ VERSION {
 		__vdso_getcpu;
 		time;
 		__vdso_time;
+		linux_tsc_calibration;
+		__vdso_linux_tsc_calibration;
 	local: *;
 	};
 }
arch/x86/entry/vdso/vdso32/vdso32.lds.S +1 -0
index 422764a..17fd07f 100644
@@ -26,6 +26,7 @@ VERSION
 		__vdso_clock_gettime;
 		__vdso_gettimeofday;
 		__vdso_time;
+		__vdso_linux_tsc_calibration;
 	};
 
 	LINUX_2.5 {
arch/x86/entry/vdso/vdsox32.lds.S +1 -0
index 05cd1c5..b69b0bf 100644
@@ -21,6 +21,7 @@ VERSION {
 		__vdso_gettimeofday;
 		__vdso_getcpu;
 		__vdso_time;
+		__vdso_linux_tsc_calibration ;
 	local: *;
 	};
 }
arch/x86/entry/vsyscall/vsyscall_gtod.c +13 -0
index e1216dd..5fc49b9b 100644
@@ -16,11 +16,14 @@
 #include <linux/timekeeper_internal.h>
 #include <asm/vgtod.h>
 #include <asm/vvar.h>
+#include <asm/cpufeature.h>
 
 int vclocks_used __read_mostly;
 
 DEFINE_VVAR(struct vsyscall_gtod_data, vsyscall_gtod_data);
 
+extern unsigned int tsc_khz;
+
 void update_vsyscall_tz(void)
 {
 	vsyscall_gtod_data.tz_minuteswest = sys_tz.tz_minuteswest;
@@ -45,6 +48,12 @@ void update_vsyscall(struct timekeeper *tk)
 	vdata->mult		= tk->tkr_mono.mult;
 	vdata->shift		= tk->tkr_mono.shift;
 
+	vdata->raw_cycle_last   = tk->tkr_raw.cycle_last;
+	vdata->raw_mask         = tk->tkr_raw.mask;
+	vdata->raw_mult         = tk->tkr_raw.mult;
+	vdata->raw_shift	= tk->tkr_raw.shift;
+	vdata->has_rdtscp	= static_cpu_has(X86_FEATURE_RDTSCP);
+
 	vdata->wall_time_sec		= tk->xtime_sec;
 	vdata->wall_time_snsec		= tk->tkr_mono.xtime_nsec;
 
@@ -74,5 +83,9 @@ void update_vsyscall(struct timekeeper *tk)
 		vdata->monotonic_time_coarse_sec++;
 	}
 
+	vdata->monotonic_time_raw_sec  = tk->raw_sec;
+	vdata->monotonic_time_raw_nsec = tk->tkr_raw.xtime_nsec;
+	vdata->tsc_khz = tsc_khz;
+
 	gtod_write_end(vdata);
 }
arch/x86/include/asm/vgtod.h +8 -1
index fb856c9..4c474fa 100644
@@ -22,7 +22,12 @@ struct vsyscall_gtod_data {
 	u64	mask;
 	u32	mult;
 	u32	shift;
-
+	u64	raw_cycle_last;
+	u64	raw_mask;
+	u32	raw_mult;
+	u32	raw_shift;
+	u32	has_rdtscp;
+	u32     tsc_khz;
 	/* open coded 'struct timespec' */
 	u64		wall_time_snsec;
 	gtod_long_t	wall_time_sec;
@@ -32,6 +37,8 @@ struct vsyscall_gtod_data {
 	gtod_long_t	wall_time_coarse_nsec;
 	gtod_long_t	monotonic_time_coarse_sec;
 	gtod_long_t	monotonic_time_coarse_nsec;
+	gtod_long_t	monotonic_time_raw_sec;
+	gtod_long_t	monotonic_time_raw_nsec;
 
 	int		tz_minuteswest;
 	int		tz_dsttime;
arch/x86/include/uapi/asm/vdso_tsc_calibration.h new file +73 -0

73 lines not on this page · raw message

#3 lore raw
Quoted 10 lines · Jason Vas Dias
Quoted 7 lines · Thomas Gleixner

On Sun, 11 Mar 2018, Jason Vas Dias wrote:

This looks better now. Though running that patch through checkpatch.pl results in:

total: 28 errors, 20 warnings, 139 lines checked

Hmm, I was unaware of that script, I'll run and find out why -

Documentation/process/* which I recommended for reading before definitely mentions it.

Quoted 3 lines

probably because whitespace is not visible in emacs with my monospace font and it is very difficult to see if tabs are used if somehow a '\t\ ' or ' \t' has slipped in .

It's not only about whitespace ....

Quoted 10 lines
Quoted 6 lines
Quoted 1 line

+notrace static u64 vread_tsc_raw(void)

Why do you need a separate function? I asked you to use vread_tsc(). So you might have reasons for doing that, but please then explain WHY and not just throw the stuff in my direction w/o any comment.

mainly, because vread_tsc() makes its comparison against gtod->cycles_last , a copy of tk->tkr_mono.cycle_last, while vread_tsc_raw() uses gtod->raw_cycle_last, a copy of tk->tkr_raw.cycle_last .

Well, if you look at the timekeeping core then you'll notice that it's actually the same value. It's updated in both tkr_mono and tkr_raw so its available when either of the tkr_* structs is handed to the relevant functions.

Quoted 4 lines

A complete patch , against 4.15.9, is attached , that I am using , including a suggested '__vdso_linux_tsc_calibration()' function and arch/x86/include/uapi/asm/vdso_tsc_calibration.h file that does not return any pointers into the VDSO .

I'm not going to look at attached complete patches. The development process is well defined for a reason and it's not optional.

Thanks,

tglx