From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9197 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v2] mips: add vdso support Date: Mon, 25 Jan 2016 18:21:18 -0500 Message-ID: <20160125232118.GD238@brightrain.aerifal.cx> References: <1453762812-3187-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: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1453764100 9126 80.91.229.3 (25 Jan 2016 23:21:40 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 25 Jan 2016 23:21:40 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9210-gllmg-musl=m.gmane.org@lists.openwall.com Tue Jan 26 00:21:39 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 1aNqS3-0000Kp-Ii for gllmg-musl@m.gmane.org; Tue, 26 Jan 2016 00:21:39 +0100 Original-Received: (qmail 11471 invoked by uid 550); 25 Jan 2016 23:21:32 -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 11451 invoked from network); 25 Jan 2016 23:21:31 -0000 Content-Disposition: inline In-Reply-To: <1453762812-3187-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:9197 Archived-At: 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