From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Florian Weimer <fweimer@redhat.com>
Cc: carlos <carlos@redhat.com>,
Joseph Myers <joseph@codesourcery.com>,
Szabolcs Nagy <szabolcs.nagy@arm.com>,
libc-alpha <libc-alpha@sourceware.org>,
Thomas Gleixner <tglx@linutronix.de>, Ben Maurer <bmaurer@fb.com>,
Peter Zijlstra <peterz@infradead.org>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Boqun Feng <boqun.feng@gmail.com>,
Will Deacon <will.deacon@arm.com>,
Dave Watson <davejwatson@fb.com>, Paul Turner <pjt@google.com>,
Rich Felker <dalias@libc.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
linux-api <linux-api@vger.kernel.org>
Subject: Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
Date: Thu, 10 Jan 2019 15:31:55 -0500 (EST) [thread overview]
Message-ID: <1681283664.1380.1547152315426.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <87h8fkz6qx.fsf@oldenburg2.str.redhat.com>
----- On Dec 11, 2018, at 2:40 AM, Florian Weimer fweimer@redhat.com wrote:
> * Mathieu Desnoyers:
>
>> I want to keep the __rseq_refcount symbol so out-of-libc users can
>> register rseq if they are linked against a pre-2.29 libc.
>
> Sorry, I was confused.
Hi Florian,
Thanks for your questions below. Sorry for my delayed answer, I've
been preempted by vacation time. See more below,
>
>> diff --git a/csu/Makefile b/csu/Makefile
>> index 88fc77662e..81d471587f 100644
>> --- a/csu/Makefile
>> +++ b/csu/Makefile
>> @@ -28,7 +28,7 @@ include ../Makeconfig
>>
>> routines = init-first libc-start $(libc-init) sysdep version check_fds \
>> libc-tls elf-init dso_handle
>> -aux = errno
>> +aux = errno rseq
>> elide-routines.os = libc-tls
>> static-only-routines = elf-init
>> csu-dummies = $(filter-out $(start-installed-name),crt1.o Mcrt1.o)
>
> Do we plan to add Hurd support for this?
No.
A logical path where we could move rseq.c is under sysdeps/unix/sysv/linux/rseq.c.
This would allow the __rseq_abi symbol to be used from anywhere in glibc.
>
>> diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h
>> b/sysdeps/unix/sysv/linux/rseq-internal.h
>> new file mode 100644
>> index 0000000000..2367926def
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/rseq-internal.h
>
>> +#define RSEQ_SIG 0x53053053
>
> What's this? This needs a comment.
I will move it to an installed header (sysdeps/unix/sysv/linux/sys/rseq.h)
with the following comment:
/* Signature required before each abort handler code. */
#define RSEQ_SIG 0x53053053
>
>> +extern __thread volatile struct rseq __rseq_abi
>> +__attribute__ ((tls_model ("initial-exec")));
>> +
>> +extern __thread volatile uint32_t __rseq_refcount
>> +__attribute__ ((tls_model ("initial-exec")));
>
> The volatile qualifier needs justification in a comment. (Usually,
> volatile is wrong. and it is difficult to get rid of it.)
>
> We need to document these public symbols somewhere. There should be an
> installed header file.
Moving to sysdeps/unix/sysv/linux/sys/rseq.h with the following comments:
/* volatile because fields can be read/updated by the kernel. */
extern __thread volatile struct rseq __rseq_abi
__attribute__ ((tls_model ("initial-exec")));
/* volatile because refcount can be read/updated by signal handlers. */
extern __thread volatile uint32_t __rseq_refcount
__attribute__ ((tls_model ("initial-exec")));
>
>> diff --git a/nptl/Versions b/nptl/Versions
>> index e7f691da7a..f7890f73fc 100644
>> --- a/nptl/Versions
>> +++ b/nptl/Versions
>> @@ -277,6 +277,10 @@ libpthread {
>> cnd_timedwait; cnd_wait; tss_create; tss_delete; tss_get; tss_set;
>> }
>>
>> + GLIBC_2.29 {
>> + __rseq_refcount;
>> + }
>
> Why put this into libpthread, and __rseq_abi into libc?
The __rseq_abi symbol should be available to the glibc memory allocator.
I plan to move the __rseq_abi to sysdeps/unix/sysv/linux/Versions instead
so it becomes Linux-specific.
The __rseq_refcount symbol only needs to be made available to applications
and libraries linking against libpthread, because only libpthread actually
handles the rseq registration/unregistration at thread start/exit and
library initialization.
However, considering that we want this to be Linux-specific as well,
I could move it to sysdeps/unix/sysv/linux/Versions too.
Then it would make sense to move the __rseq_refcount symbol defined in
nptl/rseq.c to sysdeps/unix/sysv/linux/rseq.c as well and group
everything together.
Therefore, both symbols will end up in sysdeps/unix/sysv/linux/Versions.
>
> What, exactly, is the benefit of having __rseq_refcount defined by
> glibc? Have you actually got this working? If an rseq library is
> linked against glibc 2.29, it will reference the GLIBC_2.29 symbol
> version, so it cannot be loaded by older glibcs. In this case,
> __rseq_refcount is not needed.
>
> If you build against pre-2.29, then the __rseq_refcount symbol will be
> unversioned. But then you don't need it glibc, either.
>
> So it seems to me that the addition to glibc is useless in both
> scenarios. Am I missing something?
Here is the scenario where it becomes useful:
librseq is built against a pre-2.29 glibc. So the __rseq_refcount symbol
it emits is unversioned. Application is build against 2.29 glibc.
Application links both against librseq (itself built against pre-2.29 glibc)
and glibc (2.29).
In that scenario, librseq and glibc rely on a unique __rseq_refcount TLS
variable per process ensure that they don't register rseq twice for each thread.
>
> By the way, you could avoid the need for unregistration if you allocated
> the rseq areas persistently, index by TID. They are quite small, so
> with the typical PID range, maybe the wasted memory due to changing TIDs
> would be acceptable?
Would we be able to access those __rseq_abi as normal TLS IE model variables ?
The overhead of indexing an array matters for a fast-path.
>
> I guess things would be so much easier if the kernel simply provided a
> means to obtain the address of a previously registered rseq area.
Even if the kernel did provide this (which is not part of the syscall ABI anyway),
I suspect we would need extra code on the fast-path to access the __rseq_abi
TLS, which I would very much like to avoid. But perhaps there are ways to do
this without extra overhead that are beyond my understanding of glibc handling
of TLS models.
I will soon post an updated patch set taking care of your comments.
Thanks!
Mathieu
>
> Thanks,
> Florian
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
next prev parent reply other threads:[~2019-01-10 20:32 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-04 19:21 [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4) Mathieu Desnoyers
2018-12-04 19:21 ` [RFC PATCH glibc 2/4] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux Mathieu Desnoyers
2018-12-04 19:44 ` [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4) Florian Weimer
2018-12-04 19:50 ` Mathieu Desnoyers
2018-12-11 10:40 ` Florian Weimer
2019-01-10 20:31 ` Mathieu Desnoyers [this message]
2019-01-11 1:11 ` Mathieu Desnoyers
2019-01-14 18:57 ` Florian Weimer
2019-01-14 18:55 ` Florian Weimer
2019-01-14 19:22 ` Mathieu Desnoyers
2019-01-14 19:37 ` Florian Weimer
2019-01-14 20:27 ` Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1681283664.1380.1547152315426.JavaMail.zimbra@efficios.com \
--to=mathieu.desnoyers@efficios.com \
--cc=bmaurer@fb.com \
--cc=boqun.feng@gmail.com \
--cc=carlos@redhat.com \
--cc=dalias@libc.org \
--cc=davejwatson@fb.com \
--cc=fweimer@redhat.com \
--cc=joseph@codesourcery.com \
--cc=libc-alpha@sourceware.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=szabolcs.nagy@arm.com \
--cc=tglx@linutronix.de \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).