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

[David Laight <david.laight.linux@gmail.com>]

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
> >   
>