mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/3] vdso: Keep the CLOCK_AUX base at full precision
@ 2026-08-31 12:55 Zhan Xusheng
  2026-08-31 12:55 ` [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem() Zhan Xusheng
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Zhan Xusheng @ 2026-08-31 12:55 UTC (permalink / raw)
  To: tglx, luto, vincenzo.frascino
  Cc: thomas.weissschuh, linux-kernel, zhanxusheng

Thomas confirmed the CLOCK_AUX base precision loss was unintentional and
asked for the fix in this shape:

  https://lore.kernel.org/all/20260831102924-5f7570f6-7dc4-4996-9a35-db9c9f577dd2@linutronix.de

1/3 adds __iter_div64_u64_rem() instead of open-coding the iterative
division a third time, and converts the two existing sites.

2/3 is the fix.  Adding the offset in scaled nanoseconds needs the
normalisation to happen against NSEC_PER_SEC << shift in the tick, so that
the stored base stays below one second and the userspace fast-path does not
iterate more.

3/3 is independent and can be dropped on its own.  The assert is on a
parameter, so it depends on the compiler deriving the range from the
vdso_clockid_valid() bail-out; gcc 13 and clang 18 both manage it, and it
does fire once MAX_AUX_CLOCKS goes past the limit, so it is not vacuous.

Checked under QEMU with an auxiliary clock enabled through
/sys/kernel/time/aux_clocks/0/aux_clock_enable, comparing CLOCK_AUX via
the vDSO against the raw syscall at offset 0, at +5.123456789 s, and with
offs_aux driven negative.  The two agree within read latency in all three,
and over 100000 interleaved pairs each the vDSO reading is never ahead of
a syscall reading taken after it.

The 1 ns bias itself is not measurable that way: a pair of reads costs
several hundred nanoseconds, so it cannot be separated from read latency.
The argument for it is the algebra in 2/3, plus a sweep over
(xtime_nsec, delta) which puts the 1 ns case at 60% for shift 24.

Zhan Xusheng (3):
  vdso/math64: Add and use __iter_div64_u64_rem()
  vdso/vsyscall: Keep the CLOCK_AUX base scaled
  vdso/gettimeofday: Assert that the clock id fits the dispatch mask

 include/vdso/math64.h   | 21 +++++++++++++++++++++
 kernel/time/vsyscall.c  | 28 ++++++++++++----------------
 lib/vdso/gettimeofday.c |  2 ++
 3 files changed, 35 insertions(+), 16 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem()
  2026-08-31 12:55 [PATCH 0/3] vdso: Keep the CLOCK_AUX base at full precision Zhan Xusheng
@ 2026-08-31 12:55 ` Zhan Xusheng
  2026-08-31 13:19   ` Thomas Weißschuh
  2026-08-31 12:55 ` [PATCH 2/3] vdso/vsyscall: Keep the CLOCK_AUX base scaled Zhan Xusheng
  2026-08-31 12:55 ` [PATCH 3/3] vdso/gettimeofday: Assert that the clock id fits the dispatch mask Zhan Xusheng
  2 siblings, 1 reply; 8+ messages in thread
From: Zhan Xusheng @ 2026-08-31 12:55 UTC (permalink / raw)
  To: tglx, luto, vincenzo.frascino
  Cc: thomas.weissschuh, linux-kernel, zhanxusheng

The vDSO basetimes for CLOCK_MONOTONIC and CLOCK_BOOTTIME are kept in the
scaled nanoseconds of tkr_mono, so normalising them means dividing by
NSEC_PER_SEC << shift, which does not fit the u32 divisor of
__iter_div_u64_rem().

update_vdso_time_data() therefore open-codes the iterative division twice.
Turning the loops into a plain modulo is not an option either, as the vDSO
has no 64-bit division helpers on 32-bit.

Add __iter_div64_u64_rem(), the u64-divisor counterpart of
__iter_div_u64_rem(), keeping the asm() barrier that stops the compiler
turning the loop into a division, and use it for both.

No functional change.

Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 include/vdso/math64.h  | 21 +++++++++++++++++++++
 kernel/time/vsyscall.c | 17 ++++++-----------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/vdso/math64.h b/include/vdso/math64.h
index 22ae212f8b28..eb39d3411981 100644
--- a/include/vdso/math64.h
+++ b/include/vdso/math64.h
@@ -21,6 +21,27 @@ __iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder)
 	return ret;
 }
 
+static __always_inline u64
+__iter_div64_u64_rem(u64 dividend, u64 divisor, u64 *remainder)
+{
+	u64 ret = 0;
+
+	while (dividend >= divisor) {
+		/*
+		 * Prevent the compiler from optimising this loop into a
+		 * modulo operation.
+		 */
+		asm("" : "+rm"(dividend));
+
+		dividend -= divisor;
+		ret++;
+	}
+
+	*remainder = dividend;
+
+	return ret;
+}
+
 #if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
 
 #ifndef mul_u64_u32_add_u64_shr
diff --git a/kernel/time/vsyscall.c b/kernel/time/vsyscall.c
index aa59919b8f2c..165ad9d7f154 100644
--- a/kernel/time/vsyscall.c
+++ b/kernel/time/vsyscall.c
@@ -30,21 +30,21 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
 {
 	struct vdso_clock *vc = vdata->clock_data;
 	struct vdso_timestamp *vdso_ts;
-	u64 nsec, sec;
+	u64 nsec_per_sec, nsec, sec;
 
 	fill_clock_configuration(&vc[CS_HRES_COARSE],	&tk->tkr_mono);
 	fill_clock_configuration(&vc[CS_RAW],		&tk->tkr_raw);
 
+	/* One second in the scaled nanoseconds of tkr_mono */
+	nsec_per_sec = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;
+
 	/* CLOCK_MONOTONIC */
 	vdso_ts		= &vc[CS_HRES_COARSE].basetime[CLOCK_MONOTONIC];
 	vdso_ts->sec	= tk->xtime_sec + tk->wall_to_monotonic.tv_sec;
 
 	nsec = tk->tkr_mono.xtime_nsec;
 	nsec += ((u64)tk->wall_to_monotonic.tv_nsec << tk->tkr_mono.shift);
-	while (nsec >= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift)) {
-		nsec -= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift);
-		vdso_ts->sec++;
-	}
+	vdso_ts->sec	+= __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);
 	vdso_ts->nsec	= nsec;
 
 	/* Copy MONOTONIC time for BOOTTIME */
@@ -55,12 +55,7 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
 
 	/* CLOCK_BOOTTIME */
 	vdso_ts		= &vc[CS_HRES_COARSE].basetime[CLOCK_BOOTTIME];
-	vdso_ts->sec	= sec;
-
-	while (nsec >= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift)) {
-		nsec -= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift);
-		vdso_ts->sec++;
-	}
+	vdso_ts->sec	= sec + __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);
 	vdso_ts->nsec	= nsec;
 
 	/* CLOCK_MONOTONIC_RAW */
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] vdso/vsyscall: Keep the CLOCK_AUX base scaled
  2026-08-31 12:55 [PATCH 0/3] vdso: Keep the CLOCK_AUX base at full precision Zhan Xusheng
  2026-08-31 12:55 ` [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem() Zhan Xusheng
@ 2026-08-31 12:55 ` Zhan Xusheng
  2026-08-31 13:21   ` Thomas Weißschuh
  2026-08-31 12:55 ` [PATCH 3/3] vdso/gettimeofday: Assert that the clock id fits the dispatch mask Zhan Xusheng
  2 siblings, 1 reply; 8+ messages in thread
From: Zhan Xusheng @ 2026-08-31 12:55 UTC (permalink / raw)
  To: tglx, luto, vincenzo.frascino
  Cc: thomas.weissschuh, linux-kernel, zhanxusheng

The vDSO basetime of a clock is stored in the scaled nanoseconds of
tkr_mono, so that the reader can floor the base and the cycle delta
together in vdso_calc_ns().

vdso_time_update_aux() instead shifts the base down to nanoseconds, adds
the offset, and shifts it back up, which zeroes the fractional nanoseconds
of xtime_nsec.  The reader then floors the base and the delta separately:

  ktime_get_aux():  base + ((delta * mult + xtime_nsec) >> shift)
  vdso:             base + (xtime_nsec >> shift)
                         + ((delta * mult) >> shift)

Since floor(a) + floor(b) <= floor(a + b), the vDSO reports 0 or 1 ns
below the syscall for the same clock.  clock_getres() reports 1 ns for
auxiliary clocks, so that is the full advertised granularity.  It is not a
monotonicity problem: across an update the step is
floor(a + d) - floor(a) - floor(d), which is 0 or 1, never negative.

Add the offset in scaled nanoseconds as the other high resolution clocks
do, and normalise with __iter_div64_u64_rem() so that the stored base
stays below one second and the userspace fast-path does not iterate more
in __iter_div_u64_rem().

monotonic_to_aux.tv_nsec is a normalised timespec64 fraction, so it stays
below NSEC_PER_SEC even for a negative offset, and the sum stays below
2 * (NSEC_PER_SEC << shift).  The largest shift clocks_calc_mult_shift()
can pick is 32, which makes that 8.6e18 against a u64 limit of 1.8e19.

Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 kernel/time/vsyscall.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/time/vsyscall.c b/kernel/time/vsyscall.c
index 165ad9d7f154..ab3ef1e7d3c8 100644
--- a/kernel/time/vsyscall.c
+++ b/kernel/time/vsyscall.c
@@ -137,8 +137,8 @@ void vdso_time_update_aux(struct timekeeper *tk)
 	struct vdso_time_data *vdata = vdso_k_time_data;
 	struct vdso_timestamp *vdso_ts;
 	struct vdso_clock *vc;
+	u64 nsec_per_sec, nsec;
 	s32 clock_mode;
-	u64 nsec;
 
 	vc = &vdata->aux_clock_data[tk->id - TIMEKEEPER_AUX_FIRST];
 	vdso_ts = &vc->basetime[VDSO_BASE_AUX];
@@ -156,10 +156,11 @@ void vdso_time_update_aux(struct timekeeper *tk)
 
 		vdso_ts->sec = tk->xtime_sec + tk->monotonic_to_aux.tv_sec;
 
-		nsec = tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift;
-		nsec += tk->monotonic_to_aux.tv_nsec;
-		vdso_ts->sec += __iter_div_u64_rem(nsec, NSEC_PER_SEC, &nsec);
-		nsec = nsec << tk->tkr_mono.shift;
+		nsec_per_sec = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;
+
+		nsec = tk->tkr_mono.xtime_nsec;
+		nsec += (u64)tk->monotonic_to_aux.tv_nsec << tk->tkr_mono.shift;
+		vdso_ts->sec += __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);
 		vdso_ts->nsec = nsec;
 	}
 
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] vdso/gettimeofday: Assert that the clock id fits the dispatch mask
  2026-08-31 12:55 [PATCH 0/3] vdso: Keep the CLOCK_AUX base at full precision Zhan Xusheng
  2026-08-31 12:55 ` [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem() Zhan Xusheng
  2026-08-31 12:55 ` [PATCH 2/3] vdso/vsyscall: Keep the CLOCK_AUX base scaled Zhan Xusheng
@ 2026-08-31 12:55 ` Zhan Xusheng
  2026-08-31 13:22   ` Thomas Weißschuh
  2 siblings, 1 reply; 8+ messages in thread
From: Zhan Xusheng @ 2026-08-31 12:55 UTC (permalink / raw)
  To: tglx, luto, vincenzo.frascino
  Cc: thomas.weissschuh, linux-kernel, zhanxusheng

The clock id dispatch turns the id into a bit in a u32:

  if (!vdso_clockid_valid(clock))
      return false;
  msk = 1U << clock;

vdso_clockid_valid() admits everything up to CLOCK_AUX_LAST, which is 23,
so the shift is in range.  Nothing states the dependency though, and
raising MAX_AUX_CLOCKS past 16 would take CLOCK_AUX_LAST to 32 or beyond
and make the shift undefined.

Assert it at both dispatch sites.  The condition is on a parameter rather
than a constant, so it relies on the compiler deriving the range from the
vdso_clockid_valid() bail-out above it.  gcc 13 and clang 18 both do: x86
vdso64 and vdso32 build clean, and raising MAX_AUX_CLOCKS to 17 fails the
assert as intended.

Suggested-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 lib/vdso/gettimeofday.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
index f7a591aba59f..ef4dcc614489 100644
--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -285,6 +285,7 @@ __cvdso_clock_gettime_common(const struct vdso_time_data *vd, clockid_t clock,
 	 * Convert the clockid to a bitmask and use it to check which
 	 * clocks are handled in the VDSO directly.
 	 */
+	BUILD_BUG_ON(clock >= BITS_PER_TYPE(msk));
 	msk = 1U << clock;
 	if (likely(msk & VDSO_HRES))
 		vc = &vc[CS_HRES_COARSE];
@@ -438,6 +439,7 @@ bool __cvdso_clock_getres_common(const struct vdso_time_data *vd, clockid_t cloc
 	 * Convert the clockid to a bitmask and use it to check which
 	 * clocks are handled in the VDSO directly.
 	 */
+	BUILD_BUG_ON(clock >= BITS_PER_TYPE(msk));
 	msk = 1U << clock;
 	if (msk & (VDSO_HRES | VDSO_RAW)) {
 		/*
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem()
  2026-08-31 12:55 ` [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem() Zhan Xusheng
@ 2026-08-31 13:19   ` Thomas Weißschuh
  2026-08-31 15:02     ` David Laight
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Weißschuh @ 2026-08-31 13:19 UTC (permalink / raw)
  To: Zhan Xusheng; +Cc: tglx, luto, vincenzo.frascino, linux-kernel, zhanxusheng

On Mon, Aug 31, 2026 at 08:55:55PM +0800, Zhan Xusheng wrote:
> The vDSO basetimes for CLOCK_MONOTONIC and CLOCK_BOOTTIME are kept in the
> scaled nanoseconds of tkr_mono, so normalising them means dividing by
> NSEC_PER_SEC << shift, which does not fit the u32 divisor of
> __iter_div_u64_rem().
> 
> update_vdso_time_data() therefore open-codes the iterative division twice.
> Turning the loops into a plain modulo is not an option either, as the vDSO
> has no 64-bit division helpers on 32-bit.
> 
> Add __iter_div64_u64_rem(), the u64-divisor counterpart of
> __iter_div_u64_rem(), keeping the asm() barrier that stops the compiler
> turning the loop into a division, and use it for both.
> 
> No functional change.
> 
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> ---
>  include/vdso/math64.h  | 21 +++++++++++++++++++++
>  kernel/time/vsyscall.c | 17 ++++++-----------
>  2 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/include/vdso/math64.h b/include/vdso/math64.h
> index 22ae212f8b28..eb39d3411981 100644
> --- a/include/vdso/math64.h
> +++ b/include/vdso/math64.h
> @@ -21,6 +21,27 @@ __iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder)
>  	return ret;
>  }
>  
> +static __always_inline u64
> +__iter_div64_u64_rem(u64 dividend, u64 divisor, u64 *remainder)
> +{
> +	u64 ret = 0;
> +
> +	while (dividend >= divisor) {
> +		/*
> +		 * Prevent the compiler from optimising this loop into a
> +		 * modulo operation.
> +		 */
> +		asm("" : "+rm"(dividend));

This could probably be OPTIMIZER_HIDE_VAR(), but for consistency with 
__iter_div_u64_rem() it should probably stay like it is.

> +
> +		dividend -= divisor;
> +		ret++;
> +	}
> +
> +	*remainder = dividend;
> +
> +	return ret;
> +}
> +
>  #if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
>  
>  #ifndef mul_u64_u32_add_u64_shr
> diff --git a/kernel/time/vsyscall.c b/kernel/time/vsyscall.c
> index aa59919b8f2c..165ad9d7f154 100644
> --- a/kernel/time/vsyscall.c
> +++ b/kernel/time/vsyscall.c
> @@ -30,21 +30,21 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
>  {
>  	struct vdso_clock *vc = vdata->clock_data;
>  	struct vdso_timestamp *vdso_ts;
> -	u64 nsec, sec;
> +	u64 nsec_per_sec, nsec, sec;
>  
>  	fill_clock_configuration(&vc[CS_HRES_COARSE],	&tk->tkr_mono);
>  	fill_clock_configuration(&vc[CS_RAW],		&tk->tkr_raw);
>  
> +	/* One second in the scaled nanoseconds of tkr_mono */
> +	nsec_per_sec = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;

I am not a fan of the additional variables introduced in this patch.
Both the patch itself and the final code are harder to understand with
them in my opinion.
If you want to keep them, they should be introduced in a dedicated patch.

> +
>  	/* CLOCK_MONOTONIC */
>  	vdso_ts		= &vc[CS_HRES_COARSE].basetime[CLOCK_MONOTONIC];
>  	vdso_ts->sec	= tk->xtime_sec + tk->wall_to_monotonic.tv_sec;
>  
>  	nsec = tk->tkr_mono.xtime_nsec;
>  	nsec += ((u64)tk->wall_to_monotonic.tv_nsec << tk->tkr_mono.shift);
> -	while (nsec >= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift)) {
> -		nsec -= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift);
> -		vdso_ts->sec++;
> -	}
> +	vdso_ts->sec	+= __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);

