Re: [PATCH v3 1/3] vdso/math64: Add and use __iter_div64_u64_rem()

[Thomas Weißschuh]

Hi Zhan,

thanks for the new version. However please slow down a bit with sending new
revisions to give people time to respond.

On Tue, Sep 01, 2026 at 10:06:34AM +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.

This is not vDSO userspace code, but on the kernel side.
The problem is that divisions are unnecessarily slow.

Also a module alone would not be enough, as we need the division result.

> Add __iter_div64_u64_rem(), the u64-divisor counterpart of
> __iter_div_u64_rem(), and use it for both.  The remainder goes straight
> into the basetime as the coarse clocks already do, which also makes the
> copy of the CLOCK_MONOTONIC values for CLOCK_BOOTTIME take both of them
> from the same place.
> 
> The quotient is a u32 like the u32-divisor version returns.  A loop that
> subtracts only makes sense when it iterates a handful of times, and it
> keeps 32-bit from carrying the counter in a register pair: vsyscall.o
> loses 16 bytes of text on x86-64 and the loop drops from 36 to 28
> instructions on 32-bit gcc.
> 
> The barrier keeps the value in a register rather than offering a memory
> alternative like __iter_div_u64_rem() does.  Both forms stop the loop
> becoming a division, but clang takes the memory alternative and spills
> inside the loop, on 64-bit as well.  Converting __iter_div_u64_rem() the
> same way is worth 64 bytes of vdso64 text and 112 of vdso32 built with
> clang 18, since it sits on the clock_gettime() path through
> vdso_set_timespec(); that is left for a separate patch as it also has
> users outside the vDSO.
> 
> No functional change.
> 
> Suggested-by: David Laight <david.laight.linux@xxxxxxxxx>
> Signed-off-by: Zhan Xusheng <zhanxusheng@xxxxxxxxxx>
> ---
>  include/vdso/math64.h  | 22 ++++++++++++++++++++++
>  kernel/time/vsyscall.c | 18 +++++++-----------
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/include/vdso/math64.h b/include/vdso/math64.h
> index 22ae212f8b28..02abdf6e82ed 100644
> --- a/include/vdso/math64.h
> +++ b/include/vdso/math64.h
> @@ -21,6 +21,28 @@ __iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder)
>  	return ret;
>  }
>  
> +static __always_inline u32
> +__iter_div64_u64_rem(u64 dividend, u64 divisor, u64 *remainder)
> +{
> +	u32 ret = 0;
> +
> +	while (dividend >= divisor) {
> +		/*
> +		 * Prevent the compiler from optimising this loop into a
> +		 * modulo operation.  Keep the value in a register, as clang
> +		 * spills it when offered a memory alternative.
> +		 */
> +		asm("" : "=r"(dividend) : "0"(dividend));

This is now the same as OPTIMIZER_HIDE_VAR(). Let's use the standard macro.
In my opinion we can then also drop the comment about the memory alternative.

If this variant is proven to be the better one, we should also use it in
__iter_div_u64_rem(), in a new first patch.
Any unnecessary divergence will confuse future readers.

> +
> +		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..993258d5f1e7 100644
> --- a/kernel/time/vsyscall.c
> +++ b/kernel/time/vsyscall.c
> @@ -41,14 +41,13 @@ static inline void update_vdso_time_data(struct vdso_time_data *vdata, struct ti
>  
>  	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->nsec	= nsec;
> +	vdso_ts->sec	+= __iter_div64_u64_rem(nsec,
> +				(u64)NSEC_PER_SEC << tk->tkr_mono.shift,
> +				&vdso_ts->nsec);

This alignment looks off. You have 100 characters width. Better:

	vdso_ts->sec	+= __iter_div64_u64_rem(nsec,
					        (u64)NSEC_PER_SEC << tk->tkr_mono.shift,
						&vdso_ts->nsec);

>  
>  	/* Copy MONOTONIC time for BOOTTIME */
>  	sec	= vdso_ts->sec;
> +	nsec	= vdso_ts->nsec;
>  	/* Add the boot offset */
>  	sec	+= tk->monotonic_to_boot.tv_sec;
>  	nsec	+= (u64)tk->monotonic_to_boot.tv_nsec << tk->tkr_mono.shift;

(...)