Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use initial-exec TLS when building seastar as shared library #2255

Merged
merged 1 commit into from
May 20, 2024

Conversation

tchaikov
Copy link
Contributor

quote from
https://patchwork.ozlabs.org/project/glibc/patch/8734v1ieke.fsf@oldenburg.str.redhat.com/

On the glibc side, we should recommend that intercepting mallocs and its
dependencies use initial-exec TLS because that kind of TLS does not use
malloc. If intercepting mallocs using dynamic TLS work at all, that's
totally by accident, and was in the past helped by glibc bug 19924.

so instead of allocating TLS variables using malloc, let's allocate them using initial-exec TLS model. another approach is to single out the static TLS variables in the code path of malloc/free and apply __attribute__ ((tls_model("initial-exec"))) to them, and optionally only do this when we are building shared library.

but this could be overkill as

  1. we build static library in the release build
  2. the total size of the static TLS variables is presumably small, so the application linking against the seastar shared library should be able to afford this.

see also
https://patchwork.ozlabs.org/project/glibc/patch/8734v1ieke.fsf@oldenburg.str.redhat.com/ and
https://sourceware.org/bugzilla/show_bug.cgi?id=19924

Fixes #2247
Signed-off-by: Kefu Chai kefu.chai@scylladb.com

quote from
https://patchwork.ozlabs.org/project/glibc/patch/8734v1ieke.fsf@oldenburg.str.redhat.com/

> On the glibc side, we should recommend that intercepting mallocs and its
> dependencies use initial-exec TLS because that kind of TLS does not use
> malloc.  If intercepting mallocs using dynamic TLS work at all, that's
> totally by accident, and was in the past helped by glibc bug 19924.

so instead of allocating TLS variables using malloc, let's allocate them
using initial-exec TLS model. another approach is to single out the
static TLS variables in the code path of malloc/free and apply
`__attribute__ ((tls_model("initial-exec")))` to them, and optionally
only do this when we are building shared library.

but this could be overkill as

1. we build static library in the release build
2. the total size of the static TLS variables is presumably small, so
   the application linking against the seastar shared library
   should be able to afford this.

see also
https://patchwork.ozlabs.org/project/glibc/patch/8734v1ieke.fsf@oldenburg.str.redhat.com/
and
https://sourceware.org/bugzilla/show_bug.cgi?id=19924

Fixes scylladb#2247
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@avikivity
Copy link
Member

What's the downside of initial-exec? I guess it can't be used with dlopen(), which seems minor here.

@avikivity
Copy link
Member

Seems reasonable to me. But please verify it works with scylladb.

@tchaikov tchaikov marked this pull request as draft May 20, 2024 10:33
@tchaikov tchaikov marked this pull request as ready for review May 20, 2024 12:42
@tchaikov
Copy link
Contributor Author

tchaikov commented May 20, 2024

@avikivity yes, as you put. an application would not be able to dlopen libseastar.so for accessing its TLS symbols. but i also learned that glibc's ld.so does reserve some space in static TLS block, and allows dlopen on such a shared object if its TLS size is small. but this does not apply to musl's dynamic loader. anyway, i concur with you this issue is minor.

tested with following steps

$ cd seastar
$ git cherry-pick 6c44eed
$ git show HEAD --name-only --oneline 
2ee99a7b2 (HEAD) build: use initial-exec TLS when building seastar as shared library
CMakeLists.txt
$ cd ..
$ ./configure.py --mode dev --compiler clang++ --c-compiler clang
$ ninja build/dev/scylla
$ grep CMAKE_BUILD_TYPE build/dev/seastar/CMakeCache.txt 
CMAKE_BUILD_TYPE:STRING=Dev
$ cd ~/dev/scylla-dtest
$ JAVA_HOME=/usr/lib/jvm/jre-11 pytest -s --log-cli-level=debug -c $PWD/pytest.ini --cassandra-dir $HOME/dev/scylladb/build/dev  repair_additional_test.py::TestRepairAdditional::test_repair_one_node_alter_rf
...
==================================================== 1 passed, 2 warnings in 59.91s ====================================================

@avikivity avikivity merged commit d8a70b3 into scylladb:master May 20, 2024
12 checks passed
@tchaikov tchaikov deleted the tls-shared-object branch May 20, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

seastar allocator causes infinite recursive call if seastar is compiled as a shared library
2 participants