[PATCH v2] mips: add vdso support

[PATCH v2] mips: add vdso support

[Hauke Mehrtens]

vdso support is available on mips starting with kernel 4.4, see kernel
commit a7f4df4e21 "MIPS: VDSO: Add implementations of gettimeofday()
and clock_gettime()" for details.

In Linux kernel 4.4.0 the mips code returns -ENOSYS in case it can not
handle the vdso call and assumes the libc will call the original
syscall in this case. Handle this case in musl. Currently Linux kernel
4.4.0 handles the following types: CLOCK_REALTIME_COARSE,
CLOCK_MONOTONIC_COARSE, CLOCK_REALTIME and CLOCK_MONOTONIC.

These are some measurements of calling clock_gettime(CLOCK_MONOTONIC,
&tp); 1.000.000 times.

without vdso:
root@OpenWrt:/# time ./vdso-test
real 0m 0.95s
user 0m 0.24s
sys 0m 0.70s

with vdso:
root@OpenWrt:/# time /usr/bin/vdso-test
real 0m 0.35s
user 0m 0.34s
sys 0m 0.00s

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---

I just saw that the mips stuff is implemented in a bad way, it returns
-ENOSYS when it can not handle the vdso call. I already complained to
the mips kernel developers. ;-) This code was not tested, I am still
building.

 arch/mips/syscall_arch.h |  4 ++++
 src/time/clock_gettime.c | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/mips/syscall_arch.h b/arch/mips/syscall_arch.h
index e74e0ad..39c0ea3 100644
--- a/arch/mips/syscall_arch.h
+++ b/arch/mips/syscall_arch.h
@@ -161,3 +161,7 @@ static inline long __syscall6(long n, long a, long b, long c, long d, long e, lo
 	if (n == SYS_fstatat) __stat_fix(c);
 	return r2;
 }
+
+#define VDSO_USEFUL
+#define VDSO_CGT_SYM "__vdso_clock_gettime"
+#define VDSO_CGT_VER "LINUX_2.6"
diff --git a/src/time/clock_gettime.c b/src/time/clock_gettime.c
index 1572de0..dba99ff 100644
--- a/src/time/clock_gettime.c
+++ b/src/time/clock_gettime.c
@@ -26,13 +26,23 @@ void *__vdsosym(const char *, const char *);
 int __clock_gettime(clockid_t clk, struct timespec *ts)
 {
 #ifdef VDSO_CGT_SYM
+	int ret;
 	static int (*volatile cgt)(clockid_t, struct timespec *);
 	if (!cgt) {
 		void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
 		if (!f) f = (void *)sc_clock_gettime;
 		a_cas_p(&cgt, 0, f);
 	}
-	return cgt(clk, ts);
+	ret = cgt(clk, ts);
+
+	/*
+	 * mips in linux kernel 4.4.0 returns -ENOSYS if it can not
+	 * handle the syscall in vdso, the original syscall should be
+	 * called by the libc in such a case.
+	 */
+	if (ret == -ENOSYS)
+		return sc_clock_gettime(clk, ts);
+	return ret;
 #else
 	return sc_clock_gettime(clk, ts);
 #endif
-- 
2.7.0.rc3

Re: [PATCH v2] mips: add vdso support

[Rich Felker]

On Tue, Jan 26, 2016 at 12:00:12AM +0100, Hauke Mehrtens wrote:
> I just saw that the mips stuff is implemented in a bad way, it returns
> -ENOSYS when it can not handle the vdso call. I already complained to
> the mips kernel developers. ;-) This code was not tested, I am still
> building.

Thanks for catching that! It would have been a bad regression.

