Date:   Thu, 11 Oct 2018 15:42:58 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Szabolcs Nagy <Szabolcs.Nagy@....com>
Cc:     nd <nd@....com>, Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Boqun Feng <boqun.feng@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-api <linux-api@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...capital.net>,
        Dave Watson <davejwatson@...com>, Paul Turner <pjt@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Russell King <linux@....linux.org.uk>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Andi Kleen <andi@...stfloor.org>,
        Chris Lameter <cl@...ux.com>, Ben Maurer <bmaurer@...com>,
        rostedt <rostedt@...dmis.org>,
        Josh Triplett <josh@...htriplett.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Catalin Marinas <Catalin.Marinas@....com>,
        Will Deacon <Will.Deacon@....com>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Joel Fernandes <joelaf@...gle.com>, shuah <shuah@...nel.org>,
        carlos <carlos@...hat.com>, Florian Weimer <fweimer@...hat.com>,
        Joseph Myers <joseph@...esourcery.com>
Subject: Re: [RFC PATCH for 4.21 01/16] rseq/selftests: Add reference
 counter to coexist with glibc

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

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

Powered by blists - more mailing lists