Re: [PATCH v2] hppa/vdso: Add wrappers for vDSO functions

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 27/06/24 06:46, Helge Deller wrote:
> * Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>:
>>
>> On 25/06/24 16:48, Helge Deller wrote:
>>> On 6/24/24 22:31, Adhemerval Zanella Netto wrote:
>>>>
>>>>
>>>> On 21/06/24 16:26, Helge Deller wrote:
>>>>> The upcoming parisc (hppa) v6.11 Linux kernel will include vDSO
>>>>> support for gettimeofday(), clock_gettime() and clock_gettime64()
>>>>> syscalls for 32- and 64-bit userspace.
>>>>> The patch below adds the necessary glue code for glibc.
>>>>>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>
>>>>> Changes in v2:
>>>>> - add vsyscalls for 64-bit too
>>>>>
>>>>> diff -up ./sysdeps/unix/sysv/linux/hppa/sysdep.h.org ./sysdeps/unix/sysv/linux/hppa/sysdep.h
>>>>> --- ./sysdeps/unix/sysv/linux/hppa/sysdep.h.org    2024-06-15 20:20:58.992000000 +0000
>>>>> +++ ./sysdeps/unix/sysv/linux/hppa/sysdep.h    2024-06-21 19:19:02.132000000 +0000
>>>>> @@ -468,6 +468,18 @@ L(pre_end):                    ASM_LINE_SEP    \
>>>>>   #define CLOB_ARGS_1 CLOB_ARGS_2, "%r25"
>>>>>   #define CLOB_ARGS_0 CLOB_ARGS_1, "%r26"
>>>>>
>>>>> +#define VDSO_NAME    "LINUX_6.11"
>>>>> +#define VDSO_HASH    182951793
>>>>> +
>>>>> +#ifdef __LP64__
>>>>> +# define HAVE_CLOCK_GETTIME_VSYSCALL    "__vdso_clock_gettime"
>>>>> +# define HAVE_GETTIMEOFDAY_VSYSCALL     "__vdso_gettimeofday"
>>>>> +#else
>>>>> +# define HAVE_CLOCK_GETTIME_VSYSCALL    "__vdso_clock_gettime"
>>>>> +# define HAVE_CLOCK_GETTIME64_VSYSCALL  "__vdso_clock_gettime64"
>>>>> +# define HAVE_GETTIMEOFDAY_VSYSCALL     "__vdso_gettimeofday"
>>>>> +#endif /* __LP64__ */
>>>>> +
>>>>>   #endif    /* __ASSEMBLER__ */
>>>>
>>>> Not sure why you have added the gettimeofday support, currently 32 bits
>>>> it is already routed to to clock_gettime (which will use
>>>> __vdso_clock_gettime64 anyway).
>>>
>>> Yep, you are right.
>>> I actually didn't checked if it's being used but just saw it's defined
>>> for other arches too, so I assumed it to be used.
>>>
>>>> For hppa to actually use, it would require to add a way to call it
>>>> for !USE_IFUNC_GETTIMEOFDAY gettimeofday, which I am not it really be
>>>> an improvement here.
>>>
>>> Yes, that doesn't make sense.
>>>
>>> Looking at the code it seems riscv, sparc, arm, mips and s390 define it
>>> too, without being used. Do you suggest we should remove gettimeofday
>>> vsyscall from hppa again (or just keep like the others even if not being used)?
>>
>> The riscv only added it for CONFIG_64BIT, while old ABIs like arm, sparc,
>> mips, and s390 added before 64 bit time_t landed on 5.1.  With current 
>> 32 bits ABIs moving to 64 bit time_t userland, there is no much sense in 
>> providing a 32 bit gettimeofday, nor I think I adding a  gettimeoday64 would 
>> make much sense (maybe on really high sensitive workloads that require 
>> low-latency timestamping, but clock_gettime would work better anyway).
>>
>> It is highly unlikely that the symbol will ever be used by userland,
>> and I think it only make sense to provide clock_gettime64.  The glibc 
>> 32 bit time_t clock_gettime routes to clock_gettime64 and it will only 
>> fallback to 32 bit vDSO symbol if the 64 bit time_t one is not present.
> 
> Like patch below?
> Helge
> 
> 
> [PATCH] hppa/vdso: Provide 64-bit clock_gettime() vDSO only
> 
> Adhemerval noticed that the gettimeofday() and 32-bit clock_gettime()
> vDSO calls won't be used by glibc on hppa, so there is no need to
> declare them.  Both syscalls will be emulated by utilizing return values
> of the 64-bit clock_gettime() vDSO instead..
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Suggested-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> 
> diff --git a/sysdeps/unix/sysv/linux/hppa/sysdep.h b/sysdeps/unix/sysv/linux/hppa/sysdep.h
> index af62f7501e..e47975e5cf 100644
> --- a/sysdeps/unix/sysv/linux/hppa/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/hppa/sysdep.h
> @@ -473,11 +473,8 @@ L(pre_end):					ASM_LINE_SEP	\
>  
>  #ifdef __LP64__
>  # define HAVE_CLOCK_GETTIME_VSYSCALL    "__vdso_clock_gettime"
> -# define HAVE_GETTIMEOFDAY_VSYSCALL     "__vdso_gettimeofday"
>  #else
> -# define HAVE_CLOCK_GETTIME_VSYSCALL    "__vdso_clock_gettime"
>  # define HAVE_CLOCK_GETTIME64_VSYSCALL  "__vdso_clock_gettime64"
> -# define HAVE_GETTIMEOFDAY_VSYSCALL     "__vdso_gettimeofday"
>  #endif /* __LP64__ */
>  
>  #endif	/* __ASSEMBLER__ */

Yeah, it seems ok thanks.