[PATCH v3 1/2] vdso: clock_gettime: call normal syscall in case of an error

[PATCH v3 1/2] vdso: clock_gettime: call normal syscall in case of an error

[Hauke Mehrtens]

When the vdso call returns an error just call the normal syscall and
return its result. clock_gettime() only fails with EFAULT if tp points
to an inaccessible address space or with EINVAL if the clk_id is
unknown. EFAULT is an implementation problem and performance for this
case is not important. When it returns EINVAL a normal syscall could
still work, so try it.

This fixes a bug when an error occurred errno was not set and it was a
value different than -1 returned.
In addition on MIPS the Linux kernel 4.4 only supports some clk_ids and
assumes the libc will call the original function if it gets a -ENOSYS,
so do this for all error codes.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 src/time/clock_gettime.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/time/clock_gettime.c b/src/time/clock_gettime.c
index 1572de0..9d3ff7e 100644
--- a/src/time/clock_gettime.c
+++ b/src/time/clock_gettime.c
@@ -26,16 +26,18 @@ void *__vdsosym(const char *, const char *);
 int __clock_gettime(clockid_t clk, struct timespec *ts)
 {
 #ifdef VDSO_CGT_SYM
+	int r;
 	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);
-#else
-	return sc_clock_gettime(clk, ts);
+	r = cgt(clk, ts);
+	/* In case an error occurred try the normal syscall */
+	if (!r) return r;
 #endif
+	return sc_clock_gettime(clk, ts);
 }
 
 weak_alias(__clock_gettime, clock_gettime);
-- 
2.7.0.rc3

[PATCH v3 2/2] 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>
---
 arch/mips/syscall_arch.h | 4 ++++
 1 file changed, 4 insertions(+)

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"
-- 
2.7.0.rc3

Re: [PATCH v3 1/2] vdso: clock_gettime: call normal syscall in case of an error

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

On Tue, Jan 26, 2016 at 09:26:33PM +0100, Hauke Mehrtens wrote:
> When the vdso call returns an error just call the normal syscall and
> return its result. clock_gettime() only fails with EFAULT if tp points
> to an inaccessible address space or with EINVAL if the clk_id is
> unknown. EFAULT is an implementation problem and performance for this
> case is not important. When it returns EINVAL a normal syscall could
> still work, so try it.
> 
> This fixes a bug when an error occurred errno was not set and it was a
> value different than -1 returned.
> In addition on MIPS the Linux kernel 4.4 only supports some clk_ids and
> assumes the libc will call the original function if it gets a -ENOSYS,
> so do this for all error codes.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  src/time/clock_gettime.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/time/clock_gettime.c b/src/time/clock_gettime.c
> index 1572de0..9d3ff7e 100644
> --- a/src/time/clock_gettime.c
> +++ b/src/time/clock_gettime.c
> @@ -26,16 +26,18 @@ void *__vdsosym(const char *, const char *);
>  int __clock_gettime(clockid_t clk, struct timespec *ts)
>  {
>  #ifdef VDSO_CGT_SYM
> +	int r;
>  	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);
> -#else
> -	return sc_clock_gettime(clk, ts);
> +	r = cgt(clk, ts);
> +	/* In case an error occurred try the normal syscall */
> +	if (!r) return r;
>  #endif
> +	return sc_clock_gettime(clk, ts);
>  }
>  
>  weak_alias(__clock_gettime, clock_gettime);
> -- 
> 2.7.0.rc3

This patch should work fine, but it does cause every failed
clock_gettime operation to make a double syscall on non-broken
kernels. If we're going to call sc_clock_gettime in the fallback path
now, and if we can't tail-call to the cgt pointer, I think it makes
more sense just to reorganize the code completely. I'm attaching a new
version I'm about to commit that makes the following changes:

- Inlines all of the sc_clock_gettime code into the end of
  __clock_gettime and eliminates the former.

- Accepts success or EINVAL returns from the vdso but falls back to
  the syscall for any other error.

- Uses a temp var to store the function pointer during the call so
  that volatile doesn't force it to get reloaded from memory multiple
  times.

- Adjust types so that there's no longer the aliasing violation of
  a_cas_p modifying a void* object overlapped with a function pointer.

- Uses a nice lazy init idiom to get rid of the if(cgt) branch.

Quick reading of the asm output shows the new version generating
rather efficient code paths, probably better than before.

After committing this I'll commit your patch with mips vdso support.

Rich

[-- Attachment #2: clock_gettime.c --]
[-- Type: text/plain, Size: 1381 bytes --]

#include <time.h>
#include <errno.h>
#include <stdint.h>
#include "syscall.h"
#include "libc.h"
#include "atomic.h"

#ifdef VDSO_CGT_SYM

void *__vdsosym(const char *, const char *);

static void *volatile vdso_func;

static int cgt_init(clockid_t clk, struct timespec *ts)
{
	void *p = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
	int (*f)(clockid_t, struct timespec *) =
		(int (*)(clockid_t, struct timespec *))p;
	a_cas_p(&vdso_func, (void *)cgt_init, p);
	return f ? f(clk, ts) : -ENOSYS;
}

static void *volatile vdso_func = (void *)cgt_init;

#endif

int __clock_gettime(clockid_t clk, struct timespec *ts)
{
	int r;

#ifdef VDSO_CGT_SYM
	int (*f)(clockid_t, struct timespec *) =
		(int (*)(clockid_t, struct timespec *))vdso_func;
	if (f) {
		r = f(clk, ts);
		if (!r) return r;
		if (r == -EINVAL) return __syscall_ret(r);
		/* Fall through on errors other than EINVAL. Some buggy
		 * vdso implementations return ENOSYS for clocks they
		 * can't handle, rather than making the syscall. This
		 * also handles the case where cgt_init fails to find
		 * a vdso function to use. */
	}
#endif

	r = __syscall(SYS_clock_gettime, clk, ts);
	if (r == -ENOSYS) {
		if (clk == CLOCK_REALTIME) {
			__syscall(SYS_gettimeofday, ts, 0);
			ts->tv_nsec = (int)ts->tv_nsec * 1000;
			return 0;
		}
		r = -EINVAL;
	}
	return __syscall_ret(r);
}

weak_alias(__clock_gettime, clock_gettime);