Re: [RFC PATCH] powerpc/32: Switch VDSO to C implementation.

[Christophe Leroy]

Hi Thomas,

On 01/09/2020 02:05 PM, Thomas Gleixner wrote:
> Christophe!
>
> Christophe Leroy <christophe.leroy@xxxxxx> writes:
> > In do_hres(), I see:
> >
> > 		cycles = __arch_get_hw_counter(vd->clock_mode);
> > 		ns = vdso_ts->nsec;
> > 		last = vd->cycle_last;
> > 		if (unlikely((s64)cycles < 0))
> > 			return -1;
> >
> > __arch_get_hw_counter() returns a u64 values. On the PPC, this is read
> > from the timebase which is a 64 bits counter.
> >
> > Why returning -1 if (s64)cycles < 0 ? Does it means we have to mask out
> > the most significant bit when reading the HW counter ?
>
> Only if you expect the HW counter to reach a value which has bit 63
> set. That'd require:
>
> uptime		counter frequency
>
> ~292 years      1GHz
> ~ 58 years      5GHz
>
> assumed that the HW counter starts at 0 when the box is powered on.
>
> The reason why this is implemented in this way is that
> __arch_get_hw_counter() needs a way to express that the clocksource of
> the moment is not suitable for VDSO so that the syscall fallback gets
> invoked.
>
> Sure we could have used a pointer for the value and a return value
> indicating the validity, but given the required uptime the resulting
> code overhead seemed to be not worth it. At least not for me as I'm not
> planning to be around 58 years from now :)

I managed to get better code and better performance by splitting out the 
validity check as follows. Would it be suitable for all arches ?


diff --git a/arch/powerpc/include/asm/vdso/gettimeofday.h 
b/arch/powerpc/include/asm/vdso/gettimeofday.h
index 689f51b0d8c9..11cdd6faa4ad 100644
--- a/arch/powerpc/include/asm/vdso/gettimeofday.h
+++ b/arch/powerpc/include/asm/vdso/gettimeofday.h
@@ -114,15 +114,17 @@ int clock_getres32_fallback(clockid_t _clkid, 
struct old_timespec32 *_ts)
 	return ret;
 }

-static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
+static __always_inline bool __arch_is_hw_counter_valid(s32 clock_mode)
 {
 	/*
 	 * clock_mode == 0 implies that vDSO are enabled otherwise
 	 * fallback on syscall.
 	 */
-	if (clock_mode)
-		return ULLONG_MAX;
+	return clock_mode ? false : true;
+}

+static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
+{
 	return get_tb();
 }

diff --git a/lib/vdso/gettimeofday.c b/lib/vdso/gettimeofday.c
index ee9da52a3e02..90bb5dfd0db0 100644
--- a/lib/vdso/gettimeofday.c
+++ b/lib/vdso/gettimeofday.c
@@ -46,11 +46,12 @@ static inline int do_hres(const struct vdso_data 
*vd, clockid_t clk,

 	do {
 		seq = vdso_read_begin(vd);
+		if (!__arch_is_hw_counter_valid(vd->clock_mode))
+			return -1;
+
 		cycles = __arch_get_hw_counter(vd->clock_mode);
 		ns = vdso_ts->nsec;
 		last = vd->cycle_last;
-		if (unlikely((s64)cycles < 0))
-			return -1;

 		ns += vdso_calc_delta(cycles, last, vd->mask, vd->mult);
 		ns >>= vd->shift;


Thanks
Christophe