Fix slow tls access after dlopen

Fix slow tls access after dlopen

[Paul Zimmermann]

       Hi Szabolcs,

please could you repost [1] as a distinct patch as requested by Carlos?

This issue affects the SageMath software tool, with a major slowdown:

https://trac.sagemath.org/ticket/34850

(see comment 97 which provides a short C program to reproduce the issue).

Best regards,
Paul

[1] https://patchwork.ozlabs.org/project/glibc/patch/b116855de71098ef7dd2875dd3237f8f3ecc12c2.1618301209.git.szabolcs.nagy@arm.com/

Re: Fix slow tls access after dlopen

[Szabolcs Nagy]

The 01/06/2023 11:06, Paul Zimmermann via Libc-alpha wrote:
>        Hi Szabolcs,
> 
> please could you repost [1] as a distinct patch as requested by Carlos?
> 
> This issue affects the SageMath software tool, with a major slowdown:
> 
> https://trac.sagemath.org/ticket/34850
> 
> (see comment 97 which provides a short C program to reproduce the issue).

i see, thanks.

> 
> Best regards,
> Paul
> 
> [1] https://patchwork.ozlabs.org/project/glibc/patch/b116855de71098ef7dd2875dd3237f8f3ecc12c2.1618301209.git.szabolcs.nagy@arm.com/

yeah i resent that patch (only updated the commit message):
https://patchwork.ozlabs.org/project/glibc/patch/20221019111439.63501-1-szabolcs.nagy@arm.com/

but it requires more work (to document the new sync mechanism).
and requires actual review. and i think there is a better way but
there are non-trivial trade-offs so reviewing the design would help:

in tls access slow path we currently update the thread dtv up to the
generation of the accessed module, but always do the slow path if the
dtv is older than the global generation. the plan is to update the dtv
to the global generation (latest dlopen/dlclose), this means we don't
keep hitting the slow path after a dlopen, but we do hit it once after
every dlopen/dlclose (even though an independent library is loaded,
not related to the current tls access, this is ok if dlopen/dlclose of
libs with tls is rare).

dtv update does two things: 1. resize the dtv array (of the current
thread) upto max modid + some, 2. go through the global dtv slotinfo
list and if the generation of entry i is newer than the dtv generation
then free/reset dtv[i]. (note that dtv[i] is not allocated here, it's
either reset or left alone.)

the original patch in bugzilla and my patch tries to do the dtv update
with lock-free atomics which means the global generation count has to
be accessed with acquire-load and release-store and a lot of racy
loads need to use relaxed mo atomics to avoid data races. this works
except out-of-thin-air values may break the code. OOTA value is only a
theoretical issue with relaxed mo access i think, here it can cause ub
down the line and that ub can cause arbitrary value to be stored
concurrently justifying the OOTA value in the first place, this is a
bit ugly and the use of atomics makes the code harder to read, but
otherwise i don't see a problem with this approach.

another approach is to take GL(dl_load_tls_lock) in tls access slow
path and then the sync is much simpler (no nasty relaxed atomics).
the drawback is that malloc/free is used under the lock which may
be interposed such that they use GD tls which makes lock ordering
deadlock more likely (note such issue already exists in dlopen and
dlclose and tls access already uses the same lock and malloc/free
in tls_get_addr_tail, however now the malloc/free would be under
the lock so deadlock can happen between two threads only doing tls
access, no dlopen/dlclose is needed). another problem is lock
contention when there are many tls accesses concurrently with a
dlopen/dlclose that bumps the generation count so all those accesses
have to wait for each other and dlopen/dlclose for the same lock.

so using a lock is not ideal but if we do bigger changes it may work
or we may get away with simpler code:

- ideally an independent dlopen should not slow tls access down.
  i think this can be done by updating dtv[i] in all threads at
  dlclose time and leaving dtv resize for the tls access slow path.
  (moving dtv resize to dlopen/dlclose too might work but the sync
  with tls access is non-trivial then), then generation count is not
  needed at all. dtv[i] is valid if dtv array is large enough. first
  access still needs to allocate. dlclose will be slower and has to
  sync with thread creation and exit and with dtv resize at tls
  access. this is a reasonable trade-off i think. requires dtv resize
  change to avoid the malloc lock ordering problem (not use realloc
  or use a per-thead-lock to sync with dlclose) and target specific
  asm change (at least for tlsdesc) and removal of all the slotinfo
  generation count code.

- tls_get_addr_tail should not use the tls lock: dlopen can just
  set up map->l_tls_offset and have it const during the lifetime of
  the module. (i'm not sure why tls offset is set up lazily i dont
  think it buys us anything but i might missed something around dl
  namespaces).

- _dl_update_slotinfo is called outside of tls access unnecessarily
  (in dlopen and reloc processing) i think these should be removed.
  what might make sense instead is to update the dtv once in rtld when
  the process is still single threaded and all modules are relocated
  but ctors are not yet run (this can ensure that libs loaded at start
  time to have reliable and as-safe GD tls access: they will use
  static tls without any locks or malloc on first access).

- completely avoiding locks and malloc in GD tls access would require
  upfront allocation of tls for (and sync with) all threads, this is
  likely too much memory use (and dlopen sync overhead), but even
  with lazy tls allocation e.g. we can guarantee as-safe, no-malloc,
  no-failure GD tls access when tls was already accessed in that
  thread (so independent dlopen cannot force a dtv update only the
  first access may require lock/malloc). not sure how useful this is
  but then we can make dlsym report allocation failure for tls syms
  so users can do an initial tls access in relevant threads to ensure
  fast and reliable behaviour later (even in signal handlers).

if we want a fix for this release i can repost my previous patch (with
lock-free atomics that is likely backportable) but at some point a
larger rewrite would be better i think.