RFR(S): JDK-8129108: nmethod related crash in CMS

[Jesper Wilhelmsson]

Looks good!
/Jesper


Mikael Gerdin skrev den 23/6/15 13:47:
> Hi Poonam,
>
> On 2015-06-22 23:19, Poonam Bajaj Parhar wrote:
>> Hello Mikael,
>>
>> Thanks for taking a look at the changes.
>>
>> Yes, you are right, the best place to add the SO_AllCodeCache
>> root_scanning option is
>> CMSCollector::setup_cms_unloading_and_verification_state(). There the
>> issue is that
>> we are adding/removing the root scanning options based on if the
>> verification options
>> are enabled/disabled.
>>
>> SO_AllCodeCache should get added to the root_scanning_options if we are
>> not unloading
>> classes regardless whether we are verifying or not.
>>
>> I compared this code with the code of jdk7 and the only part that has
>> changed in there is related
>> to the Perm Gen. I could also reproduce the issue with 7u fastdebug
>> build with
>> -XX:+CMSParallelInitialMarkEnabled and -XX:-VerifyBeforeExit.
>>
>> Here's the updated webrev:
>> http://cr.openjdk.java.net/~poonam/8129108/webrev.01/
>>
>> Failing testcase works fine with these changes.
>
> I think the change seems ok, but I'd feel a lot safer if you had some more
> testing results.
>
> /Mikael
>
>>
>> Thanks,
>> Poonam
>>
>> On 6/22/2015 6:41 AM, Mikael Gerdin wrote:
>>> Poonam,
>>>
>>> On 2015-06-20 06:32, Poonam Bajaj Parhar wrote:
>>>> Hello,
>>>>
>>>> Please review this change for bug:
>>>> JDK-8129108: <https://bugs.openjdk.java.net/browse/JDK-8129108> nmethod
>>>> related crash in CM
>>>>
>>>> There is an assertion failure with fastdebug build with test
>>>> /nsk.coverage.arguments.arguments002.arguments002/.
>>>> This failure occurs when CMSCollector::_should_unload_classes is false
>>>> and the _roots_scanning_options does
>>>> not contain SO_AllCodeCache:
>>>>
>>>> #  Internal Error
>>>> (hotspot/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp:4472),
>>>> pid=16267, tid=0xf36ffb70
>>>> #  assert(_collector->should_unload_classes() ||
>>>> (_collector->CMSCollector::roots_scanning_options() &
>>>> GenCollectedHeap::SO_AllCodeCache)) failed: if we didn't scan the code
>>>> cache, we have to be ready to drop nmethods with expired weak oops
>>>>
>>>>
>>>> This problem was exposed with the fix ofJDK-8085965:
>>>> <https://bugs.openjdk.java.net/browse/JDK-8085965> VM hangs in
>>>> C2Compiler,
>>>> that disables CMSClassUnloadingEnabled when -Xnoclassgc or
>>>> -XX:-ClassUnloading
>>>> are specified on the command line.
>>>>
>>>> CMSCollector::_roots_scanning_options option should contain
>>>> SO_AllCodeCache to ensure
>>>> that we scan code cache when we are not unloading classes.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~poonam/8129108/webrev/
>>>
>>> This looks very similar to the code in
>>> CMSCollector::setup_cms_unloading_and_verification_state
>>> which is called from CMSCollector::checkpointRootsInitialWork
>>>
>>> I think that the if-clause in setup_cms_unloading... needs to be fixed
>>> to handle the class unloading case separately from the verification
>>> state.
>>> It's not entirely clear to me exactly which cases the complicated
>>> conditions in the if statements try to capture, though. You may want
>>> to look at a previous version of the sources (before perm gen removal)
>>> to better understand the reasoning behind it.
>>>
>>> /Mikael
>>>
>>>> Testing: JPRT, testcase
>>>> /nsk.coverage.arguments.arguments002.arguments002/
>>>>
>>>> Thanks,
>>>> Poonam
>>>>
>>