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 01:16 -0300, Alexandre Oliva wrote:
> On Jun  4, 2015, Torvald Riegel <triegel@redhat.com> wrote:
> 
> > On Wed, 2015-06-03 at 17:31 -0300, Alexandre Oliva wrote:
> >> On Jun  3, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
> >> 
> >> > How's this?
> >> 
> >> Or rather this, that I got after updating the patch file :-)
> 
> > Why did you remove the atomic accesses altogether?
> 
> Because I rearranged the code so that the double-checked lock pattern is
> self-evident.  I thought we had agreed long ago that we didn't need
> atomics for double-checked locks.

No, not generally.  See my other email.

> You added an "*Correct*" in this
> thread, so now I guess you have to show why the proposed change is not
> correct.  Please explain?

See my other email.  The example in there has atomic accesses for the
flag, your patch doesn't have them for l_tls_offset, which takes the
role of the flag.

> > You made good progress towards a consistent fix with the reasoning on
> > synchronization you provided for the static case
> 
> It was not for the static case only.  It covered both cases.
> 
> > This doesn't add any of the documentation I want to see either.
> 
> The deal I suggested was that I'd answer your questions and you'd write
> the documentation.  Now you're moving the goalpost.

No, I was speaking about the documentation for the code you touched --
not for everything.  If you make a change involving synchronization, you
have to provide enough documentation to argue why it works.  If it is an
incremental change, this doesn't mean you have to bring all the
documentation transitively required, but you have to add docs for the
immediate neighborhood of your change.

> Anyway, my manager
> told me not to spend time on this Q&A project I had suggested, so you'll
> have to find some other way to get the documentation you want.
> Stonewalling a trivial patch on the grounds that it fails to document
> something that was not documented to begin with it not reasonable; it
> won't even get you the documentation, it will just cause the regression
> to be around longer.

If your patch were trivial and correct, you could trivially explain why
it is correct.  You haven't done that yet.  You applied double-checked
locking incorrectly, and in other cases you're arguing that there's
actually no concurrency, which conflicts with that you want the
double-checked locking in the first place.  You have data races in your
code, or you haven't properly explained why there are none.

If we apply fixes, they need to be correct -- even if they don't solve
all the issues.  I don't yet see why your patch results in correct code
(even when just considering the functions you touched), and I have
plenty of experience reasoning about concurrent code.  Carlos and
Siddhesh seem to not be convinced either.  Isn't this enough indication
that maybe something's wrong with your patch, or you need to add more
comments to the code?

Also, even if we consider the potential case that you'd be much better
at concurrency than all of us, you'd still need to add sufficient
documentation for all of us, because it matters that the project (and
the people involved) understand the code, not that just you understand
the code.
For example, personally, I don't need all the code comments in
pthread_once that I added -- I do know this synchronization pattern.
However, it may not be obvious to others that don't look at concurrent
code regularly, in the same way that I'd be pretty lost in libm.
Therefore, we add comments so we can raise the overall level of
accessibility and understanding.  Period.