From owner-freebsd-arch@FreeBSD.ORG Sun Jan 4 18:20:31 2015 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 67CD4705; Sun, 4 Jan 2015 18:20:31 +0000 (UTC) Received: from mx1.stack.nl (relay04.stack.nl [IPv6:2001:610:1108:5010::107]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mailhost.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id EC4D5355; Sun, 4 Jan 2015 18:20:30 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 37D12B805D; Sun, 4 Jan 2015 19:20:27 +0100 (CET) Received: by snail.stack.nl (Postfix, from userid 1677) id 2272528494; Sun, 4 Jan 2015 19:20:27 +0100 (CET) Date: Sun, 4 Jan 2015 19:20:27 +0100 From: Jilles Tjoelker To: Konstantin Belousov Subject: Re: Fixing dlopen("libpthread.so") Message-ID: <20150104182026.GA61250@stack.nl> References: <20141226165337.GJ1754@kib.kiev.ua> <20150103212837.GC46373@stack.nl> <20150104114600.GC42409@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150104114600.GC42409@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: threads@freebsd.org, arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 04 Jan 2015 18:20:31 -0000 On Sun, Jan 04, 2015 at 01:46:00PM +0200, Konstantin Belousov wrote: > On Sat, Jan 03, 2015 at 10:28:37PM +0100, Jilles Tjoelker wrote: > > I think this can work, conceptually. > The changes were already committed, right before I got your mail. > Still, thank you for the useful review. > > Does this also mean that the order of libc and libthr on the link line > > no longer matters? (so SVN r265003 could be reverted?) > I think yes, it could. I have similar queries already. > IMO it makes sense to wait some time for the changes to settle before > even asking for testing of such reversals. OK. > > It looks like this change may introduce more indirection in common paths > > which might harm performance. The effect is slight and on the other hand > > more single-threaded applications can benefit from non-libthr > > performance. > > I noticed a problem in lib/libc/gen/raise.c: it calls the INTERPOS_pause > > entry instead of the INTERPOS_raise entry. Alternatively, libc could > > call thr_self() and thr_kill() so libthr need not do anything (much like > > how lib/libc/gen/sem_new.c works). > This was noted already and corrected raise.c committed. > I have mixed feeling about the proposal to change raise(3) to use > threading syscalls. On the one hand, it indeed works, on the other, > there are subtle differences, not all important, WRT the change. E.g., > puzzling ktrace output, changed si_code from SI_USER to SI_LWP (is this > a bug on its own ?) . SI_LWP already occurs with libthr's raise(), and with pthread_kill(pthread_self(), sig); which POSIX says is equivalent. > > The number of interposers could be reduced slightly by doing simple work > > like wait(), waitpid() and wait3() before instead of after > > interposition. This would be an additional change and the current patch > > is more like the existing code. > Could you, please, explain this ? What exactly should be done before what, > and how would it help ? In libc, define the below wait3(). All code in libc that allows libthr to interpose wait3() and in libthr that interposes wait3() can then go away. pid_t wait3(int *istat, int options, struct rusage *rup) { return (((pid_t (*)(pid_t, int *, int, struct rusage *)) __libc_interposing[INTERPOS_wait4])(WAIT_ANY, istat, options, rup)); } This change is also possible for wait() and waitpid(), but not for e.g. sigwait() which has different [EINTR] behaviour than sigtimedwait(). > > In lib/libc/sys/__error.c, __error_selector should be declared static or > > __hidden and an accessor function added for libthr, to avoid an > > additional pointer chase in a common path. One more PLT and GOT > > reference in some situations may be avoided by making a __hidden alias > > for __error_unthreaded and making that __error_selector's initial value. > __error_unthreaded can be made static, it seems. > I added __set_error_selector() and made __error_selector static. > > Although __libc_interposing is properly not exported (with > > __libc_interposing_slot accessor), the full benefit is not obtained > > because __libc_interposing is not declared __hidden in > > lib/libc/include/libc_private.h, so the compiler will generate an > > indirect access anyway (and ld will fix it up using a relative > > relocation). > Done. > > In lib/libthr/thread/thr_private.h, __error_threaded() can be declared > > __hidden. > Done. The changes look good to me. > > Sharing __sys_* code between libc and libthr is a net loss (the two > > copies of the name, GOT entry, PLT entry and longer instruction > > sequences are almost certainly larger than a syscall stub (32 bytes on > > amd64)), so I think the best fix for most indirection from > > __libc_interposing entries is to give libthr its own copy and mark both > > copies hidden, instead of adding hidden aliases now. > Do you mean linking syscall stubs (__sys_* trampolines) statically into > the libthr, and referencing them directly from the C code ? Yes, this > will be huge benefitial, both for libthr and ld-elf.so.1, for rtld it is > even kind of required, see r276646 and other uses of __sys. I thought > about it. But we need something like libsyscalls.a and libsyscalls_pic.a > extracted from libc first. > If I am misinterpreting your suggestion, please explain it in more > details. Your interpretation is correct. -- Jilles Tjoelker