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

[Rich Felker <dalias@libc.org>]

[-- 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);