Request for reviews (XS): 7011386: race in objArrayKlass::array_klass_impl (original) (raw)

David Holmes David.Holmes at oracle.com
Tue Jan 11 18:28:51 PST 2011

David Holmes said the following on 01/12/11 12:21:

Vladimir Kozlov said the following on 01/12/11 09:48:

You are right, I added storestore barriers and make these fields volatile:

http://cr.openjdk.java.net/~kvn/7011386/webrev It strikes me as very odd to have to add explicit memory barriers to code that holds two locks while it executes! I understand the problem is that there is a lock-free reader of the values being set. Where is the code that does the lock-free reading of these values? Here: 238 ak->setlowerdimension(thisoop()); 239 OrderAccess::storestore(); 240 thisoop->sethigherdimension(ak()); 241 OrderAccess::storestore(); what is the subsequent store that #241 is ordering with? I think this is either unnecessary, or else is intended to be a full fence() if we are really saying that the store at #240 must be made visible to all other threads.

On further thought I'd say it is unnecessary. Following the barrier there will be two mutex releases that involve additional barriers. The key requirement is that if you see the store from #240 then you must see the store from #238, and the storestore at #239 achieves that.

David

David

Vladimir

Tom Rodriguez wrote: On Jan 11, 2011, at 11:15 AM, Vladimir Kozlov wrote:

Tom,

Using oopstore() instead of oopstorewithoutcheck() will generate barriers but it is overkill. Those are gc barriers. I was really referring to fences. The code is a classic instance of the double check locking idiom for lazy init: objArrayKlassHandle ak (THREAD, thisoop->higherdimension()); if (ak.isnull()) { if (ornull) return NULL; ResourceMark rm; JavaThread *jt = (JavaThread *)THREAD; { MutexLocker mc(Compilelock, THREAD); // for vtables // Ensure atomic creation of higher dimensions MutexLocker mu(MultiArraylock, THREAD); // Check if another thread beat us ak = objArrayKlassHandle(THREAD, thisoop->higherdimension()); if( ak.isnull() ) { So doesn't that mean that we need a fence of some kind? tom I think marking receiver as volatile should be enough: void sethigherdimension(klassOop k) volatile { oopstorewithoutcheck((oop*) &higherdimension, (oop) k); } void setlowerdimension(klassOop k) volatile { oopstorewithoutcheck((oop*) &lowerdimension, (oop) k); } Thanks, Vladimir Tom Rodriguez wrote: That look good, though I keep wondering if we need a barrier in between or if those fields really should be volatile. It seems like we're playing a little loose with the locking for these lazy values. tom On Jan 11, 2011, at 9:54 AM, Vladimir Kozlov wrote: http://cr.openjdk.java.net/~kvn/7011386/webrev

Fixed 7011386: race in objArrayKlass::arrayklassimpl Other threads may access lowerdimension field before it is initialized by thread which holds the lock in objArrayKlass::arrayklassimpl(). Move lowerdimension field initialization before higherdimension.

More information about the hotspot-runtime-dev mailing list