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

[Torvald Riegel <triegel at redhat dot com>]

On Fri, 2015-06-05 at 16:18 -0300, Alexandre Oliva wrote:
> On Jun  5, 2015, "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
> > The solution does not meet my standards for fixing atomicity issues.
> > It lacks proper documentation on the synchronization changes being made.
> 
> Could it be because there aren't any synchronization changes being made?
> Seriously, the changes, if any, were made in the previous patch,

I'm not aware of another one with synchronization bits in it.  I
probably simply didn't notice it.

> that
> didn't meet such a storm of stonewalling and nitpicking.  This one just
> fixes a tiny part of the change in the previous patch.
> 
> Why is it drawing so much attention?  Why are so different standards
> being applied to this one?

As a counter-example, I reviewed Szabolcs' Lazy TLSDESC relocation data
race fix at the same level of detail, and requested comments at the same
level of detail as for your patch.

If you feel that there are other patches that haven't been reviewed
closely enough, please mention them specifically so we can see whether
we need to improve the affected code.

> >> But if this larger fix does not contain the very change I'm proposing,
> >> tls_get_addr on variables assigned static TLS will remain much slower
> >> than needed, because there's absolutely no need to take a lock when it
> >> is already decided whether the module should use static or dynamic TLS.
> 
> > I agree there is no need to take the lock, but your patch needs to be
> > expanded in more detail to make it crystal clear exactly what
> > synchronization is being done and why.
> 
>   /* If the TLS block for the map is already assigned to dynamic, or
>      to some static TLS offset, the decision is final, and no lock is
>      required.  Now, if the decision hasn't been made, take the rtld
>      lock, so that an ongoing dlopen gets a chance to complete,
>      possibly assigning the module to static TLS and initializing the
>      corresponding TLS area for all threads,

That sounds like it is not just about reaching consensus on the final
value of l_tls_offset:  For example, you mention initialization in
there, which seems to indicate that there is some dependency on
happening after initialization (ie, wait for dlopen to complete
initialization); but elsewhere in the thread you say there are no such
constraints and it's really just consensus on just this value.

Also, it sounds as if any thread concurrent with the dlopen could only
actually create a concurrent access if dlopen has the lock already --
but that doesn't seem to be a requirement based on the "spin"
example/analogy you gave.

> and then retest; if the
>      decision is still pending, force the module to dynamic TLS.
> 
>      The risk that the thread accesses an earlier value in that memory
>      location, from before it was recycled into a link map in another
>      thread,

I wouldn't point out "earlier" here, but rather specifically address
which stores (eg, initialization) the code needs a happens-before for.

> is removed by the need for some happens before
>      relationship between the loader that set that up and the TLS
>      access that referenced the module id.  In the presence of such a
>      relationship, the value will be at least as recent as the
>      initialization, and in its absence, calling tls_get_addr with its
>      module id invokes undefined behavior.  */
> 
> Seriously, what's missing from this?  What's not crystal clear about it?

See above.  The lack of atomic accesses in the code also made this seem
inconsistent.