From: Florian Weimer <fw@deneb.enyo.de>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>,
libc-alpha <libc-alpha@sourceware.org>,
carlos <carlos@redhat.com>, Rich Felker <dalias@libc.org>,
linux-api <linux-api@vger.kernel.org>,
Boqun Feng <boqun.feng@gmail.com>,
Will Deacon <will.deacon@arm.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ben Maurer <bmaurer@fb.com>, Dave Watson <davejwatson@fb.com>,
Thomas Gleixner <tglx@linutronix.de>,
Paul <paulmck@linux.vnet.ibm.com>, Paul Turner <pjt@google.com>,
Joseph Myers <joseph@codesourcery.com>,
Szabolcs Nagy <szabolcs.nagy@arm.com>
Subject: Re: [PATCH glibc 5/9] glibc: Perform rseq(2) registration at C startup and thread creation (v17)
Date: Mon, 27 Apr 2020 18:54:04 +0200 [thread overview]
Message-ID: <87zhawvphv.fsf@mid.deneb.enyo.de> (raw)
In-Reply-To: <284293396.70630.1588005648556.JavaMail.zimbra@efficios.com> (Mathieu Desnoyers's message of "Mon, 27 Apr 2020 12:40:48 -0400 (EDT)")
* Mathieu Desnoyers:
>>> +#include <sys/syscall.h>
>>> +#include <stdint.h>
>>> +#include <kernel-features.h>
>>> +#include <sys/rseq.h>
>>> +
>>> +__thread struct rseq __rseq_abi = {
>>> + .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
>>> +};
>>
>> { should go onto its own line.
>
> OK
>
>> I'd also add attribute_tls_model_ie,
>> also it's implied by the declaration in the header.
>
> This contradicts feedback I received from Szabolcs Nagy in September 2019:
>
> https://public-inbox.org/libc-alpha/c58d4d6e-f22a-f5d9-e23a-5bd72cec1a86@arm.com/
>
> "note that libpthread.so is built with -ftls-model=initial-exec
>
> (and if it wasn't then you'd want to put the attribute on the
> declaration in the internal header file, not on the definition,
> so the actual tls accesses generate the right code)"
>
> In the context of his feedback, __rseq_abi was defined within nptl/pthread_create.c.
> It is now defined in sysdeps/unix/sysv/linux/rseq-sym.c, which is built into the
> csu which AFAIU ends up in libc.so. His comment still applies though, because
> libc.so is also built with -ftls-model=initial-exec.
>
> So should I apply the "initial-exec" TLS model only to the __rseq_abi
> declaration, or is it preferred to apply it to both the declaration
> and the definition ?
I do not have a strong preference here. Technically, the declaration
in the header file should be enough.
>>> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h
>>> b/sysdeps/unix/sysv/linux/sys/rseq.h
>>> new file mode 100644
>>> index 0000000000..503dce4cac
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h
>>> @@ -0,0 +1,186 @@
>>
>> I think there is some value in making this header compatible with
>> inclusion from the assembler (including constants for the relevant
>> struct offsets), but that can be a later change.
>
> Agreed. By "later", do you mean before merging the patch, between
> merge of the patch and next glibc release, or for a subsequent glibc
> release ?
It can happen some time after merging the patch, preferably for this
release. But I don't think it's release-critical.
>>> +/* struct rseq_cs is aligned on 4 * 8 bytes to ensure it is always
>>> + contained within a single cache-line. It is usually declared as
>>> + link-time constant data. */
>>> +struct rseq_cs
>>> + {
>>> + /* Version of this structure. */
>>> + uint32_t version;
>>> + /* enum rseq_cs_flags. */
>>> + uint32_t flags;
>>> + uint64_t start_ip;
>>> + /* Offset from start_ip. */
>>> + uint64_t post_commit_offset;
>>> + uint64_t abort_ip;
>>> +} __attribute__((aligned(4 * sizeof(uint64_t))));
>>
>> The comment is wrong. 32-byte alignment does not put struct rseq_cs
>> on its own cache line on many (most?) CPUs. Not using the constant 32
>> looks like unnecessary obfuscation to me.
>
> There is a difference between "being contained within a single cache-line"
> and "being the only structure in a cache-line". The objective here is the
> former.
Fair enough.
> I agree that the constant 32 may be clearer here. I will change to align(32).
With a space, please. 8-)
>>> +/* struct rseq is aligned on 4 * 8 bytes to ensure it is always
>>> + contained within a single cache-line.
>>> +
>>> + A single struct rseq per thread is allowed. */
>>> +struct rseq
>>> + {
>>> + /* Restartable sequences cpu_id_start field. Updated by the
>>> + kernel. Read by user-space with single-copy atomicity
>>> + semantics. This field should only be read by the thread which
>>> + registered this data structure. Aligned on 32-bit. Always
>>
>> What does “Aligned on 32-bit” mean in this context? Do you mean to
>> reference 32-*byte* alignment here?
>
> No. I really mean 32-bit (4-byte). Being aligned on 32-byte guarantees that
> this field is aligned at least on 4-byte. This is required by single-copy
> atomicity semantics.
>
> Should I update this comment to state "Aligned on 4-byte" instead ?
I think this is implied by all Linux ABIs. And the explicit alignment
specification for struct rseq makes the alignment 32 bytes.
>>> + /* Restartable sequences rseq_cs field.
>>> +
>>> + Contains NULL when no critical section is active for the current
>>> + thread, or holds a pointer to the currently active struct rseq_cs.
>>> +
>>> + Updated by user-space, which sets the address of the currently
>>> + active rseq_cs at the beginning of assembly instruction sequence
>>> + block, and set to NULL by the kernel when it restarts an assembly
>>> + instruction sequence block, as well as when the kernel detects that
>>> + it is preempting or delivering a signal outside of the range
>>> + targeted by the rseq_cs. Also needs to be set to NULL by user-space
>>> + before reclaiming memory that contains the targeted struct rseq_cs.
>>> +
>>> + Read and set by the kernel. Set by user-space with single-copy
>>> + atomicity semantics. This field should only be updated by the
>>> + thread which registered this data structure. Aligned on 64-bit. */
>>> + union {
>>> + uint64_t ptr64;
>>> +#ifdef __LP64__
>>> + uint64_t ptr;
>>> +#else
>>> + struct {
>>> +#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
>>> defined(__BIG_ENDIAN)
>>> + uint32_t padding; /* Initialized to zero. */
>>> + uint32_t ptr32;
>>> +#else /* LITTLE */
>>> + uint32_t ptr32;
>>> + uint32_t padding; /* Initialized to zero. */
>>> +#endif /* ENDIAN */
>>> + } ptr;
>>> +#endif
>>> + } rseq_cs;
>>
>> Are these conditionals correct for x32?
>
> Let's see. With x86 gcc:
>
> -m64: (__x86_64__ && __LP64__)
> -m32: (__i386__)
> -mx32: (__x86_64__ && __ILP32__)
>
> So with "#ifdef __LP64__" we specifically target 64-bit pointers. The rest
> falls into the "else" case, which expects 32-bit pointers. Considering that
> x32 has 32-bit pointers, I don't see any issue here.
Does the kernel have a separate 32-bit entry point for rseq on x32?
If not, it will expect the 64-bit struct layout.
> We don't mind that user-space uses that pointer, but we never want the kernel
> to touch that pointer rather than the 32/64-bit-aware fields. One possibility
> would be to do:
>
> union
> {
> uint64_t ptr64;
> #ifdef __LP64__
> uint64_t ptr;
> #else
> struct
> {
> #if (defined (__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined (__BIG_ENDIAN)
> uint32_t padding; /* Initialized to zero. */
> uint32_t ptr32;
> #else /* LITTLE */
> uint32_t ptr32;
> uint32_t padding; /* Initialized to zero. */
> #endif /* ENDIAN */
> } ptr;
> #endif
>
> #ifndef __KERNEL__
> const struct rseq_cs *uptr;
> #endif
> } rseq_cs;
>
> in the union, so only user-space can see that field. Thoughts ?
I think this depends on where the x32 question lands.
next prev parent reply other threads:[~2020-04-27 16:54 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200326155633.18236-1-mathieu.desnoyers@efficios.com>
2020-03-26 15:56 ` [PATCH glibc 5/9] glibc: Perform rseq(2) registration at C startup and thread creation (v17) Mathieu Desnoyers
2020-04-27 9:11 ` Florian Weimer
2020-04-27 16:40 ` Mathieu Desnoyers
2020-04-27 16:54 ` Florian Weimer [this message]
2020-04-27 17:26 ` Mathieu Desnoyers
2020-04-27 20:27 ` Mathieu Desnoyers
2020-04-28 12:02 ` Florian Weimer
2020-04-28 12:33 ` Mathieu Desnoyers
2020-04-28 12:35 ` Florian Weimer
2020-04-28 12:43 ` Mathieu Desnoyers
2020-04-28 12:54 ` Florian Weimer
2020-04-28 14:58 ` Mathieu Desnoyers
2020-04-29 8:16 ` Szabolcs Nagy
2020-04-29 8:18 ` Florian Weimer
2020-04-29 8:52 ` Szabolcs Nagy
2020-04-28 12:56 ` Mathieu Desnoyers
2020-04-29 12:19 ` Florian Weimer
2020-04-27 11:59 ` Florian Weimer
2020-04-27 16:47 ` Mathieu Desnoyers
2020-04-27 16:59 ` Florian Weimer
2020-03-26 15:56 ` [PATCH glibc 6/9] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) Mathieu Desnoyers
2020-04-27 9:13 ` Florian Weimer
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=87zhawvphv.fsf@mid.deneb.enyo.de \
--to=fw@deneb.enyo.de \
--cc=bmaurer@fb.com \
--cc=boqun.feng@gmail.com \
--cc=carlos@redhat.com \
--cc=dalias@libc.org \
--cc=davejwatson@fb.com \
--cc=joseph@codesourcery.com \
--cc=libc-alpha@sourceware.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mtk.manpages@gmail.com \
--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).