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

David Holmes David.Holmes at oracle.com
Tue Jan 11 18:21:53 PST 2011

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->set_lower_dimension(this_oop()); 239 OrderAccess::storestore(); 240 this_oop->set_higher_dimension(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.

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