review (M) for 6965184: possible races in make_not_entrant_or_zombie

[Tom Rodriguez]

On Jul 2, 2010, at 5:18 PM, Vladimir Kozlov wrote:

> Tom,
> 
> Nice VTune cleanup :)
> 
> Thank you for consolidating sweep logging code.
> 
> In nmethod.cpp why you don't use the_method() in your new code (// Remove nmethod from method) ?

It's only purpose is to make sure the method doesn't get unloaded until the method exits.  The nmethodLocker keeps the sweeper from actually freeing the nmethod itself but there's nothing stopping it from actually getting unloaded which would make nmethod::_method become null which could break later code.  I considered augmenting the nmethodLocker to also have a strong reference to the method but I think this is the only place that's exposed to that problem.  I think the other uses of nmethodLocker are safe because of their context.

> 
> In sweeper.cpp why you need second (_invocations > 0) check? Only current thread will modify _invocations now after your changes.

I moved the decrement of _invocations later in my changes and didn't go back to make sure the changes were all consistent.

> Why you replaced next code:
> 
> !   // We want to visit all nmethods after NmethodSweepFraction invocations.
> !   // If invocation is 1 we do the rest
> !   int todo = CodeCache::nof_blobs();
> !   if (_invocations > 1) {
> !     todo = (CodeCache::nof_blobs() - _seen) / _invocations;
> !   }
> ...
> !     for(int i = 0; i < todo && _current != NULL; i++) {
> 
> whith this:
> 
> !   int todo = (CodeCache::nof_nmethods() - _seen) / _invocations;
> ...
> !     // The last invocation iterates to the end of the code cache
> !     for (int i = 0; (i < todo || _invocations == 0) && _current != NULL; i++) {

The original code relied on nof_blobs() being greater than the number of nmethods remaining to be processed in the code cache on the final pass.  If it happens to be less then we won't process the whole code cache.  It's very unlikely to happen in practice but I can build a scenario where it would.  I decided to make the last pass explicitly process the whole thing but I moved the decrement of _invocations that broke this code.  It's should be _invocations == 1 so the last pass will process until _current == NULL.

> 
> First, _invocations can't be 0 according to the code in possibly_sweep()
> Second, if it was 0 then you will get division by 0 in todo expression.
> 
> Next assert also looks strange:
> +   assert(_invocations > 0 || _current == NULL, "must have scanned the whole cache");

Same invocations update problem.  I'll fix it and retest and send out a the updated webrev.

tom

> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/6965184
>> 6965184: possible races in make_not_entrant_or_zombie
>> Reviewed-by:
>> While investigating the sweeper concurrency problems I identified a
>> bunch of minor issues that could lead to races.  NMethod groups a
>> bunch of flags into a struct using bitfield which was okay in the past
>> because they were either written by the constructor, at a safepoint or
>> with a single lock held.  Now some of those fields are written under
>> one lock and some are written under another so they should be
>> separated.  I've done away with nmFlags all together and moved a bunch
>> of common initialization into a new method.  I also deleted a other of
>> unused or dead things.
>> In make_not_entrant_or_zombie, all the zombie logic used to be
>> performed at a safepoint but now it can happen concurrently so the
>> logic that is executed outside of the Patching_lock has to be more
>> careful.  All the not_entrant logic has been moved into the region
>> protected by the Patching_lock.  The link between the nmethod and the
>> methodOop is also broken ealier though a strong reference to the
>> methodOop is maintained to make sure it stays alive.
>> There were problems with the maintenance of _sweep_started and
>> _invocations that could mistakenly allow another thread into
>> possibly_sweep but only if there was no work to be done.  Also threads
>> besides the sweeping thread could clear _sweep_started.
>> The sweeper divides the cache based on the number of blobs even though
>> it only works on nmethod, so I added a nof_nmethods count along with a
>> nof_adapters count just for informational purposes.
>> As a cleanup I also deleted the dead and unused VTune logic since it
>> doesn't work and VTune uses JVMTI agents these days.
>> Tested with NSK jvmti,jdi,jdb,hprof,jit,regression and JDI_REGRESSION
>> tests.