Re: [PATCH v3] elf: Fix slow tls access after dlopen [BZ #19924]

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 08/23/2023 16:06, Adhemerval Zanella Netto wrote:
> The fix sound reasonable, and I did not see any regression on some different
> architectures (aarch64, powerpc, sparc, x86).  However I am not sure how
> compreensible our own regression tests are for this issue, since afaik
> there is no concurrent tests that stress concurrent dlopen module and
> TLS access.  It also does fix the performance issue from bug report.
> 
> Below some minor comments.
...
> > --- a/elf/dl-reloc.c
> > +++ b/elf/dl-reloc.c
> > @@ -114,9 +114,7 @@ _dl_try_allocate_static_tls (struct link_map *map, bool optional)
> >  #ifdef SHARED
> >        if (__builtin_expect (THREAD_DTV()[0].counter != GL(dl_tls_generation),
> >  			    0))
> > -	/* Update the slot information data for at least the generation of
> > -	   the DSO we are allocating data for.  */
> > -	(void) _dl_update_slotinfo (map->l_tls_modid);
> > +	(void) _dl_update_slotinfo (map->l_tls_modid, GL(dl_tls_generation));
> 
> Shouldn't it use atomic_load_relaxed on GL(dl_tls_generation) for consistency?

_dl_try_allocate_static_tls is called during tlsdesc reloc
processing.

relocation processing always runs with the GL(dl_load_lock)
held and then no concurrent write to the gen count is possible.

loads outside the lock have to use atomics to sync with the
atomic store under the lock, loads under the lock can be
normal loads.

> >  struct link_map *
> > -_dl_update_slotinfo (unsigned long int req_modid)
> > +_dl_update_slotinfo (unsigned long int req_modid, size_t new_gen)
> >  {
> >    struct link_map *the_map = NULL;
> >    dtv_t *dtv = THREAD_DTV ();
> >  
> > -  /* The global dl_tls_dtv_slotinfo array contains for each module
> > -     index the generation counter current when the entry was created.
> > +  /* CONCURRENCY NOTES:
> > +
> > +     The global dl_tls_dtv_slotinfo array contains for each module
> 
> Maybe use the correct name, dl_tls_dtv_slotinfo_list, so it is easier
> to reference on the code.

ok. (it was this way in the orig comment, but using the full
identifier is probably better)

> 
> > +     index the generation counter current when that entry was updated.
> >       This array never shrinks so that all module indices which were
> 
> I think it should 'that' here (it specifies the module indices and there
> is no comma).

what do you mean?

> 
> > -     valid at some time can be used to access it.  Before the first
> > -     use of a new module index in this function the array was extended
> > -     appropriately.  Access also does not have to be guarded against
> > -     modifications of the array.  It is assumed that pointer-size
> > -     values can be read atomically even in SMP environments.  It is
> > -     possible that other threads at the same time dynamically load
> > -     code and therefore add to the slotinfo list.  This is a problem
> > -     since we must not pick up any information about incomplete work.
> > -     The solution to this is to ignore all dtv slots which were
> > -     created after the one we are currently interested.  We know that
> > -     dynamic loading for this module is completed and this is the last
> > -     load operation we know finished.  */
> > -  unsigned long int idx = req_modid;
> > +     valid at some time can be used to access it.  Concurrent loading
> > +     and unloading of modules can update slotinfo entries or extend
> > +     the array.  The updates happen under the GL(dl_load_tls_lock) and
> > +     finish with the release store of the generation counter to
> > +     GL(dl_tls_generation) which is synchronized with the load of
> > +     new_gen in the caller.  So updates up to new_gen are synchronized
> > +     but updates for later generations may not be.
> > +
> > +     Here we update the thread dtv from old_gen (== dtv[0].counter) to
> > +     new_gen generation.  For this each dtv[i] entry is either set to
> 
> Maybe a comma after 'For this'.

ok

> > +     an unallocated state (set), or left unmodified (nop).  Where (set)
> > +     may resize the dtv first if modid i >= dtv[-1].counter. The rules
> > +     for the decision between (set) and (nop) are
> > +
> > +     (1) If slotinfo entry i is concurrently updated then either (set)
> > +         or (nop) is valid: TLS access cannot use dtv[i] unless it is
> > +         synchronized with a generation > new_gen.
> > +
> > +     Otherwise if the generation of slotinfo entry i is gen and the
> 
> Maybe a comma after 'Otherwise'.

ok