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

["Carlos O'Donell" <carlos at redhat dot com>]

On 06/05/2015 03:18 PM, 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, 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?  It's not like our consensus has changed
> since then.  It's not like the problem fixed by this patch is less
> serious than the one fixed before.

Different people review different patches.

It has drawn attention because it is impacting certain use cases
which I prioritize as needing to be fixed.

>>> 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, 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, 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?

For synchronization issues I want to see comments at all the sites
of code that coordinate to provide the synchronization, followed by
a higher level comment explaining how the synchronization happens.

One comment that covers all sites and fails to explain why the
synchronization is correct is insufficient.

>>> So what is this simple regression fix waiting for to get installed,
>>> again?
> 
>> As of today only Fedora is using your previous patches which reveal these
>> older bugs, and until we branch for 2.22, this patch can wait while we
>> look into a more holistic fix.
> 
> And then you're going to have a single patch fixing two or more
> different issues?  Isn't that against our conventions, that prefer
> separate patches to address separate issues?

No. We are going to fix one issue per commit.

Cheers,
Carlos.