Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1341168 > unrolled thread

[PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-24 00:30 +0100
    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Thomas Gleixner <tglx@linutronix.de> - 2016-02-24 12:20 +0100
      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-24 18:20 +0100
      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-02-26 00:40 +0100
        Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-26 18:50 +0100
    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-25 11:00 +0100
      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-25 18:00 +0100
        Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-25 18:10 +0100
          Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-25 18:20 +0100
            Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-26 12:40 +0100
              Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Thomas Gleixner <tglx@linutronix.de> - 2016-02-26 17:40 +0100
                Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-26 18:30 +0100
                  Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Thomas Gleixner <tglx@linutronix.de> - 2016-02-26 19:10 +0100
                    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-26 21:30 +0100
                      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-02-27 00:10 +0100
                        Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-27 01:50 +0100
                          Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-02-27 07:30 +0100
                            Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-27 15:20 +0100
                              Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-27 16:00 +0100
                                Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-27 19:40 +0100
                                  Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-02-27 20:10 +0100
                                  Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-28 01:00 +0100
                                    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-28 02:00 +0100
                                      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-28 15:40 +0100
                                        Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-29 11:40 +0100
                                          Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-03-01 21:30 +0100
                                            Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-03-01 22:40 +0100
                                              Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-03-01 22:40 +0100
                                              Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-03-01 23:00 +0100
                                                Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-03-02 11:40 +0100
                                    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-29 11:40 +0100
                                      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread Arnd Bergmann <arnd@arndb.de> - 2016-02-29 11:50 +0100
                                        Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-02-29 13:50 +0100
                                          Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread Arnd Bergmann <arnd@arndb.de> - 2016-02-29 14:20 +0100
                                          Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-02-29 19:30 +0100
                                          Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-02 11:50 +0100
                                    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-03-01 19:30 +0100
                                      Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-03-01 19:50 +0100
                                  Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Geert Uytterhoeven <geert@linux-m68k.org> - 2016-02-28 14:10 +0100
                                    Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-28 17:30 +0100
                                  Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of  running thread Peter Zijlstra <peterz@infradead.org> - 2016-02-29 11:10 +0100
                              Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread "H. Peter Anvin" <hpa@zytor.com> - 2016-02-27 16:10 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1345067

On February 27, 2016 10:35:28 AM PST, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Sat, Feb 27, 2016 at 6:58 AM, Peter Zijlstra <peterz@infradead.org>
>wrote:
>>
>> Paul's patches have the following structure:
>>
>> struct thread_local_abi {
>>         union {
>>                 struct {
>>                         u32     cpu_id;
>>                         u32     seq;
>>                 };
>>                 u64 cpu_seq;
>>         };
>>         unsigned long post_commit_ip;
>> };
>
>Please don't do "unsigned long" in ABI structures any more.
>
>Make it u64, and make sure it is 64-bit aligned (which it would be in
>this case). Make it so that we don't have to have separate compat
>paths.
>
>               Linus

Yes, if we have to do compat crap for this entire new ABI path I think I'll scream.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

[toc] | [prev] | [next] | [standalone]

#1345117 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

----- On Feb 27, 2016, at 1:35 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Sat, Feb 27, 2016 at 6:58 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> Paul's patches have the following structure:
>>
>> struct thread_local_abi {
>>         union {
>>                 struct {
>>                         u32     cpu_id;
>>                         u32     seq;
>>                 };
>>                 u64 cpu_seq;
>>         };
>>         unsigned long post_commit_ip;
>> };
> 
> Please don't do "unsigned long" in ABI structures any more.
> 
> Make it u64, and make sure it is 64-bit aligned (which it would be in
> this case). Make it so that we don't have to have separate compat
> paths.

AFAIU, this "post_commit_ip" field is expected to be updated
with a single-copy-store by user-space. If we want to handle both
32-bit and 64-bit processes, how do you recommend doing this
without an unsigned long type ?

A 64-bit integer would not be a single-copy store for
32-bit processes, but a 32-bit integer would not be large
enough for 64-bit processes.

Would a

union {
    uint32_t val32;
    uint64_t val64;
} field;

be an acceptable option ? Then the kernel could use
one field or the other depending on the process bitness.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[toc] | [prev] | [next] | [standalone]

#1345128 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Sat, Feb 27, 2016 at 4:39 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
>
> I'm particularly interested to know what are the best practices to
> deal with an extensible bitfield (the features mask). cpu_set_t
> and sigmask each seem to do their own thing.

Quite frankly, why would the kernel ever touch anything else?

And if the kernel doesn't touch anything else, why make it part of the ABI?

I don't see why the kernel would ever want to have a more complex
interface. Explain.

           Linus

[toc] | [prev] | [next] | [standalone]

#1345286 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

----- On Feb 27, 2016, at 7:57 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Sat, Feb 27, 2016 at 4:39 PM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>>
>> I'm particularly interested to know what are the best practices to
>> deal with an extensible bitfield (the features mask). cpu_set_t
>> and sigmask each seem to do their own thing.
> 
> Quite frankly, why would the kernel ever touch anything else?
> 
> And if the kernel doesn't touch anything else, why make it part of the ABI?
> 
> I don't see why the kernel would ever want to have a more complex
> interface. Explain.

The part of ABI I'm trying to express here is for discoverability
of available features by user-space. For instance, a kernel
could be configured with "CONFIG_RSEQ=n", and userspace should
not rely on the rseq fields of the thread-local ABI in that case.

The initial idea I had was to populate a mask of available features
(hence my question above), but now that I think about it, we could
perhaps have a "query" system call receiving a "feature number", no
mask needed then. E.g.:

enum thread_local_abi_features {
    THREAD_LOCAL_FEATURE_CPU_ID = 0,
    THREAD_LOCAL_FEATURE_RSEQ = 1,
    /* Add future features here. */
};

int thread_local_abi_feature(uint64_t feature);

Another option would be to rely on specific "uninitialized"
values for each feature in struct thread_local_abi (e.g. -1
for cpu_id). We may need to reserve extra space for
"feature enabled" booleans in cases where the uninitialized
value is also used when initialized (e.g. a sequence counteR).
The advantage of using the uninitialized value and/or the
"boolean" within the struct thread_local_abi is that testing
whether the feature is active can be done by reading from
the same cache-line as when using the feature (in user-space).

Not sure what would be the best option here.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[toc] | [prev] | [next] | [standalone]

#1345634 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Sun, Feb 28, 2016 at 02:32:28PM +0000, Mathieu Desnoyers wrote:
> The part of ABI I'm trying to express here is for discoverability
> of available features by user-space. For instance, a kernel
> could be configured with "CONFIG_RSEQ=n", and userspace should
> not rely on the rseq fields of the thread-local ABI in that case.

Per the just proposed interface; discoverability would end with:

	thread_local_abi_register(NULL, TLA_ENABLE_RSEQ, 0);

failing. This would indicate your kernel does not support (or your glibc
failed to register, depending on error code I suppose).

Then your program can either fall back to full atomics or just bail.

[toc] | [prev] | [next] | [standalone]

#1347006 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

----- On Feb 29, 2016, at 5:35 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Sun, Feb 28, 2016 at 02:32:28PM +0000, Mathieu Desnoyers wrote:
>> The part of ABI I'm trying to express here is for discoverability
>> of available features by user-space. For instance, a kernel
>> could be configured with "CONFIG_RSEQ=n", and userspace should
>> not rely on the rseq fields of the thread-local ABI in that case.
> 
> Per the just proposed interface; discoverability would end with:
> 
>	thread_local_abi_register(NULL, TLA_ENABLE_RSEQ, 0);
> 
> failing. This would indicate your kernel does not support (or your glibc
> failed to register, depending on error code I suppose).
> 
> Then your program can either fall back to full atomics or just bail.

I think it's important that user-space fast-paths can quickly
detect whether the feature is enabled without having to rely on
always reading a separate cache-line. I've put together an ABI
proposal that take into account the feedback received so far.

The main trick here is to use "-1" value in cpu_id and rseq_seqnum
to mean "the feature is inactive" so user-space can call the system
call to register the feature, and the value "-2" can be set by the
kernel when it knows the feature is not available. It does mean
that seqnum would wrap from MAX_INT to 0 in the kernel, skipping
negative values.

Please let me know if I missed anything.

#ifdef __LP64__
# define TLABI_FIELD_u32_u64(field)     uint64_t field
#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
# define TLABI_FIELD_u32_u64(field)     uint32_t field, _padding ## field
#else
# define TLABI_FIELD_u32_u64(field)     uint32_t _padding ## field, field
#endif

/*
 * The thread-local ABI structure needs to be aligned at least on 32
 * bytes multiples.
 */
#define TLABI_ALIGNMENT         32

struct thread_local_abi {
        /*
         * Thread-local ABI cpu_id field.
         * Updated by the kernel, and read by user-space with
         * single-copy atomicity semantics. Aligned on 32-bit.
         * Values:
         * >= 0: CPU number of running thread.
         * -1 (initial value): means the cpu_id feature is inactive.
         * -2: cpu_id feature is not available.
         */
        int32_t cpu_id;

        /*
         * Thread-local ABI rseq_seqnum field.
         * Updated by the kernel, and read by user-space with
         * single-copy atomicity semantics. Aligned on 32-bit.
         * Values:
         * >= 0: current seqnum for this thread (feature is active).
         * -1 (initial value): means the rseq feature is inactive.
         * -2: rseq feature is not available.
         */
        int32_t rseq_seqnum;

        /*
         * Thread-local ABI rseq_post_commit_ip field.
         * Updated by user-space, and read by the kernel with
         * single-copy atomicity semantics.
         * Aligned on 64-bit.
         */
        TLABI_FIELD_u32_u64(rseq_post_commit_ip);

        /* Add new fields at the end. */
} __attribute__ ((aligned(TLABI_ALIGNMENT)));

enum thread_local_abi_feature {
        TLA_FEATURE_NONE = 0,
        TLA_FEATURE_CPU_ID = (1 << 0),
        TLA_FEATURE_RSEQ = (1 << 1),
};

/*
 * Thread local ABI system call.
 *
 * First call with (NULL, 0, 0), returns the size of the struct
 * thread_local_abi expected by the kernel, or -1 on error.
 *
 * Second, allocate a memory area to hold the struct thread_local_abi,
 * and call with (ptr, 0, 0). Returns 0 on success, or -1 on error.
 *
 * Third, enable specific features by passing a mask, e.g. call with
 * (NULL, TLA_FEATURE_CPU_ID | TLA_FEATURE_RSEQ, 0).
 * Returns 0 on success, -1 on error.
 *
 * Then the fields associated with the enabled features are managed by
 * the kernel.
 */
ssize_t thread_local_abi(struct thread_local_abi *tlabi,
                uint64_t feature_mask, int flags);

Thanks for your feedback!

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[toc] | [prev] | [next] | [standalone]

#1347035 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Tue, Mar 01, 2016 at 08:23:12PM +0000, Mathieu Desnoyers wrote:
> I think it's important that user-space fast-paths can quickly
> detect whether the feature is enabled without having to rely on
> always reading a separate cache-line. I've put together an ABI
> proposal that take into account the feedback received so far.

Nah, adding detectoring code to fast paths is silly, makes them less
fast. Doesn't userspace have self modifying code? I know that at least
glibc does linker trickery to call different functions depending on
runtime context.

> struct thread_local_abi {
>         /*
>          * Thread-local ABI cpu_id field.
>          * Updated by the kernel, and read by user-space with
>          * single-copy atomicity semantics. Aligned on 32-bit.
>          * Values:
>          * >= 0: CPU number of running thread.
>          * -1 (initial value): means the cpu_id feature is inactive.
>          * -2: cpu_id feature is not available.
>          */
>         int32_t cpu_id;
> 
>         /*
>          * Thread-local ABI rseq_seqnum field.
>          * Updated by the kernel, and read by user-space with
>          * single-copy atomicity semantics. Aligned on 32-bit.
>          * Values:
>          * >= 0: current seqnum for this thread (feature is active).
>          * -1 (initial value): means the rseq feature is inactive.
>          * -2: rseq feature is not available.
>          */
>         int32_t rseq_seqnum;

So I really hate that, that makes we have to check for these special
values whenever we increment the seq count and cannot have it wrap
naturally.

[toc] | [prev] | [next] | [standalone]

#1347037 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Tue, Mar 01, 2016 at 10:32:02PM +0100, Peter Zijlstra wrote:

> >         /*
> >          * Thread-local ABI rseq_seqnum field.
> >          * Updated by the kernel, and read by user-space with
> >          * single-copy atomicity semantics. Aligned on 32-bit.
> >          * Values:
> >          * >= 0: current seqnum for this thread (feature is active).
> >          * -1 (initial value): means the rseq feature is inactive.
> >          * -2: rseq feature is not available.
> >          */
> >         int32_t rseq_seqnum;
> 
> So I really hate that, that makes we have to check for these special
> values whenever we increment the seq count and cannot have it wrap
> naturally.

Also, since it will wrap, uint32_t is more natural, since the whole
signed overflow thing is somewhat undefined in C.

[toc] | [prev] | [next] | [standalone]

#1347056 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On 03/01/16 13:32, Peter Zijlstra wrote:
> On Tue, Mar 01, 2016 at 08:23:12PM +0000, Mathieu Desnoyers wrote:
>> I think it's important that user-space fast-paths can quickly
>> detect whether the feature is enabled without having to rely on
>> always reading a separate cache-line. I've put together an ABI
>> proposal that take into account the feedback received so far.
> 
> Nah, adding detectoring code to fast paths is silly, makes them less
> fast. Doesn't userspace have self modifying code? I know that at least
> glibc does linker trickery to call different functions depending on
> runtime context.
> 

No, userspace does not have self-modifying code.  The glibc indirect
function is done at dynamic link time; it is also worth noting that
resolving global symbols through dynamic linking often requires an
indirect call.

	-hpa

[toc] | [prev] | [next] | [standalone]

#1347938 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Tue, Mar 01, 2016 at 01:47:38PM -0800, H. Peter Anvin wrote:
> On 03/01/16 13:32, Peter Zijlstra wrote:
> > On Tue, Mar 01, 2016 at 08:23:12PM +0000, Mathieu Desnoyers wrote:
> >> I think it's important that user-space fast-paths can quickly
> >> detect whether the feature is enabled without having to rely on
> >> always reading a separate cache-line. I've put together an ABI
> >> proposal that take into account the feedback received so far.
> > 
> > Nah, adding detectoring code to fast paths is silly, makes them less
> > fast. Doesn't userspace have self modifying code? I know that at least
> > glibc does linker trickery to call different functions depending on
> > runtime context.
> > 
> 
> No, userspace does not have self-modifying code.  The glibc indirect
> function is done at dynamic link time; it is also worth noting that
> resolving global symbols through dynamic linking often requires an
> indirect call.

Boy that blows. And here I was thinking you could edit the code at
dynamic link time because nobody was running it yet :/

And I suppose JITs need an (effective) munmap()+mmap() cycle to ensure
the 'old' code is flushed from all caches etc..?

[toc] | [prev] | [next] | [standalone]

#1345631 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

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?).

[toc] | [prev] | [next] | [standalone]

#1345640

On Monday 29 February 2016 11:32:21 Peter Zijlstra wrote:
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1345763 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

----- On Feb 29, 2016, at 5:39 AM, Arnd Bergmann arnd@arndb.de wrote:

> On Monday 29 February 2016 11:32:21 Peter Zijlstra wrote:
>> 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.
> 

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

[toc] | [prev] | [next] | [standalone]

#1345796

On Monday 29 February 2016 12:41:49 Mathieu Desnoyers wrote:
> ----- On Feb 29, 2016, at 5:39 AM, Arnd Bergmann arnd@arndb.de wrote:

> > 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

[toc] | [prev] | [next] | [standalone]

#1346075

On February 29, 2016 4:41:49 AM PST, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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.

[toc] | [prev] | [next] | [standalone]

#1347947 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Mon, Feb 29, 2016 at 1:41 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- On Feb 29, 2016, at 5:39 AM, Arnd Bergmann arnd@arndb.de wrote:
>
>> On Monday 29 February 2016 11:32:21 Peter Zijlstra wrote:
>>> 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.
>>
>
> 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@linux-m68k.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

[toc] | [prev] | [next] | [standalone]

#1346851 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On 02/27/16 16:39, Mathieu Desnoyers wrote:
> 
> Very good points! Would the following interfaces be acceptable ?
> 
> /* This structure needs to be aligned cache line size. */
> struct thread_local_abi {
>         int32_t cpu_id;                               /* Aligned on
> 32-bit. */
>         uint32_t rseq_seqnum;                 /* Aligned on 32-bit. */
>         uint64_t rseq_post_commit_ip;   /* Aligned on 64-bit. */
>         /* Add new fields at the end. */
> } __attribute__((packed));
> 

First of all, DO NOT use __attribute__((packed)).  First of all, it
buggers up the alignment of the *entire structure* (the alignment of a
packed structure defaults to 1, and gcc will assume the whole structure
is misaligned, generating unaligned access instructions on architectures
which need them.)

Sadly gcc doesn't currently have an __attribute__ to express "error out
on padding" which is what you actually want here.

You may, however, want to add an explicit alignment attribute to make
sure it is cache line aligned.

Second, as far as the 32/64 bit issue is concerned, you have to order
the fields so you always access the LSB.  This is probably the best way
to do it:

#ifdef __LP64__
# define __FIELD_32_64(field,n)	uint64_t field;
#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
# define __FIELD_32_64(field,n) uint32_t field, _unused ## n;
#else
# define __FIELD_32_64(field,n) uint32_t _unused ## n, field;
#endif

All these macros are intrinsic to gcc (and hopefully to gcc-compatible
compilers) so there are no header file dependencies.

	-hpa

[toc] | [prev] | [next] | [standalone]

#1346878 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

----- On Mar 1, 2016, at 1:25 PM, H. Peter Anvin hpa@zytor.com wrote:

> On 02/27/16 16:39, Mathieu Desnoyers wrote:
>> 
>> Very good points! Would the following interfaces be acceptable ?
>> 
>> /* This structure needs to be aligned cache line size. */
>> struct thread_local_abi {
>>         int32_t cpu_id;                               /* Aligned on
>> 32-bit. */
>>         uint32_t rseq_seqnum;                 /* Aligned on 32-bit. */
>>         uint64_t rseq_post_commit_ip;   /* Aligned on 64-bit. */
>>         /* Add new fields at the end. */
>> } __attribute__((packed));
>> 
> 
> First of all, DO NOT use __attribute__((packed)).  First of all, it
> buggers up the alignment of the *entire structure* (the alignment of a
> packed structure defaults to 1, and gcc will assume the whole structure
> is misaligned, generating unaligned access instructions on architectures
> which need them.)
> 
> Sadly gcc doesn't currently have an __attribute__ to express "error out
> on padding" which is what you actually want here.

Good point.

> 
> You may, however, want to add an explicit alignment attribute to make
> sure it is cache line aligned.

Good idea, will do!

> 
> Second, as far as the 32/64 bit issue is concerned, you have to order
> the fields so you always access the LSB.  This is probably the best way
> to do it:
> 
> #ifdef __LP64__
> # define __FIELD_32_64(field,n)	uint64_t field;
> #elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> # define __FIELD_32_64(field,n) uint32_t field, _unused ## n;
> #else
> # define __FIELD_32_64(field,n) uint32_t _unused ## n, field;
> #endif
> 
> All these macros are intrinsic to gcc (and hopefully to gcc-compatible
> compilers) so there are no header file dependencies.

Thanks for the hint. I'll try it out.

Mathieu


> 
> 	-hpa

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[toc] | [prev] | [next] | [standalone]

#1345241 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

Hi Linus,

On Sat, Feb 27, 2016 at 7:35 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Feb 27, 2016 at 6:58 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> Paul's patches have the following structure:
>>
>> struct thread_local_abi {
>>         union {
>>                 struct {
>>                         u32     cpu_id;
>>                         u32     seq;
>>                 };
>>                 u64 cpu_seq;
>>         };
>>         unsigned long post_commit_ip;
>> };
>
> Please don't do "unsigned long" in ABI structures any more.
>
> Make it u64, and make sure it is 64-bit aligned (which it would be in
> this case). Make it so that we don't have to have separate compat
> paths.

__alignof__(u64) is not 8 on all architectures.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.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

[toc] | [prev] | [next] | [standalone]

#1345345 — Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

On Sun, Feb 28, 2016 at 5:07 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> __alignof__(u64) is not 8 on all architectures.

Indeed, which is why I said "make sure it's 64-bit aligned". We do it
manually for ABI structures (although we did have some discussion
about adding a alignment directive, and then having an explicitly
unaligned type for legacy cases that we got wrong).

In the above case it was already properly aligned, because the
previous structure members added up to 64-bit boundaries.

Of course, nothing then stops user space from giving us structures
that are unaligned to begin with, but that's not our problem. As long
as the layout is correct, we're fine, and that's all we care about.

              Linus

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web