This could directly store the result in vdso_ts->nsec if it produces better code.

>  	vdso_ts->nsec	= nsec;
>  
>  	/* Copy MONOTONIC time for BOOTTIME */
> @@ -55,12 +55,7 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
>  
>  	/* CLOCK_BOOTTIME */
>  	vdso_ts		= &vc[CS_HRES_COARSE].basetime[CLOCK_BOOTTIME];
> -	vdso_ts->sec	= sec;
 
I would leave this on its own line for the comment about the copy for BOOTTIME
to be easier to understand.

> -
> -	while (nsec >= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift)) {
> -		nsec -= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift);
> -		vdso_ts->sec++;
> -	}
> +	vdso_ts->sec	= sec + __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);
>  	vdso_ts->nsec	= nsec;
>  
>  	/* CLOCK_MONOTONIC_RAW */
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] vdso/vsyscall: Keep the CLOCK_AUX base scaled
  2026-08-31 12:55 ` [PATCH 2/3] vdso/vsyscall: Keep the CLOCK_AUX base scaled Zhan Xusheng
@ 2026-08-31 13:21   ` Thomas Weißschuh
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Weißschuh @ 2026-08-31 13:21 UTC (permalink / raw)
  To: Zhan Xusheng; +Cc: tglx, luto, vincenzo.frascino, linux-kernel, zhanxusheng

