Re: libitm: fixes and optimizations in gl_wt

[Richard Henderson <rth at redhat dot com>]

On 02/20/12 08:54, Torvald Riegel wrote:
> The following patches don't depend on each other but are all for the
> gl_wt TM method.
> 
> patch1:
> For memory transfers from source regions with the read-for-write
> modifier, undo logging of these regions was missing.  Also optimize the
> number of accesses to gtm_thr().

Out of curiosity, does an optional argument as in

  static void pre_write(const void *addr, size_t len, gtm_thread *tx = gtm_thr())

work just as well as

+  static void pre_write(const void *addr, size_t len)
+  {
+    gtm_thread *tx = gtm_thr();
+    pre_write(addr, len, tx);
+  }

> +	if (dst_mod == WaW || dst_mod == NONTXNAL)
> +	  tx = gtm_thr();

This sort of micro-optimization can't be good.  If we are, for some reason,
not unifying the two loads, it seems to me that it'd be better to simply
unconditionally load the thread data here:

> +    gtm_thread *tx = 0;

The actual bug fix, calling pre_write for RfW, is of course ok.

> patch2:
> Similar to the ml_wt code, we don't need to use acq_rel but just acquire
> memory order when acquiring the orec.  The following release fence is
> still necessary.

Ok.


> 
> patch3:
> Recently, I changed how gtm_rwlock handles upgrades to serial mode.
> This change was to fix privatization safety and basically consists of
> not modifying gtm_thread::shared_state until trycommit() and/or
> rollback() have finished after the upgrade (and before the switch to
> serialirr dispatch).  With this in place, we can avoid the previous
> workarounds for the commit-in-serial-mode corner case.

Ok.


> 
> patch4:
> Privatization safety in gl_wt rollback was previously enforced with
> something that probably worked but for which the precise C++11 mem model
> reasoning was unclear.  The patch fixes that (so it's clear why it works
> on the basis of the language's memory model, not on the HW models), and
> it less costly than the previously used seq_cst barrier.

Ok.

> 
> patch5:
> Just put gl_wt's global orec on a separate cacheline, same as we do in
> ml_wt.

> +  atomic<gtm_word> orec __attribute__((aligned(HW_CACHELINE_SIZE)));
> +  char tailpadding[HW_CACHELINE_SIZE - sizeof(atomic<gtm_word>)];

The "tailpadding" structure should not be required.  Since an element of
the structure is aligned, the whole class will be aligned.  And the 
compiler automatically pads aligned structures so that arrays of them
work, and satisfy the alignment.

Or is the orec overlapping with the vtable or something?


r~