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

[Torvald Riegel]

On Tue, 2015-06-09 at 00:01 -0300, Alexandre Oliva wrote:
> On Jun  7, 2015, Torvald Riegel <triegel@redhat.com> wrote:
> 
> > OK.  init sets the value unconditionally, which seems either
> > counterproductive, or init is called exactly once and that call happens
> > before any use by another thread.
> 
> The latter, as I wrote.
> 
> > If the latter (and that seems to be
> > the case as you indicate below):
> 
> And so you noticed.
> 
> > (1) the critical section is not required
> 
> Yes, it is.

Not for *this*, specifically.  You produced a reduced analogy / pattern
of the synchronization we're discussing here, so I was referring to that
specifically.

> > (2) we should use an atomic access for clarity of code
> 
> No opposition to that.

Good.

> > OK.  Then this isn't really double-checked locking,
> 
> Well, you used this term for this pattern back when we discussed
> lock-bypassing in stream orientation.  I'm trying hard to overcome my
> different background and use your terminology, but this doesn't make it
> any easier :-(

I don't remember all of that case, but I think we discussed whether it
is double-checked locking and whether it is a correct implementation.
If this other case was similar to this one, and we're just interested in
reaching consensus on one variable's value and there's no logical
relationship to something else or that is taken care of through other
means, then this wouldn't have been typical double-checked locking
either.
Sorry if this didn't come across clearly.  I'm trying to make this as
little confusing as possible.

> > If all one wants is
> > single-memory-word consensus,
> 
> That's not all we want.  We also want to give the dynamic loader
> priority in assigning a module to static TLS, while it's loading a set
> of modules.  HW CAS atomic insns don't give us that AFAIK.

OK.  That would then be a good point to document.  I'm not yet sure I
understand how that is consistent with dlopen being required to happen
before accesses to TLS by other threads.

> Also, we want to make sure we wait till the dynamic loader is done with
> defining the TLS variable before we access it.  It might be the case
> that some module's initializer recursively dlopens additional modules
> (yuck), or start a thread that attempts to access the variable.  We want
> to make those accesses wait for the loader to complete its job before
> they get a chance to make the variable dynamic, that other modules being
> loaded might want to use as static.

OK, also a good point to document.

> 
> > The fact that there is no other data associated with your state machine
> > must be pointed out in the comments.
> 
> There is all the initialization the dynamic loader performs when the
> module that defines the variable is loaded.

I can't follow you here.  You said there are no other dependencies, and
we just want to reach consensus on the final value of l_tls_offset.  So,
from the perspective of just this synchronization state machine you
gave, there's no other data that's relevant.  Now you say there is other
stuff.  What's true?

Are you just trying to point out that there is other initialization but
that other happens-before relationships (e.g., user must synchronize
externally) make this initialization happen-before all non-init()
functions in the state machine?  That would then mean that in this state
machine, the critical section in init() is indeed not required (for this
particular use!).  But above, you said it is.

> > It's often not the case that something is indeed "freestanding", so if
> > it is we want to briefly note why (using a comment in the code).
> 
> I don't think a random participant of an existing synchronization
> pattern is the right place for this sort of comprehensive
> synchronization documentation.

Maybe you misunderstood, so let me rephrase it.  When, such as in this
case, something deviates from what's typical, it's worth pointing this
out.  It deviates from a typical double-checked locking pattern because
you don't have acquire/release pairs.  If you comment in the code that
you need just consensus on the single variable, and there's no other
dependencies, you also clarify it.

> It's a cross-cutting concern, and as we
> (I?) have learned from computational reflection and aspect-oriented
> programming, there's no single right place in the code to document this;
> it's a side document that the relevant pieces should reference.

You can put the code comments somewhere in the code, for example at one
of the functions taking part, or at the decl of the variable, or
somewhere else.  And then reference it.  For example, for the semaphore
or the condvar, I just put the more general overview of the
synchronization scheme on some of the functions, and referenced that
throughout the code of the other functions.  You don't need a separate
document for it.

> >> The enum type is opaque, not visible to callers, and its alignment
> >> and size ensure it will always be loaded and stored atomically.
> 
> > Careful here.  The alignment and size of the type may *allow* a compiler
> > (and our atomic operations) to actually work and make access atomic.
> > But the compiler is *not required* to do so if plain nonatomic accesses
> > are used; it could load/store byte-wise, it could reload, or it could
> > store speculative values.
> 
> *nod*, but not relevant:

No, this is very much relevant.  We do not speculate about compiler
implementations but stick to semantics required by the standards.  If we
need exceptions to that, we document those (e.g., if we rely on a GNU
extension).

I thought we had discussed this sufficiently before, but if you want to
start this topic again, let's do it.

> - load/store of entire words byte-wise would quickly drive the compiler
>   out of existence, or at least out of the marketplace for compilers of
>   system libraries to be used in production

Remember the loads on tile that got mentioned in a previous discussion
we had?

> - reloads would not be a problem for the pattern used in the second
>   version of the patch

How do you know?  You could only argue this way by making assumptions
about other code generation in the compiler, which you haven't done.
And you certainly don't want to document these, which should be a clear
enough indication that you also don't want to reason in detail about
them, nor want anybody reading the code to have to reason about that.

Yeah, one could speculate about what a compiler may or may not do in
this *specific* piece of code.  But that's not the level of abstraction
we want to use as base for our reasoning.

For example, assume the compiler is aware of the code for
"__rtld_lock_lock_recursive (GL(dl_load_lock))" and knows that it won't
access l_tls_offset in some way (which it doesn't).   You access
l_tls_offset inside and out of a critical section, so by
data-race-freedom, there must not be concurrent write accesses.  So it
does not actually have to reload l_tls_offset inside of the critical
section, but use the value of your initial load.  This correct and
reasonable-for-C11 compiler optimization breaks your synchronization.

> - speculative stores that could affect this pattern are not permitted by
>   the relevant standards AFAICT

The standard permits to speculatively store a value, if the target is
not volatile and the speculative store does not create a data race.

For example, the compiler can turn
  spin = NEGATIVE;
into
  spin = POSITIVE; spin = NEGATIVE;

You can *speculate* that this is unlikely to happen in this case, but
that's speculation.  The standard allows such things.  There's no simple
way for you to argue that this won't happen without considering what
does or does not happen elsewhere in the program, what variables
adjacent to spin might be stored to as well, and so on.  IOW, you need
to reason about this and a whole lot of other stuff.

Therefore, to allow for local reasoning, stick to what the standard
guarantees.

> > Unless you use atomic accesses, there's no guarantee that what looks
> > like a single load or store in source code actually ends up as exactly
> > one atomic, full-size load or store in the generated code.
> 
> Alignment, size, and compiler's interest in generating reasonable code
> does.  But we both know you don't agree with that.

No, this is simply not true in general.  You can argue about likelihood
in this *particular* case, but then you're doing just that.

If you think it's unlikely compilers will try to optimize, have a look
at this:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4455.html

> > Thus, if you'd add a comment on the type, you should say that it is
> > compatible (or sufficient for) atomic accesses.
> 
> This is a very pervasive assumption in GNU libc.

It is common, but it sometimes breaks.  For example, the publicly
exposed semaphore type was aligned differently on some archs than the
alignment for the internal representation if we had changed it to use
wider atomics as offered by the arch.

> The only reason I can
> think of to add this to every situation in which it is relevant is to
> make the source code tarballs get higher compression rates.
> 
> > OK.  Note that this was the clearest bit of your patch all way along.
> 
> Well, then, what are we waiting for, considering that this is *all* the
> patch does?

I think I've made it clear what's missing.  There's more to a patch than
the intent behind it.

> > It is a performance improvement.  Using a CAS instead of a critical
> > section would make it even faster for the first accesses.
> 
> But it wouldn't wait for the dynamic loader to complete the loading or
> the relocation.
> 
> > If you want to keep the locks, I suggest mentioning the equivalent CAS
> > anyway because it's conveys the intent more clearly in this case.
> 
> Since Carlos and Siddhesh took this over, I'll leave it for them too.

So you will stop working on this?

If you intend to work on patches affecting synchronization in the
future, please remember the feedback you got in this patch.

> > Just to clarify on previous comments you make: And init() always
> > happens-before make_positive or is_positive?
> 
> Yes.
> 
> > Hmm.  This still seems somewhat inconsistent.  You are arguing that you
> > do not need an acquire load on the consensus implementation above,
> > because l_tls_offset would stand on its own and program logic doesn't
> > need ordering dependencies between this consensus and anything else.
> > Yet you seem to think the release fence is necessary.  Do you think the
> > release fence is necessary for something, and if so, what is it and
> > where is the matching acquire?
> 
> We'd already determined the release fence was needed and taken care of
> by other means.

Huh?

> > Also note that an unlock operation on a lock is *not* guaranteed to have
> > the same effects as an atomic_thread_fence_release.
> 
> *nod*
> 
> > The hardware may or may not treat unlocks (ie,
> > release stores on a *particular* memory location)
> 
> It is my understanding that, per POSIX, unlocks ought to release all
> prior stores made by the thread, and locks must acquire all
> previously-released stores by any thread.  I.e., they don't apply to
> single memory locations.  Locks don't even know what particular memory
> locations they guard, if any.  So I can't make sense of what distinction
> you intend to point out above.

We implement POSIX for users of glibc, but we do not implement on top of
POSIX inside of glibc -- we implement on top of our own code and the C
standard including its memory model.  (And you know that we don't even
implement precisely the POSIX semantics with our POSIX locks.)
In C11, there's a distinction between a release-MO fence and a mutex
unlock operation (i.e., a release-MO store).
Does that answer your question?