[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20160127171132.GM238@brightrain.aerifal.cx>
Date: Wed, 27 Jan 2016 12:11:32 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v3 1/2] vdso: clock_gettime: call normal syscall
in case of an error
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@...ke-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
View attachment "clock_gettime.c" of type "text/plain" (1382 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