>  arch/mips/syscall_arch.h |  4 ++++
>  src/time/clock_gettime.c | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/syscall_arch.h b/arch/mips/syscall_arch.h
> index e74e0ad..39c0ea3 100644
> --- a/arch/mips/syscall_arch.h
> +++ b/arch/mips/syscall_arch.h
> @@ -161,3 +161,7 @@ static inline long __syscall6(long n, long a, long b, long c, long d, long e, lo
>  	if (n == SYS_fstatat) __stat_fix(c);
>  	return r2;
>  }
> +
> +#define VDSO_USEFUL
> +#define VDSO_CGT_SYM "__vdso_clock_gettime"
> +#define VDSO_CGT_VER "LINUX_2.6"
> diff --git a/src/time/clock_gettime.c b/src/time/clock_gettime.c
> index 1572de0..dba99ff 100644
> --- a/src/time/clock_gettime.c
> +++ b/src/time/clock_gettime.c
> @@ -26,13 +26,23 @@ void *__vdsosym(const char *, const char *);
>  int __clock_gettime(clockid_t clk, struct timespec *ts)
>  {
>  #ifdef VDSO_CGT_SYM
> +	int ret;
>  	static int (*volatile cgt)(clockid_t, struct timespec *);
>  	if (!cgt) {
>  		void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
>  		if (!f) f = (void *)sc_clock_gettime;
>  		a_cas_p(&cgt, 0, f);
>  	}
> -	return cgt(clk, ts);
> +	ret = cgt(clk, ts);
> +
> +	/*
> +	 * mips in linux kernel 4.4.0 returns -ENOSYS if it can not
> +	 * handle the syscall in vdso, the original syscall should be
> +	 * called by the libc in such a case.
> +	 */
> +	if (ret == -ENOSYS)
> +		return sc_clock_gettime(clk, ts);
> +	return ret;
>  #else
>  	return sc_clock_gettime(clk, ts);
>  #endif

This could probably be written better as:

	if (ret != -ENOSYS) return ret;
#endif
	return sc_clock_gettime(clk, ts);

On the other hand if the kernel is going to fix the mips bug and use a
new symbol version for the fixed one, we could just wait and define
VDSO_CGT_VER appropriately. That's the less invasive fix (doesn't
affect other archs) but I'm not really partial.

There's some value to having the fallback anyway in case some buggy
kernel breaks the vdso function on an arch where it was previously
working. I almost wonder if we shouldn't just do:

	if (ret == -EINVAL) return ret;
#endif
	return sc_clock_gettime(clk, ts);

i.e. reject any error but EINVAL from the vdso and try the syscall,
since EINVAL is the only one that should be possible.

Rich

Re: [PATCH v2] mips: add vdso support

[Markus Wichmann]

On Mon, Jan 25, 2016 at 06:21:18PM -0500, Rich Felker wrote:
> This could probably be written better as:
> 
> 	if (ret != -ENOSYS) return ret;
> #endif
> 	return sc_clock_gettime(clk, ts);
> 

And one additional idea: If the kernel did return ENOSYS, set cgt to
sc_clock_gettime. Because if the kernel returned ENOSYS, we can assume
this is a permanent failure and not a temporary one, so there's no point
in keeping to try the VDSO version.

> i.e. reject any error but EINVAL from the vdso and try the syscall,
> since EINVAL is the only one that should be possible.
> 

clock_gettime() takes a pointer argument, so EFAULT is always possible.

Ciao,
Markus

Re: [PATCH v2] mips: add vdso support

[Hauke Mehrtens]

On 01/26/2016 04:32 PM, Markus Wichmann wrote:
> On Mon, Jan 25, 2016 at 06:21:18PM -0500, Rich Felker wrote:
>> This could probably be written better as:
>>
>> 	if (ret != -ENOSYS) return ret;
>> #endif
>> 	return sc_clock_gettime(clk, ts);
>>
> 
> And one additional idea: If the kernel did return ENOSYS, set cgt to
> sc_clock_gettime. Because if the kernel returned ENOSYS, we can assume
> this is a permanent failure and not a temporary one, so there's no point
> in keeping to try the VDSO version.

It is not permanently, but it depends on the input parameters, see this
code:
http://lxr.free-electrons.com/source/arch/mips/vdso/gettimeofday.c?v=4.4#L207

It only returns -ENOSYS if it can not handle requested clkid.

> 
>> i.e. reject any error but EINVAL from the vdso and try the syscall,
>> since EINVAL is the only one that should be possible.
>>
> 
> clock_gettime() takes a pointer argument, so EFAULT is always possible.
> 
> Ciao,
> Markus
> 

Re: [PATCH v2] mips: add vdso support

[Rich Felker]

On Tue, Jan 26, 2016 at 04:32:16PM +0100, Markus Wichmann wrote:
> > i.e. reject any error but EINVAL from the vdso and try the syscall,
> > since EINVAL is the only one that should be possible.
> > 
> 
> clock_gettime() takes a pointer argument, so EFAULT is always possible.

EFAULT is not relevant. It can only happen when the calling program
has undefined behavior, and in fact if the vdso version got used the
program would already have crashed with SIGSEGV anyway.

Rich