Hi Arnd,
thank you for your review.
On 31/05/2019 09:46, Arnd Bergmann wrote:
> On Thu, May 30, 2019 at 4:15 PM Vincenzo Frascino
> <vincenzo.frascino@xxxxxxx> wrote:
>>
>> vDSO (virtual dynamic shared object) is a mechanism that the Linux
>> kernel provides as an alternative to system calls to reduce where
>> possible the costs in terms of cycles.
>> This is possible because certain syscalls like gettimeofday() do
>> not write any data and return one or more values that are stored
>> in the kernel, which makes relatively safe calling them directly
>> as a library function.
>
> Hi Vincento,
>
> I've very happy with how this turned out overall, and as far as I can
> tell you have addressed all my previous comments. I had another
> look through the series and only noticed a few very minor issues.
>
Thanks! I agree with what you pointed out in the single patches, I will wait for
Thomas to review them as well and then will address all the comments in v7.
...
>
> One open question I touched in my review is whether we want to
> have a vdso version of clock_getres() in all architectures or not.
> I'd prefer to leave it out because there is very little advantage to
> it over the system call (the results don't change at runtime and
> can easily be cached by libc if performance ever matters), and
> it takes up a small amount of memory for the implementation.
>
I thought about it and I ended up with what proposed in this patchset mainly for
symmetry across all the architectures since in the end they use the same common
code.
It seems also that there is some performance impact (i.e.):
clock-getres-monotonic: libc(system call): 296 nsec/call
clock-getres-monotonic: libc(vdso): 5 nsec/call
I agree with you though when you say that caching it in the libc is a
possibility to overcome the performance impact.
> We shouldn't just need it for consistency because all callers
> would require implementing a fallback to the system call
> anyway, to deal with old kernels.
>
A way to address this issue would be to use versioning, which seems supported in
the vdso library (i.e. arch/x86/entry/vdso/vdso32/vdso32.lds.S).
For example for x86 (vdso32) we would have something like:
VERSION
{
LINUX_5.3 (being optimistic here :) ) {
global:
__vdso_clock_getres;
__vdso_clock_gettime64;
};
LINUX_2.6 {
global:
__vdso_clock_gettime;
__vdso_gettimeofday;
__vdso_time;
};
LINUX_2.5 {
global:
__kernel_vsyscall;
__kernel_sigreturn;
__kernel_rt_sigreturn;
local: *;
};
}
What do you think? Would this be a viable solution?
> If anyone comes up with a good reason why it should be added
> after all, let me know and I'll stop mentioning it.
>
> Arnd
>
--
Regards,
Vincenzo
