From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9216 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v3 1/2] vdso: clock_gettime: call normal syscall in case of an error Date: Wed, 27 Jan 2016 12:11:32 -0500 Message-ID: <20160127171132.GM238@brightrain.aerifal.cx> References: <1453839994-7649-1-git-send-email-hauke@hauke-m.de> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="TakKZr9L6Hm6aLOc" X-Trace: ger.gmane.org 1453914715 2416 80.91.229.3 (27 Jan 2016 17:11:55 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 27 Jan 2016 17:11:55 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9229-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jan 27 18:11:54 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1aOTdH-00087g-4G for gllmg-musl@m.gmane.org; Wed, 27 Jan 2016 18:11:51 +0100 Original-Received: (qmail 9617 invoked by uid 550); 27 Jan 2016 17:11:47 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 9587 invoked from network); 27 Jan 2016 17:11:45 -0000 Content-Disposition: inline In-Reply-To: <1453839994-7649-1-git-send-email-hauke@hauke-m.de> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:9216 Archived-At: --TakKZr9L6Hm6aLOc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 --TakKZr9L6Hm6aLOc Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="clock_gettime.c" #include #include #include #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); --TakKZr9L6Hm6aLOc--