[patch] Fix for bz14333 -- race between atexit() and exit()

[Torvald Riegel]

On Wed, 2017-07-19 at 17:37 -0700, Paul Pluzhnikov wrote:
> On Wed, Jul 19, 2017 at 5:26 AM, Torvald Riegel <triegel@redhat.com> wrote:
> 
> > It would be good if you could add comments describing the locking scheme
> > you are changing
> 
> The locking scheme is kind of trivial: hold the lock while reading or
> writing any of the relevant globals.

Even if the scheme itself is not complex in the sense that it's simply
one lock that protects a few pieces of data, figuring out what is
intended can be nontrivial.  It's certainly harder than just reading a
comment, so it makes sense to write this down.

> I've added some comments; please let me know if/where more is desired.

Kind of, I guess.  When you write that __exit_funcs_lock protects
__exit_funcs, do you mean that it also protects the full list that this
global points to?  If so, please say that.

Does that fully remove the need for what looks like an (incorrect)
attempt to build a concurrent list?  I see an atomic_write_barrier in
__on_exit that doesn't seem to have a matching acquire load elsewhere;
there's another atomic_write_barrier that may have been intended to
synchronize with the acquire CAS in __cxa_finalize (but other loads from
that field are plain loads).

So, getting back to the level of detail in comments, what I'm looking
for is text that is sufficient to express the intent behind how
concurreny is handled in this piece of code.  I think for a simple
locking scheme such as this one, having one block of comments around the
central lock is good enough; generally, I think it's nicer to add
comments to the respective data (eg, __exit_funcs) that at least say
something like "See __exit_funcs_lock for concurrency notes.", so that
developers get a heads-up when trying to access the data.  (In this
case, both lock and data are really next to each other, so that might
not add that much.)

Has anyone reviewed this patch in detail yet (including the concurrency
aspect)?  I'm not familiar enough with the exit handler code currently,
so I don't want to dive into this unless (1) nobody else has done it nor
plans to do so and (2) we could still get this into the current release.