Re: [Gc] Gc] test_stack on powerpc (power7)

On Mon, 2016-08-15 at 19:07 +0300, Ivan Maidanski wrote:
> 
> Thank you, Hans.
> 
> Here's the commit to try:
> https://github.com/ivmai/libatomic_ops/commit/85dd735949d712fa5db4bde0f0fc74f15a624222

Sorry I've not been able to apply much help,.. different priorities for
me now.. :-) 


I re-tested with libatomic_ops HEAD this morning, which is 
at 90108880a324be588ae61ec12d844f4f7db1968e
and includes 85dd735949d712fa5db4bde0f0fc74f15a624222

This still fails on power. (power7 and power8).

Can you/someone verify what the AO_load() function should be mapping to?
Per a comment I made in that early email (Jan 2014), I had found that
the AO_load() function was (and still is) being mapped to AO_load() in
sysdeps/loadstore/atomic_load.h, versus the AO_load_acquire() in
powerpc.h.  ... which led me to wonder if there is a logic issue in the
#if defined chains in generalize-small.h 

Thanks, 
-Will


> Regards,
> Ivan
> 
> --
> 
> 
> Sat, 13 Aug 2016, 23:55 +03:00 from Hans Boehm <[email protected]>:
> 
> I finally had a chance to look at this.  Sorry about the
> llloooonnnnngggggg delay.
> This is really a combination of imprecision in the atomic_ops
> definitions, my lack of understanding of PowerPC semantics at the
> time, and the resulting sloppiness in the stack implementation.
> I think that the core issue is that AO_stack_pop_explicit_aux_acquire
> really needs to ensure that the store to the black list
> via AO_compare_and_swap_acquire becomes visible before the load to
> check the list head on line 155.  This effectively needs store->load
> ordering. Currently the only ordering here is imposed by the _acquire
> on the compare_and_swap. On PowerPC that turns into an lwsync, which
> is too weak to enforce store to load ordering.
> Given our current definitions, it's probably best to drop the _acquire
> from the CAS on line 136, and add a AO_nop_full just before line 154.
> This is suboptimal on x86, and we may want to make the fence
> conditional on "not x86", where the CAS already includes sufficient
> ordering.  (With C++11 atomics, this would also be tricky and probably
> involve making a bunch of accesses seq_cst.)
> It would be good if someone with PowerPC access could confirm that
> this fixes the problem.
> Hans
> On Wed, Aug 3, 2016 at 1:16 PM, Ivan Maidanski <ivmai-JGs/[email protected]> wrote:
> >Hello Pavel,
> >> I've seen Fedora maintainer applied this workaround downstream. My
> question is whether it is better to have it applied, or rather not.
> >No. I checked gcc (4.8 ppc64le) __atomic_load_n - it is the same as
> current AO_load implementation. This means even if we apply this hack
> and later switch to gcc atomics then the issue comes again.
> >But I'm OK with more local change - replace one or several AO_load
> operations with AO_load_acquire in atomic_ops_stack.c for power7 -
> this is, of course, still a hack as we don't know the root cause of
> the failure.
> >1. The attached patch should work but please check it.
> >2. Is it possible to make the patch with a smaller number of changes?
> >> I need to understand more deeply the "stack_aux" structure and how
> you are using it, I still can't see whether there is guaranteed that
> the background algorithm may not have collisions (considering 2+
> threads trying to make a pop and 2+ threads trying to make a push
> operation in parallel).
> >I want Hans to look at it deeper. (It seems he's busy at this
> moment.)
> >PS. Link to the issue on Github:
> https://github.com/ivmai/libatomic_ops/issues/15
> >Regards,
> >Ivan
> >> 
> Wed, 27 Jul 2016, 21:26 +03:00 from Ivan Maidanski <ivmai-JGs/[email protected]>:
> >> 
> Hello Pavel and Hans,
> >>To Pavel:
> >>> I've seen Fedora maintainer applied this workaround downstream.
> >>Please give a reference? (sorry I failed to access the patch in ML)
> >>To Hans:
> >>What do you think of the failure reason? Could you please better
> description of stack_aux?
> >>On Mon, 2012-12-17 at 15:34 +0100, Pavel Raiskup wrote:
> >>> ... 
> >>> I need to understand more deeply the "stack_aux" structure and how
> you are using it, I still can't see whether there is guaranteed that
> the background algorithm may not have collisions (considering 2+
> threads trying to make a pop and 2+ threads trying to make a push
> operation in parallel).
> >>Regards,
> >>Ivan
> >>> 
> Wed, 27 Jul 2016, 17:40 +03:00 from Pavel Raiskup
> <[email protected]>:
> >>>On Sunday, February 16, 2014 7:20:13 PM CEST Ivan Maidanski wrote:
> >>> 
> >  Hi Will,
> >>> 
> > 
> >>> 
> > I don't think current definition of AO_load is incorrect. (The
> definition
> >>> 
> > has never been changed and all other supported platforms have the
> same
> >>> 
> > no-barrier atomic load definition.)
> >>> 
> > 
> >>> 
> > Probably the barrier should be added to stack implementation.
> >>> 
> > Is it really correct to have "first = AO_load(list)" (in
> AO_stack_pop_explicit_aux_acquire) instead of "first =
> AO_load_acquire(list)" ?
> >>> 
> I've seen Fedora maintainer applied this workaround downstream.  My
> question is
> >>> 
> whether it is better to have it applied, or rather not.
> >>> 
> Note that there were some attempts to make this work on ppc64 before:
> >>>http://comments.gmane.org/gmane.comp.programming.garbage-collection.boehmgc/5470
> >>> 
> That thread is however scattered across several mailing list archives
> -> I've
> >>> 
> posted one patch for which I would love to hear ideas:
> >>>http://www.hpl.hp.com/hosted/linux/mail-archives/gc/2012-December/005507.html
> >>> 
> Pavel
> >>> 
> _______________________________________________
> >>> 
> bdwgc mailing list
> >>>[email protected]
> >>>https://lists.opendylan.org/mailman/listinfo/bdwgc
> _______________________________________________
> bdwgc mailing list
> [email protected]
> https://lists.opendylan.org/mailman/listinfo/bdwgc
> 
>