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

On Sat, Feb 27, 2016 at 02:15:01PM +0000, Mathieu Desnoyers wrote:

I'm concerned that this thread-local ABI structure may become messy.
Let's just imagine how we would first introduce a "cpu_id" field (int32_t),
and eventually add a "seqnum" field for rseq in the future (unsigned long).

The rseq seq number can be uint32_t, in fact it is in Paul's patches.

(This is true because every seq increment will guarantee a userspace
exception and reload of the value, its impossible to wrap the thing and
get a false positive.)

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

Although he allows the post_commit_ip to be a separate field (which I
don't think makes sense).

/* This structure needs to be aligned on pointer size. */

I would mandate the thing be cacheline aligned, and sod packed, that can
lead to horrible layouts.

If the goal is really to keep the burden on the task struct
small, we could use kmalloc()/kfree() to allocate and free an
array of pointers to the various per-thread features, rather

*groan*, no that's even worse, then you get even more loads to update
the fields. The point is to reduce the total overhead of having this
stuff.

Having a single pointer with known offsets is best because then its
guaranteed a single load, then having the whole data structure in a
single cacheline again saves on memops, you can only miss once.