On Mon, Aug 31, 2026 at 08:55:56PM +0800, Zhan Xusheng wrote:
> The vDSO basetime of a clock is stored in the scaled nanoseconds of
> tkr_mono, so that the reader can floor the base and the cycle delta
> together in vdso_calc_ns().
> 
> vdso_time_update_aux() instead shifts the base down to nanoseconds, adds
> the offset, and shifts it back up, which zeroes the fractional nanoseconds
> of xtime_nsec.  The reader then floors the base and the delta separately:
> 
>   ktime_get_aux():  base + ((delta * mult + xtime_nsec) >> shift)
>   vdso:             base + (xtime_nsec >> shift)
>                          + ((delta * mult) >> shift)
> 
> Since floor(a) + floor(b) <= floor(a + b), the vDSO reports 0 or 1 ns
> below the syscall for the same clock.  clock_getres() reports 1 ns for
> auxiliary clocks, so that is the full advertised granularity.  It is not a
> monotonicity problem: across an update the step is
> floor(a + d) - floor(a) - floor(d), which is 0 or 1, never negative.
> 
> Add the offset in scaled nanoseconds as the other high resolution clocks
> do, and normalise with __iter_div64_u64_rem() so that the stored base
> stays below one second and the userspace fast-path does not iterate more
> in __iter_div_u64_rem().
> 
> monotonic_to_aux.tv_nsec is a normalised timespec64 fraction, so it stays
> below NSEC_PER_SEC even for a negative offset, and the sum stays below
> 2 * (NSEC_PER_SEC << shift).  The largest shift clocks_calc_mult_shift()
> can pick is 32, which makes that 8.6e18 against a u64 limit of 1.8e19.
> 
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> ---
>  kernel/time/vsyscall.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/time/vsyscall.c b/kernel/time/vsyscall.c
> index 165ad9d7f154..ab3ef1e7d3c8 100644
> --- a/kernel/time/vsyscall.c
> +++ b/kernel/time/vsyscall.c
> @@ -137,8 +137,8 @@ void vdso_time_update_aux(struct timekeeper *tk)
>  	struct vdso_time_data *vdata = vdso_k_time_data;
>  	struct vdso_timestamp *vdso_ts;
>  	struct vdso_clock *vc;
> +	u64 nsec_per_sec, nsec;
>  	s32 clock_mode;
> -	u64 nsec;
>  
>  	vc = &vdata->aux_clock_data[tk->id - TIMEKEEPER_AUX_FIRST];
>  	vdso_ts = &vc->basetime[VDSO_BASE_AUX];
> @@ -156,10 +156,11 @@ void vdso_time_update_aux(struct timekeeper *tk)
>  
>  		vdso_ts->sec = tk->xtime_sec + tk->monotonic_to_aux.tv_sec;
>  
> -		nsec = tk->tkr_mono.xtime_nsec >> tk->tkr_mono.shift;
> -		nsec += tk->monotonic_to_aux.tv_nsec;
> -		vdso_ts->sec += __iter_div_u64_rem(nsec, NSEC_PER_SEC, &nsec);
> -		nsec = nsec << tk->tkr_mono.shift;
> +		nsec_per_sec = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;

