RFR(S): 8153267: nmethod's exception cache not multi-thread safe

[Jamsheed C m]

Thanks for the reply. trying to understand stuffs.

> void nmethod::add_handler_for_exception_and_pc(Handle exception, 
> address pc, address handler) {
>   // There are potential race conditions during exception cache 
> updates, so we
>   // must own the ExceptionCache_lock before doing ANY modifications. 
> Because
>   // we don't lock during reads, it is possible to have several 
> threads attempt
>   // to update the cache with the same data. We need to check for 
> already inserted
>   // copies of the current data before adding it.
>
>   MutexLocker ml(ExceptionCache_lock);
>   ExceptionCache* target_entry = 
> exception_cache_entry_for_exception(exception);
>
>   if (target_entry == NULL || 
> !target_entry->add_address_and_handler(pc,handler)) {
>     target_entry = new ExceptionCache(exception,pc,handler);
>     add_exception_cache_entry(target_entry);
>   }
> }

[1]there is a storestore mem barrier before count is updated in 
add_address_and_handler
this ensure exception pc and handler address are updated before count is 
incremented  and  Exception cache entry is updated at ( 
nm->_exception_cache or in the list ec->_next ).

> address nmethod::handler_for_exception_and_pc(Handle exception, 
> address pc) {
>   // We never grab a lock to read the exception cache, so we may
>   // have false negatives. This is okay, as it can only happen during
>   // the first few exception lookups for a given nmethod.
>   ExceptionCache* ec = exception_cache();
>   while (ec != NULL) {
>     address ret_val;
>     if ((ret_val = ec->match(exception,pc)) != NULL) {
>       return ret_val;
>     }
>     ec = ec->next();
>   }
>   return NULL;
> }

and in read logic. we first check ec entry is available (non null check) 
before proceeding further.
if ec is non null and ec_type,excpetion pc, and handler are available 
by[1]. though count can be reordered and not updated with new value.

this fixes the issue. why you think it doesn't?

Best Regards,
Jamsheed


On 4/5/2016 3:40 PM, Doerr, Martin wrote:
>
> Hi Jamsheed,
>
> thanks for pointing me to it. Interesting that you have found such a 
> problem so shortly before me J
>
> My webrev addresses some aspects which are not covered by your fix:
>
> -add_handler_for_exception_and_pc adds a new ExceptionCache instance 
> in the other case. They need to get released as well.
>
> -The readers of the _exception_cache field are not safe, yet. As 
> Andrew Haley pointed out, optimizers may modify load accesses for 
> non-volatile fields.
>
> So I think my change is still needed.
>
> And after taking a closer look at your change, I think the _count 
> field which is addressed by your fix needs to be volatile as well. I 
> can incorporate that in my change if you like.
>
> Would you agree?
>
> Best regards,
>
> Martin
>
> *From:*hotspot-compiler-dev 
> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] *On Behalf Of 
> *Jamsheed C m
> *Sent:* Montag, 4. April 2016 08:14
> *To:* hotspot-compiler-dev at openjdk.java.net
> *Subject:* Re: RFR(S): 8153267: nmethod's exception cache not 
> multi-thread safe
>
> Hi Martin,
>
> "nmethod's exception cache not multi-thread safe"  bug is fixed in b107
> bug id: https://bugs.openjdk.java.net/browse/JDK-8143897
> fix changeset: 
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/f918c20107d9
> discussion link: 
> http://openjdk.5641.n7.nabble.com/RFR-XS-8143897-Weblogic12medrec-assert-handler-address-SharedRuntime-compute-compiled-exc-handler-nme-td255611.html
>
> Best Regards,
> Jamsheed
>
> On 4/1/2016 6:07 PM, Doerr, Martin wrote:
>
>     Hello everyone,
>
>     we have found a concurrency problem with the nmethod’s exception
>     cache. Readers of the cache may read stale data on weak memory
>     platforms.
>
>     The writers of the cache are synchronized by locks, but there may
>     be concurrent readers: The compiler runtimes use
>     nmethod::handler_for_exception_and_pc to access the cache without
>     locking.
>
>     Therefore, the nmethod's field _exception_cache needs to be
>     volatile and adding new entries must be done by releasing stores.
>     (Loading seems to be fine without acquire because there's an
>     address dependency from the load of the cache to the usage of its
>     contents which is sufficient to ensure ordering on all openjdk
>     platforms.)
>
>     I also added a minor cleanup: I changed nmethod::is_alive to read
>     the volatile field _state only once. It is certainly undesired to
>     force the compiler to load it from memory twice.
>
>     Webrev is here:
>
>     http://cr.openjdk.java.net/~mdoerr/8153267_exception_cache/webrev.00/
>     <http://cr.openjdk.java.net/%7Emdoerr/8153267_exception_cache/webrev.00/>
>
>     Please review. I will also need a sponsor.
>
>     Best regards,
>
>     Martin
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160406/0fa4f488/attachment-0001.html>