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

Christophe Leroy
2020-01-09T15:21:17Z

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