----- On Oct 11, 2018, at 1:04 PM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:

On 11/10/18 17:37, Mathieu Desnoyers wrote:

quoted

----- On Oct 11, 2018, at 12:20 PM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:

quoted

On 11/10/18 16:13, Mathieu Desnoyers wrote:

quoted

----- On Oct 11, 2018, at 6:37 AM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:

quoted

On 10/10/18 20:19, Mathieu Desnoyers wrote:

quoted

+__attribute__((visibility("hidden"))) __thread
+volatile struct libc_rseq __lib_rseq_abi = {

...

but it's in a magic struct that's called "abi" which is confusing,
the counter is not abi, it's in a hidden object.

No, it is really an ABI between user-space apps/libs. It's not meant to be
hidden. glibc implements its own register/unregister functions (it does not
link against librseq). librseq exposes register/unregister functions as public
APIs. Those also use the refcount. I also plan to have existing libraries, e.g.
liblttng-ust and possibly liburcu flavors, implement the
registration/unregistration and refcount handling on their own, so we don't
have to add a requirement on additional linking on librseq for pre-existing
libraries.

So that refcount is not an ABI between kernel and user-space, but it's a
user-space ABI nevertheless (between program and shared objects).

if that's what you want, then your declaration is wrong.
the object should not have hidden visibility.

Actually, if we look closer into my patch, it defines two symbols,
one of which is an alias:

__attribute__((visibility("hidden"))) __thread
volatile struct libc_rseq __lib_rseq_abi = {
        .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
};

extern __attribute__((weak, alias("__lib_rseq_abi"))) __thread
volatile struct rseq __rseq_abi;

Note that the public __rseq_abi symbol is weak but does not have
hidden visibility. I do this to ensure I don't get prototype
mismatch for __rseq_abi between rseq.c and rseq.h (it is required
to be a struct rseq by rseq.h), but I want the space to hold the
extra refcount field present in struct libc_rseq.

then each library (glibc etc) will have its own separate
tls object with their own separate refcounter (and they
will unregister when their own refcounter hits 0)

Given they all interact with the public __rseq_abi symbol,
at field refcount offset, they all effectively use the same
refcount field per thread, which serves the intended purpose.

either the struct should be public abi (extern tls
symbol) or the register/unregister functions should
be public abi (so when multiple implementations are
present in the same process only one of them will
provide definition for the public abi symbol and
thus there will be one refcounter).

Those are two possible solutions, indeed. Considering that
we already need to expose the __rseq_abi symbol as a public
ABI in a way that ensures that multiple implementations
in a same process end up only using one of them, it seems
straightforward to simply extend that structure and hold the
refcount there, rather than having two extra ABI symbols
(register/unregister functions).

One very appropriate question here is whether we want to
expose the layout of struct libc_rseq (which includes the
refcount) in a public header file, and if so, which project
should hold it ? Or do we just want to document the layout
of this ABI so projects can define the structure layout
internally ? As my implementation currently stands, I have
the following structure duplicated into rseq selftests,
librseq, and glibc:

/*
 * linux/rseq.h defines struct rseq as aligned on 32 bytes. The kernel ABI
 * size is 20 bytes. For support of multiple rseq users within a process,
 * user-space defines an extra 4 bytes field as a reference count, for a
 * total of 24 bytes.
 */
struct libc_rseq {
        /* kernel-userspace ABI. */
        __u32 cpu_id_start;
        __u32 cpu_id;
        __u64 rseq_cs;
        __u32 flags;
        /* user-space ABI. */
        __u32 refcount;
} __attribute__((aligned(4 * sizeof(__u64))));

That duplicated structure only needs to be present in early-adopter
applications/libraries. Those linking on librseq or relying on newer
glibc to register rseq don't need to know about this extended layout:
all they need to care about is the layout of struct rseq (without the
added refcount). 

Thanks,

Mathieu


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