by Christophe Leroy

[permalink] [raw]

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

Hi Thomas,

On 01/09/2020 02:05 PM, Thomas Gleixner wrote:
> Christophe!
>
> Christophe Leroy <[email protected]> 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

2020-01-10 22:43:24

by Thomas Gleixner

[permalink] [raw]

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

Christophe,

Christophe Leroy <[email protected]> writes:
> On 01/09/2020 02:05 PM, Thomas Gleixner wrote:
>> 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 ?

A quick test on x86 shows only a minimal impact, but it's in the
noise. So from my side that's fine, but I can't talk for ARM[64]/MIPS

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

I don't think we need an arch specific function here. I rather convert
the boolean of ARM[64] to the x86/MIPS way of VCLOCK_* modes and let the
generic code check for clock_mode == VCLOCK_NONE.

There is some magic in ARM and MIPS which wants to be able to disable
the whole thing at compile time, but that can be handled in the generic
code with just an extra config switch.

I'll have a stab at that tomorrow.

Thanks,

tglx