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
[off-list ref] 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

----- On Feb 27, 2016, at 7:57 PM, Linus Torvalds torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:

On Sat, Feb 27, 2016 at 4:39 PM, Mathieu Desnoyers
[off-list ref] wrote:

quoted

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

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.

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

On Sun, Feb 28, 2016 at 02:32:28PM +0000, Mathieu Desnoyers wrote:

quoted

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

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.

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

quoted

        /*
         * 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.

On 03/01/16 13:32, Peter Zijlstra wrote:

On Tue, Mar 01, 2016 at 08:23:12PM +0000, Mathieu Desnoyers wrote:

quoted

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

On Tue, Mar 01, 2016 at 01:47:38PM -0800, H. Peter Anvin wrote:

On 03/01/16 13:32, Peter Zijlstra wrote:

quoted

On Tue, Mar 01, 2016 at 08:23:12PM +0000, Mathieu Desnoyers wrote:

quoted

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