Re: [RFC PATCH for 4.21 01/16] rseq/selftests: Add reference counter to coexist with glibc

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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

> On 11/10/18 17:37, Mathieu Desnoyers wrote:
>> ----- On Oct 11, 2018, at 12:20 PM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>>> On 11/10/18 16:13, Mathieu Desnoyers wrote:
>>>> ----- On Oct 11, 2018, at 6:37 AM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>>>>> On 10/10/18 20:19, Mathieu Desnoyers wrote:
>>>>>> +__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