Same as for the previous patch, the additional variable hurts in my opinion.

> +
> +		nsec = tk->tkr_mono.xtime_nsec;
> +		nsec += (u64)tk->monotonic_to_aux.tv_nsec << tk->tkr_mono.shift;
> +		vdso_ts->sec += __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);
>  		vdso_ts->nsec = nsec;
>  	}
>  
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] vdso/gettimeofday: Assert that the clock id fits the dispatch mask
  2026-08-31 12:55 ` [PATCH 3/3] vdso/gettimeofday: Assert that the clock id fits the dispatch mask Zhan Xusheng
@ 2026-08-31 13:22   ` Thomas Weißschuh
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Weißschuh @ 2026-08-31 13:22 UTC (permalink / raw)
  To: Zhan Xusheng; +Cc: tglx, luto, vincenzo.frascino, linux-kernel, zhanxusheng

On Mon, Aug 31, 2026 at 08:55:57PM +0800, Zhan Xusheng wrote:
> The clock id dispatch turns the id into a bit in a u32:
> 
>   if (!vdso_clockid_valid(clock))
>       return false;
>   msk = 1U << clock;
> 
> vdso_clockid_valid() admits everything up to CLOCK_AUX_LAST, which is 23,
> so the shift is in range.  Nothing states the dependency though, and
> raising MAX_AUX_CLOCKS past 16 would take CLOCK_AUX_LAST to 32 or beyond
> and make the shift undefined.
> 
> Assert it at both dispatch sites.  The condition is on a parameter rather
> than a constant, so it relies on the compiler deriving the range from the
> vdso_clockid_valid() bail-out above it.  gcc 13 and clang 18 both do: x86
> vdso64 and vdso32 build clean, and raising MAX_AUX_CLOCKS to 17 fails the
> assert as intended.
> 
> Suggested-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Reviewed-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Thanks!

> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> ---
>  lib/vdso/gettimeofday.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
> index f7a591aba59f..ef4dcc614489 100644
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -285,6 +285,7 @@ __cvdso_clock_gettime_common(const struct vdso_time_data *vd, clockid_t clock,
>  	 * Convert the clockid to a bitmask and use it to check which
>  	 * clocks are handled in the VDSO directly.
>  	 */
> +	BUILD_BUG_ON(clock >= BITS_PER_TYPE(msk));
>  	msk = 1U << clock;
>  	if (likely(msk & VDSO_HRES))
>  		vc = &vc[CS_HRES_COARSE];
> @@ -438,6 +439,7 @@ bool __cvdso_clock_getres_common(const struct vdso_time_data *vd, clockid_t cloc
>  	 * Convert the clockid to a bitmask and use it to check which
>  	 * clocks are handled in the VDSO directly.
>  	 */
> +	BUILD_BUG_ON(clock >= BITS_PER_TYPE(msk));
>  	msk = 1U << clock;
>  	if (msk & (VDSO_HRES | VDSO_RAW)) {
>  		/*
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem()
  2026-08-31 13:19   ` Thomas Weißschuh
@ 2026-08-31 15:02     ` David Laight
  0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2026-08-31 15:02 UTC (permalink / raw)
  To: Thomas Weißschuh
  Cc: Zhan Xusheng, tglx, luto, vincenzo.frascino, linux-kernel, zhanxusheng

On Mon, 31 Aug 2026 15:19:23 +0200
Thomas Weißschuh <thomas.weissschuh@linutronix.de> wrote:

> On Mon, Aug 31, 2026 at 08:55:55PM +0800, Zhan Xusheng wrote:
> > The vDSO basetimes for CLOCK_MONOTONIC and CLOCK_BOOTTIME are kept in the
> > scaled nanoseconds of tkr_mono, so normalising them means dividing by
> > NSEC_PER_SEC << shift, which does not fit the u32 divisor of
> > __iter_div_u64_rem().
> > 
> > update_vdso_time_data() therefore open-codes the iterative division twice.
> > Turning the loops into a plain modulo is not an option either, as the vDSO
> > has no 64-bit division helpers on 32-bit.
> > 
> > Add __iter_div64_u64_rem(), the u64-divisor counterpart of
> > __iter_div_u64_rem(), keeping the asm() barrier that stops the compiler
> > turning the loop into a division, and use it for both.
> > 
> > No functional change.
> > 
> > Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> > ---
> >  include/vdso/math64.h  | 21 +++++++++++++++++++++
> >  kernel/time/vsyscall.c | 17 ++++++-----------
> >  2 files changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/vdso/math64.h b/include/vdso/math64.h
> > index 22ae212f8b28..eb39d3411981 100644
> > --- a/include/vdso/math64.h
> > +++ b/include/vdso/math64.h
> > @@ -21,6 +21,27 @@ __iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder)
> >  	return ret;
> >  }
> >  
> > +static __always_inline u64
> > +__iter_div64_u64_rem(u64 dividend, u64 divisor, u64 *remainder)
> > +{
> > +	u64 ret = 0;

Can that be u32?
You don't want to loop many times, and it might stop 32bit x86
spilling values inside the loop.

> > +
> > +	while (dividend >= divisor) {
> > +		/*
> > +		 * Prevent the compiler from optimising this loop into a
> > +		 * modulo operation.
> > +		 */
> > +		asm("" : "+rm"(dividend));  
> 
> This could probably be OPTIMIZER_HIDE_VAR(), but for consistency with 
> __iter_div_u64_rem() it should probably stay like it is.

Won't clang make a 'pig's breakfast' of "+rm" ?

David

> 
> > +
> > +		dividend -= divisor;
> > +		ret++;
> > +	}
> > +
> > +	*remainder = dividend;
> > +
> > +	return ret;
> > +}
> > +
> >  #if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
> >  
> >  #ifndef mul_u64_u32_add_u64_shr
> > diff --git a/kernel/time/vsyscall.c b/kernel/time/vsyscall.c
> > index aa59919b8f2c..165ad9d7f154 100644
> > --- a/kernel/time/vsyscall.c
> > +++ b/kernel/time/vsyscall.c
> > @@ -30,21 +30,21 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
> >  {
> >  	struct vdso_clock *vc = vdata->clock_data;
> >  	struct vdso_timestamp *vdso_ts;
> > -	u64 nsec, sec;
> > +	u64 nsec_per_sec, nsec, sec;
> >  
> >  	fill_clock_configuration(&vc[CS_HRES_COARSE],	&tk->tkr_mono);
> >  	fill_clock_configuration(&vc[CS_RAW],		&tk->tkr_raw);
> >  
> > +	/* One second in the scaled nanoseconds of tkr_mono */
> > +	nsec_per_sec = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;  
> 
> I am not a fan of the additional variables introduced in this patch.
> Both the patch itself and the final code are harder to understand with
> them in my opinion.
> If you want to keep them, they should be introduced in a dedicated patch.
> 
> > +
> >  	/* CLOCK_MONOTONIC */
> >  	vdso_ts		= &vc[CS_HRES_COARSE].basetime[CLOCK_MONOTONIC];
> >  	vdso_ts->sec	= tk->xtime_sec + tk->wall_to_monotonic.tv_sec;
> >  
> >  	nsec = tk->tkr_mono.xtime_nsec;
> >  	nsec += ((u64)tk->wall_to_monotonic.tv_nsec << tk->tkr_mono.shift);
> > -	while (nsec >= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift)) {
> > -		nsec -= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift);
> > -		vdso_ts->sec++;
> > -	}
> > +	vdso_ts->sec	+= __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);  
> 
> This could directly store the result in vdso_ts->nsec if it produces better code.
> 
> >  	vdso_ts->nsec	= nsec;
> >  
> >  	/* Copy MONOTONIC time for BOOTTIME */
> > @@ -55,12 +55,7 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
> >  
> >  	/* CLOCK_BOOTTIME */
> >  	vdso_ts		= &vc[CS_HRES_COARSE].basetime[CLOCK_BOOTTIME];
> > -	vdso_ts->sec	= sec;  
>  
> I would leave this on its own line for the comment about the copy for BOOTTIME
> to be easier to understand.
> 
> > -
> > -	while (nsec >= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift)) {
> > -		nsec -= (((u64)NSEC_PER_SEC) << tk->tkr_mono.shift);
> > -		vdso_ts->sec++;
> > -	}
> > +	vdso_ts->sec	= sec + __iter_div64_u64_rem(nsec, nsec_per_sec, &nsec);
> >  	vdso_ts->nsec	= nsec;
> >  
> >  	/* CLOCK_MONOTONIC_RAW */
> > -- 
> > 2.43.0
> >   
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-08-31 15:02 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-31 12:55 [PATCH 0/3] vdso: Keep the CLOCK_AUX base at full precision Zhan Xusheng
2026-08-31 12:55 ` [PATCH 1/3] vdso/math64: Add and use __iter_div64_u64_rem() Zhan Xusheng
2026-08-31 13:19   ` Thomas Weißschuh
2026-08-31 15:02     ` David Laight
2026-08-31 12:55 ` [PATCH 2/3] vdso/vsyscall: Keep the CLOCK_AUX base scaled Zhan Xusheng
2026-08-31 13:21   ` Thomas Weißschuh
2026-08-31 12:55 ` [PATCH 3/3] vdso/gettimeofday: Assert that the clock id fits the dispatch mask Zhan Xusheng
2026-08-31 13:22   ` Thomas Weißschuh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®