Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
- On Jan 14, 2019, at 2:37 PM, Florian Weimer fwei...@redhat.com wrote: > * Mathieu Desnoyers: > >> - On Jan 14, 2019, at 10:55 AM, Florian Weimer fwei...@redhat.com wrote: >> >>> * Mathieu Desnoyers: >>> Therefore, both symbols will end up in sysdeps/unix/sysv/linux/Versions. >>> >>> I'm not sure what you mean by that. The physical location in the >>> directory tree has little effect on which shared object the symbol is >>> placed in; that will need other changes. >> >> I'm currently moving the symbol definitions to csu/rseq-sym.c. On Linux, >> its content is overridden by a new sysdeps/unix/sysv/linux/rseq-sym.c >> which contains both __rseq_abi and __rseq_refcount symbols. On other >> platforms, it is a stub file. > > You don't need a stub file if you use the “ifeq ($(subdir),csu)” > construct. OK > > The other question is whether this belongs into the csu subdirectory. > Since TLS is not available in ld.so, the initialization would have to > happen rather late, after relocation, but before ELF constructors are > run. > > (A side effect is that the rseq area would not be usable from IFUNC > resolvers.) Do you have a specific directory location in mind where we should put the built object ? e.g. "ifeq ($(subdir),posix)" or "ifeq ($(subdir),misc)" ? Moreover, from where should we call the rseq initialization ? I'm having trouble with invalid system calls parameters if I place it in LIBC_START_MAIN() just before or after the call to __pthread_initialize_minimal. I get what appears to be invalid parameters to sys_rseq, possibly due to stack corruption (?). I'm investigating at the moment. But if you prefer we call the rseq init from elsewhere, please let me know. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
* Mathieu Desnoyers: > - On Jan 14, 2019, at 10:55 AM, Florian Weimer fwei...@redhat.com wrote: > >> * Mathieu Desnoyers: >> >>> Therefore, both symbols will end up in >>> sysdeps/unix/sysv/linux/Versions. >> >> I'm not sure what you mean by that. The physical location in the >> directory tree has little effect on which shared object the symbol is >> placed in; that will need other changes. > > I'm currently moving the symbol definitions to csu/rseq-sym.c. On Linux, > its content is overridden by a new sysdeps/unix/sysv/linux/rseq-sym.c > which contains both __rseq_abi and __rseq_refcount symbols. On other > platforms, it is a stub file. You don't need a stub file if you use the “ifeq ($(subdir),csu)” construct. The other question is whether this belongs into the csu subdirectory. Since TLS is not available in ld.so, the initialization would have to happen rather late, after relocation, but before ELF constructors are run. (A side effect is that the rseq area would not be usable from IFUNC resolvers.) Thanks, Florian
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
- On Jan 14, 2019, at 10:55 AM, Florian Weimer fwei...@redhat.com wrote: > * Mathieu Desnoyers: > >> Therefore, both symbols will end up in >> sysdeps/unix/sysv/linux/Versions. > > I'm not sure what you mean by that. The physical location in the > directory tree has little effect on which shared object the symbol is > placed in; that will need other changes. I'm currently moving the symbol definitions to csu/rseq-sym.c. On Linux, its content is overridden by a new sysdeps/unix/sysv/linux/rseq-sym.c which contains both __rseq_abi and __rseq_refcount symbols. On other platforms, it is a stub file. >>> 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. > > No, that wouldn't be possible in this case. You would need another > indirection. Thanks for the clarification! Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
* Mathieu Desnoyers: > Now that I think about it, it's important to move the rseq registration > done at nptl init (in my current code) to some lower-level csu initialiation, > so applications that happen _not_ to link against libpthread also get > registered rseq for the main thread. Yes. In general, we want to avoid to force libraries which do not create threads to link against libpthread, and try to provide interfaces which are required for synchronization within libc. Thanks, Florian
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
* Mathieu Desnoyers: > Therefore, both symbols will end up in > sysdeps/unix/sysv/linux/Versions. I'm not sure what you mean by that. The physical location in the directory tree has little effect on which shared object the symbol is placed in; that will need other changes. >> 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. No, that wouldn't be possible in this case. You would need another indirection. Thanks, Florian
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
- On Jan 10, 2019, at 12:31 PM, Mathieu Desnoyers
mathieu.desnoy...@efficios.com wrote:
> - On Dec 11, 2018, at 2:40 AM, Florian Weimer fwei...@redhat.com wrote:
[...]
>>
>>> 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.
Now that I think about it, it's important to move the rseq registration
done at nptl init (in my current code) to some lower-level csu initialiation,
so applications that happen _not_ to link against libpthread also get
registered rseq for the main thread.
Does it make sense ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
- On Dec 11, 2018, at 2:40 AM, Florian Weimer fwei...@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 00..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
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
* 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.
> 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?
> diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h
> b/sysdeps/unix/sysv/linux/rseq-internal.h
> new file mode 100644
> index 00..2367926def
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/rseq-internal.h
> +#define RSEQ_SIG 0x53053053
What's this? This needs a comment.
> +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.
> 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?
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?
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?
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.
Thanks,
Florian
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
- On Dec 4, 2018, at 2:44 PM, Florian Weimer fwei...@redhat.com wrote: > * Mathieu Desnoyers: > >> diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> index c370fda73d..9da78d59d2 100644 >> --- a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> @@ -235,3 +235,4 @@ GLIBC_2.28 tss_create F >> GLIBC_2.28 tss_delete F >> GLIBC_2.28 tss_get F >> GLIBC_2.28 tss_set F >> +GLIBC_2.29 __rseq_refcount T 0x4 > > This part looks buggy. > > It's unclear based on this patch whether you actually want to get rid of > the __rseq_refcount symbol. 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. Why does it look buggy ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
- On Dec 4, 2018, at 2:44 PM, Florian Weimer fwei...@redhat.com wrote: > * Mathieu Desnoyers: > >> diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> index c370fda73d..9da78d59d2 100644 >> --- a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist >> @@ -235,3 +235,4 @@ GLIBC_2.28 tss_create F >> GLIBC_2.28 tss_delete F >> GLIBC_2.28 tss_get F >> GLIBC_2.28 tss_set F >> +GLIBC_2.29 __rseq_refcount T 0x4 > > This part looks buggy. > > It's unclear based on this patch whether you actually want to get rid of > the __rseq_refcount symbol. 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. Why does it look buggy ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
* Mathieu Desnoyers: > diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > index c370fda73d..9da78d59d2 100644 > --- a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > @@ -235,3 +235,4 @@ GLIBC_2.28 tss_create F > GLIBC_2.28 tss_delete F > GLIBC_2.28 tss_get F > GLIBC_2.28 tss_set F > +GLIBC_2.29 __rseq_refcount T 0x4 This part looks buggy. It's unclear based on this patch whether you actually want to get rid of the __rseq_refcount symbol. Thanks, Florian
Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at nptl init and thread creation (v4)
* Mathieu Desnoyers: > diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > index c370fda73d..9da78d59d2 100644 > --- a/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist > @@ -235,3 +235,4 @@ GLIBC_2.28 tss_create F > GLIBC_2.28 tss_delete F > GLIBC_2.28 tss_get F > GLIBC_2.28 tss_set F > +GLIBC_2.29 __rseq_refcount T 0x4 This part looks buggy. It's unclear based on this patch whether you actually want to get rid of the __rseq_refcount symbol. Thanks, Florian
