Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1503467992.2999.1582234410317.JavaMail.zimbra@efficios.com>
 <20200221154923.GC194360@google.com> <1683022606.3452.1582301632640.JavaMail.zimbra@efficios.com>
 <CAEXW_YRT7AjaJs7mPyNd=J6fhBicYwGbQMK2Senwm3cBhFvWPw@mail.gmail.com>
In-Reply-To: <CAEXW_YRT7AjaJs7mPyNd=J6fhBicYwGbQMK2Senwm3cBhFvWPw@mail.gmail.com>
From:   Chris Kennelly <ckennelly@google.com>
Date:   Tue, 25 Feb 2020 22:38:37 -0500
Message-ID: <CAEE+ybmQb02u-=c1sHozkJ+RXOi2Hno6qYJ0Vx9rOpKjSQ4fPQ@mail.gmail.com>
Subject: Re: Rseq registration: Google tcmalloc vs glibc
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Paul Turner <pjt@google.com>,
        Florian Weimer <fweimer@redhat.com>,
        "Carlos O'Donell" <codonell@redhat.com>,
        libc-alpha <libc-alpha@sourceware.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        paulmck <paulmck@kernel.org>, Boqun Feng <boqun.feng@gmail.com>,
        Brian Geffon <bgeffon@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 25, 2020 at 10:25 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Fri, Feb 21, 2020 at 11:13 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> >
> > ----- On Feb 21, 2020, at 10:49 AM, Joel Fernandes, Google joel@joelfernandes.org wrote:
> >
> > [...]
> > >>
> > >> 3) Use the  __rseq_abi TLS cpu_id field to know whether Rseq has been
> > >> registered.
> > >>
> > >> - Current protocol in the most recent glibc integration patch set.
> > >> - Not supported yet by Linux kernel rseq selftests,
> > >> - Not supported yet by tcmalloc,
> > >>
> > >> Use the per-thread state to figure out whether each thread need to register
> > >> Rseq individually.
> > >>
> > >> Works for integration between a library which exists for the entire lifetime
> > >> of the executable (e.g. glibc) and other libraries. However, it does not
> > >> allow a set of libraries which are dlopen'd/dlclose'd to co-exist without
> > >> having a library like glibc handling the registration present.
> > >
> > > Mathieu, could you share more details about why during dlopen/close
> > > libraries we cannot use the same __rseq_abi TLS to detect that rseq was
> > > registered?
> >
> > Sure,
> >
> > A library which is only loaded and never closed during the execution of the
> > program can let the kernel implicitly unregister rseq at thread exit. For
> > the dlopen/dlclose use-case, we need to be able to explicitly unregister
> > each thread's __rseq_abi which sit in a library which is going to be
> > dlclose'd.
>
> Mathieu, Thanks a lot for the explanation, it makes complete sense. It
> sounds from Chris's reply that tcmalloc already checks
> __rseq_abi.cpu_id and is not dlopened/closed. Considering these, it
> seems to already handle things properly - CMIIW.

I'll make a note about this, since we can probably benefit from some
more comments about the assumptions/invariants the fastpath uses.

> Chris, Brian, is there any other concern to upgrading of tcmalloc
> version in ChromeOS? I believe there was some concern about detection
> of rseq kernel support. A quick look at tcmalloc shows it does not do
> such detection, but I can stand corrected.

The build time configuration is because we need to have an assembly
implementation of the restartable sequence (example: x86_64
[https://github.com/google/tcmalloc/blob/master/tcmalloc/internal/percpu_rseq_x86_64.S]),
although I've been moving these to inline assembly recently.

If we have build time support for the code path, we'll try to invoke
the rseq() call and see if registration succeeds:
https://github.com/google/tcmalloc/blob/master/tcmalloc/internal/percpu.cc#L85-L107

> One more thing, currently
> tcmalloc does not use rseq on ARM. If I recall, ARM does have rseq
> support as well. So we ought to enable it for that arch as well if
> possible. Why not enable it on all arches and then dynamically detect
> at runtime if needed support is available?

As far as ARM support goes for TCMalloc's per-CPU/rseq path, I haven't
had the bandwidth to write an assembly implementation (although I'd
welcome one).

Thanks,
Chris Kennelly