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

[Christophe Leroy <christophe.leroy@c-s.fr>]

Hi Thomas,

On 01/09/2020 02:05 PM, Thomas Gleixner wrote:
> Christophe!
> 
> Christophe Leroy <christophe.leroy@c-s.fr> 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