Re: [PATCH] Linux: Use 32-bit vDSO for clock_gettime, gettimeofday, time (bug 28071)

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

On 12/07/2021 10:15, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 12/07/2021 07:40, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> Using the test (a slight modified one from the bug report):
>>>>
>>>> --
>>>> #include <time.h>
>>>> #include <stdio.h>
>>>> #include <errno.h>
>>>>
>>>> int
>>>> main (void)
>>>> {
>>>>   struct timespec ts;
>>>>   errno = 0;
>>>>   clock_gettime (CLOCK_REALTIME, &ts);
>>>>   printf ("errno = %m (%d)\n", errno);
>>>>   errno = 0;
>>>>   clock_gettime (CLOCK_MONOTONIC, &ts);
>>>>   printf ("errno = %m (%d)\n", errno);
>>>> }
>>>> --
>>>>
>>>> I see no syscall on 5.11 kernel, only a clock_gettime (CLOCK_MONOTONIC)
>>>> on the 4.4 and a clock_gettime_time64 plus a clock_gettime on the 3.10.
>>>
>>> This still introduces a severe performance regression on older kernels.
>>> It may well make some exsting 32-bit applications unusable until the
>>> kernel is upgraded.  I'm not sure if this is a good idea.
>>>
>>> At least I can see that the clobbering of errno is gone.
>>
>> Yes and this how we initially decided to provide 64-bit time_t support,
>> where 32-bit implementations are done on top of 64-bit ones.  I am not
>> very fond of starting to adding 32-bit specific implementations, the
>> complexity to fix some specific cases do not really pay in the long
>> term imho.  
>>
>> We might also add back the global time64 internal variable that indicates
>> if the kernel supports 64-bit (removed by 9465c3a9fb557), but it also has
>> its ow issues (live migration like CRIU).
> 
> Hmm.  I see.  Let's fix the ENOSYS issue then because it breaks OpenJDK.
> 
> I'm a bit surprised that we still see the extra syscalls with your
> patch, but I suppose that's just the way the INTERNAL_VSYSCALL_CALL
> macro works.

The INTERNAL_VSYSCALL_CALL issues the syscall if the vDSO is not present
as a fallback mechanism.  It should not be really necessary on most 
implementation currently, but there are some architectures and kernel
version where the vDSO does not actually does it (I think mips on kernel
4.x version).

However, I think open-coding is should be simple; specially now for the
case we do not really want it to fallback to syscall.

> 
> Regarding the actual patch, there are a few missing spaces before
> parenthesis:
> 
> +  int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp)
> +    = GLRO(dl_vdso_clock_gettime64);
> +  int (*vdso_time)(clockid_t clock_id, struct timespec *tp)
> +    = GLRO(dl_vdso_clock_gettime);

My understanding is for GLRO the space is not really required because the
macro is not used a function call.  I followed the same idea for the
function pointer definition.

Regardless of the missing space, are you ok with my patch then? 

> 
> Thanks,
> Florian
>