On Sun, Feb 28, 2016 at 12:39:54AM +0000, Mathieu Desnoyers wrote:
/* This structure needs to be aligned cache line size. */
struct thread_local_abi {
int32_t cpu_id;
uint32_t rseq_seqnum;
uint64_t rseq_post_commit_ip;
/* Add new fields at the end. */
} __attribute__((packed));
I would really not use packed; that can lead to horrible layout.
Suppose someone would add:
uint32_t foo;
uint64_t bar;
With packed, you get an unaligned uint64_t in there, which is horrible.
Without packed, you get a hole, which you can later fill.
/* Thread local ABI system calls. */
int thread_local_abi_len(size_t *features_mask_len, size_t *tlabi_len);
See below; maybe we can fudge the register call to return the size when
called 'right', maybe that'll end up too ugly, dunno. But I don't think
we need the feature mask bits.
Maybe: TLA_FLAG_GETSIZE ?
int thread_local_abi_features(uint8_t *mask);
Not sure you need this; see below. Either you know about a
TLA_ENABLE_feat flag and you can attempt enabling it (failing if the
kernel doesn't support it), or you don't, in which case you won't
attempt use.
int thread_local_abi_register(struct thread_local_abi *tlabi);
This has the problem that the moment you register for this, we must have
all features enabled. And esp. the rseq stuff has non-trivial overhead.
I would much rather have something where we only enable the features
actually used by the program at hand.
Also, every syscall should have a flags argument, so maybe we can do
something like:
#define TLA_ENABLE_CPU 0x01
#define TLA_ENABLE_RSEQ 0x03 /* RSEQ must imply CPU */
int thread_local_abi_register(struct tla *tla, unsigned int enable, unsigned int flags);
Where (g)libc would unconditionally set up the structure with
.enabled=0, .flags=0, and anybody actually wanting to make use of the
thing do:
thread_local_abi_register(NULL, TLA_ENABLE_CPU, 0);
Obviously calling register with !NULL address twice will error (you
already registered), calling with NULL before !NULL will also error.
And if you really worry about running out of feature bits, we could of
course pass it in a mask, but I'm not sure I can see 30 other features
we would want to cram into this (yes, yes, famous last words etc.. 640kb
anyone?).
On Monday 29 February 2016 11:32:21 Peter Zijlstra wrote:
On Sun, Feb 28, 2016 at 12:39:54AM +0000, Mathieu Desnoyers wrote:
quoted
/* This structure needs to be aligned cache line size. */
struct thread_local_abi {
int32_t cpu_id;
uint32_t rseq_seqnum;
uint64_t rseq_post_commit_ip;
/* Add new fields at the end. */
} __attribute__((packed));
I would really not use packed; that can lead to horrible layout.
Suppose someone would add:
uint32_t foo;
uint64_t bar;
With packed, you get an unaligned uint64_t in there, which is horrible.
Without packed, you get a hole, which you can later fill.
What's making things worse is that on some architectures, adding
__packed will force access by bytes rather than just reading
a 32-bit or 64-bit numbers directly, so it's slow and non-atomic.
Arnd
----- On Feb 29, 2016, at 5:39 AM, Arnd Bergmann arnd-r2nGTMty4D4@public.gmane.org wrote:
On Monday 29 February 2016 11:32:21 Peter Zijlstra wrote:
quoted
On Sun, Feb 28, 2016 at 12:39:54AM +0000, Mathieu Desnoyers wrote:
quoted
/* This structure needs to be aligned cache line size. */
struct thread_local_abi {
int32_t cpu_id;
uint32_t rseq_seqnum;
uint64_t rseq_post_commit_ip;
/* Add new fields at the end. */
} __attribute__((packed));
I would really not use packed; that can lead to horrible layout.
Suppose someone would add:
uint32_t foo;
uint64_t bar;
With packed, you get an unaligned uint64_t in there, which is horrible.
Without packed, you get a hole, which you can later fill.
Actually, Peter is wrong about the hole there. On some 32-bit architectures,
64-bit integers are aligned on 32-bit, not 64-bit. So there may or may not
be a hole there, and that would lead to a mess.
What's making things worse is that on some architectures, adding
__packed will force access by bytes rather than just reading
a 32-bit or 64-bit numbers directly, so it's slow and non-atomic.
Agreed that many architectures issue slower instructions when reading
from packed structures, which is unwanted.
Could we require that each field be naturally aligned and require that
they are placed so _no_ padding whatsoever should ever be added by the
compiler ? If that's possible, then we could remove the packed.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Monday 29 February 2016 12:41:49 Mathieu Desnoyers wrote:
----- On Feb 29, 2016, at 5:39 AM, Arnd Bergmann arnd-r2nGTMty4D4@public.gmane.org wrote:
quoted
What's making things worse is that on some architectures, adding
__packed will force access by bytes rather than just reading
a 32-bit or 64-bit numbers directly, so it's slow and non-atomic.
Agreed that many architectures issue slower instructions when reading
from packed structures, which is unwanted.
Could we require that each field be naturally aligned and require that
they are placed so _no_ padding whatsoever should ever be added by the
compiler ? If that's possible, then we could remove the packed.
Yes, I think that is a reasonable requirement.
Arnd
On February 29, 2016 4:41:49 AM PST, Mathieu Desnoyers [off-list ref] wrote:
Agreed that many architectures issue slower instructions when reading
from packed structures, which is unwanted.
And detrimental to atomicity.
Could we require that each field be naturally aligned and require that
they are placed so _no_ padding whatsoever should ever be added by the
compiler ? If that's possible, then we could remove the packed.
What people have been trying to tell you is that we *must* do this, and no compiler truck like packed will help.
--
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
On Mon, Feb 29, 2016 at 1:41 PM, Mathieu Desnoyers
[off-list ref] wrote:
----- On Feb 29, 2016, at 5:39 AM, Arnd Bergmann arnd-r2nGTMty4D4@public.gmane.org wrote:
quoted
On Monday 29 February 2016 11:32:21 Peter Zijlstra wrote:
quoted
On Sun, Feb 28, 2016 at 12:39:54AM +0000, Mathieu Desnoyers wrote:
quoted
/* This structure needs to be aligned cache line size. */
struct thread_local_abi {
int32_t cpu_id;
uint32_t rseq_seqnum;
uint64_t rseq_post_commit_ip;
/* Add new fields at the end. */
} __attribute__((packed));
I would really not use packed; that can lead to horrible layout.
Suppose someone would add:
uint32_t foo;
uint64_t bar;
With packed, you get an unaligned uint64_t in there, which is horrible.
Without packed, you get a hole, which you can later fill.
Actually, Peter is wrong about the hole there. On some 32-bit architectures,
64-bit integers are aligned on 32-bit, not 64-bit. So there may or may not
... or even on 16-bit.
be a hole there, and that would lead to a mess.
indeed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds