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

David Holmes david.holmes at oracle.com
Wed Jan 15 19:32:24 PST 2014

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 forcerevokeandrebias. If an other thread wants to inflate the lock written to the mark word in forcerevokeandrebias, 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 threadstate field in 2006 and can not reconstruct the concrete cause. But things as setting the lastjavaframe and then the state to innative 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 lastjavaframe, 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 revokebias 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.

More information about the hotspot-dev mailing list