Re: [patch] libitm: Fix race condition in dispatch choice at transaction begin.

[Richard Henderson <rth at redhat dot com>]

[ bkoz, see <bkoz> below ]

On 02/14/2012 04:48 AM, Torvald Riegel wrote:
> BTW, why can we assume that the dispatch objects are initialized before
> we run any transactions (e.g., from within a constructor of another
> static object)?  Or is this the library initialization problem that we
> still have to deal with at some point?

ELF shared libraries are initialized in dependency order.  Static libraries
simply must be careful to be linked in the proper order on the command line.

It should be fine unless the user really tries to do something wrong.

>     	libitm/
>     	* beginend.cc (GTM::gtm_thread::begin_transaction): Move serial lock
>     	acquisition to ...
>     	* retry.cc (GTM::gtm_thread::decide_begin_dispatch): ... here.
>     	(default_dispatch): Make atomic.
>     	(GTM::gtm_thread::set_default_dispatch): Access atomically.
>     	(GTM::gtm_thread::decide_retry_strategy): Access atomically and
>     	use decide_begin_dispatch() if default_dispatch might have changed.
>     	(GTM::gtm_thread::number_of_threads_changed): Initialize
>     	default_dispatch here.

I discussed this with Torvald at length on IRC.  For the record, I think the
change to use atomic<> obscures the actual bug fix, that of rearranging
decide_begin_dispatch.

The change to use atomic<> is at best a theoretical bug fix, for some 
embedded target that doesn't load/store pointers atomically by default.
Or for consumption by some static race detector.

I'd have been happier with the one interesting reference to default_dispatch
being changed to use __atomic_load.  Rather than having special markup for
all of the non-interesting references, and spending cycles figuring out
whether the use of memory_order_relaxed is really correct at each instance.

All that said the patch is ok, Torvald.

<bkoz>
I wonder if the atomic<> template wouldn't have been better with another
(defaulted) template argument to set the default memory access mode for the
object.  In this case we could have used

  static std::atomic<GTM::abi_dispatch*, memory_order_relaxed> default_dispatch;

and then not have had to worry about ugly-ing up the source with .load/.store
accessors to change all of the accesses back to relaxed.

Is a change like this something that might get us in trouble somehow with the
c++ standard, or cause us ABI problems vs 4.7 if we changed this in 4.8?  Or
is the ABI thing a non-issue since we expose the entire template to the user?
</bkoz>



r~