The 05/10/2021 22:56, Carlos O'Donell wrote:
> On 4/16/21 5:12 AM, Szabolcs Nagy via Libc-alpha wrote:
> > The 04/15/2021 15:21, Adhemerval Zanella wrote:
> >> On 13/04/2021 05:19, Szabolcs Nagy via Libc-alpha wrote:
> >>> This is a follow up patch to the fix for bug 19329.  This adds
> >>> relaxed MO atomics to accesses that are racy, but relaxed MO is
> >>> enough.
> >>
> >> Could you extend a bit why relaxed MO should be suffice?
> > 
> > is it ok to change the commit message to:
> > 
> > This is a follow up patch to the fix for bug 19329.  This adds relaxed
> > MO atomics to accesses that are racy, but relaxed MO is enough.
> 
> Suggest:
> 
> This is a follow up patch to the fix for bug 19329.  This adds relaxed
> MO atomics to accesses that were previously data races but are now
> race conditions, and where relaxed MO is sufficient.
>  
> > The racy accesses all follow the pattern that the write is behind the
> 
> s/racy accesses/race conditions/g
> 
> > dlopen lock, but a read can happen concurrently (e.g. during tls access)
> > without holding the lock.  For slotinfo entries the read value only
> > matters if it reads from a synchronized write in dlopen or dlclose,
> > otherwise the related dtv entry is not valid to access so it is fine to
> > leave it in inconsistent state. Same for GL(dl_tls_max_dtv_idx) and
> 
> s/it in/it in an/g
> 
> s/Same/The same applies for/g
> 
> > GL(dl_tls_generation), but there we rely on the read value being larger
> > than the last written value that was synchronized.
> 
> Do you mean to imply that the synchronized writes all increase the generation
> counter, and so any out of order reads rely on the value to be increasing?

yes, if the current thread is synchronized with a dlopen and reads
GL(dl_tls_genertion) with relaxed MO then either the gen of the
dlopened module is read or a larger value. So we can use relaxed MO
value to see if the dtv needs update at tls access.

(This is the difficult bit in fixing bug 19924: we can use relaxed
MO because we update the dtv upto the gen of the module. slotinfo
entries with larger gen are ignored, but if we want to update upto
the global gen then we need additional synchronization.)

> Suggested:
> The same applies for GL(dl_tls_max_dtv_idx) and GL(dl_tls_generation), but
> there the algorithm relies on the fact that the read of the last synchronized
> write is an increasing value.

Thanks for the review, i attached the fixed patch i plan to commit
later today if there are no further comments.