RFR (M): 8029396: PPC64 (part 212): Several memory ordering fixes in C-code.

[David Holmes]

I had overlooked the fact that these changes had been pushed and was 
awaiting further discussion. :(

I am concerned about the thread state related changes as they are 
potentially/likely redundant. Thread state transitions generally involve 
full memory fences as part of the transition. Concerns about store 
ordering in the lead up to that, eg _last_java_frame, need to be 
examined in detail to see where the variable is actually read. In many 
cases you will find that variables are only read in conditions where 
there has already been explicit synchronization between the threads 
involved - eg at a safepoint, or when a thread has been suspended etc.

Plus I ask again:
 > With regards to this part of the code do you force UseMembar true for
 > PPC64? The memory serialization page mechanism is not reliable for
 > non-TSO systems.

Cheers,
David
-----

On 20/12/2013 11:33 AM, David Holmes wrote:
> Hi Goetz,
>
> On 20/12/2013 12:19 AM, Lindenmaier, Goetz wrote:
>> Hi David,
>>
>> the GC stuff is only called from shared code.
>
> Good to hear. I always wonder though whether the cost of passing the
> extra parameter through and checking it, outweighs the benefit of not
> issuing the action (the release in this case)? I'm not a compiler person
> but perhaps the extra parameter forces parameter passing via the stack
> rather than registers, or changes an inlining decision, or maybe the
> additional control flow check causes a problem ... Any hard data that
> not using release semantics all the time actually yields a benefit?
>
>> The ordering in BiasedLocking is needed, e.g., in the context of
>> force_revoke_and_rebias.
>> If an other thread wants to inflate the lock written to the mark word
>> in force_revoke_and_rebias, it must be assured that changing the
>> displaced
>> header is visible to that other thread.
>
> I'll take your word for it. I don't have time to try and analyse the
> BiasedLocking code in depth and I don't think it is a performance issue
> for that code given the potentially redundant barrier occurs during a
> safepoint anyway.
>
>> We added the memory barriers for the _thread_state field in 2006 and can
>> not reconstruct the concrete cause.  But things as setting the
>> last_java_frame
>> and then the state to in_native should be ordered.
>
> I am much more concerned about this. I can't accept a simple wrapping of
> all accesses with acquire/release semantics because there may be a few
> cases where it is needed - this code is too hot for that. We need to
> examine the individual cases, like last_java_frame, where you think
> there is an issue.
>
> With regards to this part of the code do you force UseMembar true for
> PPC64? The memory serialization page mechanism is not reliable for
> non-TSO systems.
>
> My Xmas break begins in about 5 hours but I will be checking in on email
> at times :)
>
> Cheers,
> David
>
>> Best regards,
>>    Goetz.
>>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Donnerstag, 19. Dezember 2013 11:52
>> To: Lindenmaier, Goetz
>> Cc: 'hotspot-dev at openjdk.java.net';
>> 'ppc-aix-port-dev at openjdk.java.net'; Vladimir Kozlov
>> Subject: Re: RFR (M): 8029396: PPC64 (part 212): Several memory
>> ordering fixes in C-code.
>>
>> Somewhat late but I was away for two weeks.
>>
>> GC stuff:
>>
>> Is the use of the new release parameter always set to true from shared
>> code? If so I don't have a problem with it being used to optimize the
>> stores under certain conditions. But if pcc64 will pass true where other
>> archs won't then again I object to this because it should be an
>> algorithmic correctness requirement that is always present.
>>
>>
>> General: I find a lot of the commentary excessively platform specific.
>> Eg We don't expect any C++ compiler we currently use to emit memory
>> barriers for C++ volatiles. If they start doing that we will have a
>> bazillion unnecessary injected barriers in our code!
>>
>> BiasedLocking:
>>
>> It is not clear to me that the BiasedLocking change is needed. AFAICS
>> there is only one path where revoke_bias is not called at a safepoint
>> and the comments around that seem to indicate that it was considered
>> safe to do so. It may be they assumed TSO when making that decision but
>> I'd be interested to know how this change was arrived at.
>>
>> Thread state:
>>
>> The thread state changes have me most concerned as they are heavily used
>> and so the performance impact here could be extensive. Many of them
>> occur on paths that involve membars or membar-equivalent actions so they
>> would seem redundant then. Again I would like to see some analysis
>> showing these are in fact essential for correctness. There may well be
>> some situations where they are, but to me this seems an even better
>> candidate for adding the "release" parameter when needed!
>>
>> David
>> -----
>>
>> On 3/12/2013 2:51 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> This change contains a row of fixes to the memory ordering in
>>> runtime, GC etc.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8029396-0-memo/
>>>
>>> These are:
>>> - Accessing arrays in CMS (compactibleFreeListSpace.cpp)
>>> - CMS: Do release when marking a card dirty. The release must only be
>>> done if GC is running. (several files)
>>> - Method counter initialization (method.hpp).
>>> - Order accessing f1/f2 in constant pool cache.
>>> - Release stores in OopMapCache constructor (instanceKLass.cpp).
>>> - BiasedLocking: Release setting object header to displaced mark.
>>> - Release state of nmethod sweeper (sweeper.cpp).
>>> - Do barriers when writing the thread state (thread.hpp).
>>>
>>> Please review and test this change.
>>>
>>> If requested, I can part this into smaller changes.  But for now
>>> I wanted to put them all into one change as they all address the
>>> problems with the PPC memory model.
>>>
>>> Best regards,
>>>     Goetz.
>>>