[PR18457] Don't require rtld lock to compute DTV addr for static TLS

[Alexandre Oliva]

On Jun  3, 2015, Torvald Riegel <triegel@redhat.com> wrote:

> It would be good to have the synchronization for TLS, in particular how
> we implement it and which lock protects accesses to which data, be
> documented somewhere.  I didn't see a pointer to such documentation,
> does it exist?

I very much doubt it.

> If not, and given that you seem to be familiar with it, it would be
> good if you could add it.

Surely not as part of this regression fix, but yeah, that would be nice.
I'll talk to my manager about such a large project, but I'm not very
hopeful, considering I'm back in GCC for the foreseeable future.

>> -  /* Make sure that, if a dlopen running in parallel forces the
>> -     variable into static storage, we'll wait until the address in the
>> -     static TLS block is set up, and use that.  If we're undecided
>> -     yet, make sure we make the decision holding the lock as well.  */
>> -  if (__glibc_unlikely (the_map->l_tls_offset
>> -			!= FORCED_DYNAMIC_TLS_OFFSET))
>> +  /* If the TLS block for the map is already assigned to dynamic or to
>> +     static TLS, avoid the lock.  Be careful to use the same value for
>> +     both tests; if we reloaded it, the second test might mistake
>> +     forced dynamic for an offset.  Now, if the decision hasn't been
>> +     made, take the rtld lock, so that an ongoing dlopen gets a chance
>> +     to complete, and then retest; if the decision is still pending,
>> +     force the module to dynamic TLS.  */
>> +  ptrdiff_t offset = atomic_load_relaxed (&the_map->l_tls_offset);

> This should document why relaxed MO is sufficient

The comments I added attempted to do so.  Can you please try to indicate
what appears to be missing in them?

As for why we need relaxed MO, the first question is why we need atomics
to begin with.  We don't really need anything special there, we just
need to make sure we perform the newly-added test using the same value
used in the preexisting unguarded test.  I was tempted to load the value
into a var and use the asm("":"+X"(var)) idiom, to make sure the var is
not reloaded, but I figured a relaxed atomic load would express that
more nicely.

Now, if you were to ask me for formal proof that relaxed is enough, I
would have to tell you it probably isn't.  Although the memory used for
l_tls_offset in a link map will necessarily be initialized by a thread
holding the load lock, if that memory had been used for anything else
before, another thread might still access a previous value without an
acquire, and if what it finds there happens to be
FORCED_DYNAMIC_TLS_OFFSET, we'd get unexpected behavior.  That said, we
seem to have got along fine with a non-atomic load, so making it a
relaxed atomic load doesn't make it any worse.

This is just one out of many examples of how the current TLS
implementation makes assumptions about the underlying memory model that
are not documented and that are hardly guaranteed by the language or by
the broad variation of target architectures.  It's like we're driving,
blind, at high speed, on a busy freeway, facing the wrong way.  Yet
somehow it at least appears to work, and I guess most of us are too
scared to touch it in any significant way ;-)

I've already mentioned other fragile aspects in the way we deal with
slotinfo, in how DTV growth is AS-Unsafe, in how dynamic TLS Descriptors
are using volatile where atomics would have been a better choice (they
didn't exist back then :-), and the list of suspicious behavior related
with TLS just keeps growing.  I like your goal of getting it documented
and adjusted to current standards, but given our differences in
terminology and background, I very much doubt any such docs coming from
me will meet your expectations.

Perhaps the best way to go about this project is to make it Q&As: you
ask about the properties of something you're trying to document or clean
up, I (or someone else) answer them, then, after any further
clarifications, you turn that into documentation and code changes.
Would you like to do that?


>> __rtld_lock_lock_recursive (GL(dl_load_lock));

> Why do we need the lock at all?

It's right there in the comments, both before and after the change,
though I tried to make the reason more immediately apparent.  Hint:
concurrent dlopen.

> Are we protecting just access to
> l_tls_offset or to other memory locations as well?

We must wait for dlopen to complete.  It might assign the module to
static TLS if some IE relocation requires it.

>> -      if (__glibc_likely (the_map->l_tls_offset == NO_TLS_OFFSET))
>> +      offset = the_map->l_tls_offset;

> l_tls_offset is effectively concurrently accessed data,

> Likewise for the store below.

Yeah, but these are guarded by the lock.

Sure, to be absolutely safe, we want all loads and stores to be atomic
(current code assumes aligned word accesses are relaxed atomic, with
acquire and release implied by the locks), but I'm not inclined to
revisit all that just to fix this regression.  A minimal change is
better for that.


>> +static int running = 1;
>> +  while (running)

> That's a data race with the modification do_end, or isn't it?  Use
> atomics, a semaphore (with trywait), or trylock.

Andreas, any preferences as to how to adjust your (?) testcase to meet
Torvald's standards?

/me takes a mental note to acknowledge the testcase author in the
ChangeLog.

Sorry I didn't do that last night.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer