RFR(S): 7147724: G1: hang in SurrogateLockerThread::manipulatePLL

[Tony Printezis]

John,

Hi. The changes related to the VM operations (i.e. the introduction and 
use of the should_retry_gc flag) are fine.

I have some concerns about taking the locks with the safepoint check though.

new_region_try_secondary_free_list()

is called from:
new_region()

which (for mutator threads that are allocating) is called from:
new_mutator_alloc_region()

which in turn is called from the MutatorAllocRegion instance when the 
current mutator alloc region needs to be replaced. This is done while 
holding the Heap_lock (to make sure the operation is atomic) and I don't 
think a safepoint should happen during it. So, will doing the safepoint 
check potentially allow a safepoint during this atomic operation?

On the other hard, VM ops (the GC VM ops at least) take the Heap_lock 
first before initiating the safepoint. So, doesn't this mean that 
whether a mutator thread does or does not the safepoint check when it's 
allocating a new region really does not matter given that a safepoint 
cannot happen while it's holding the Heap_lock?

I might be missing something here...

Tony

On 03/09/2012 06:54 PM, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks for looking again. Replies inline....
>
> On 03/09/12 02:25, Bengt Rutisson wrote:
>>
>> Hi John,
>>
>> I think I like this change, but either I am confused or you forgot to 
>> remove the "bool no_safepoint_check" from:
>>
>> G1CollectedHeap::humongous_obj_allocate_find_first()
>> G1CollectedHeap::humongous_obj_allocate()
>>
>> Did you mean to remove it?
>
> Yes I did mean to remove it. Done.
>
>>
>> Just a style comment on this piece of code:
>>
>>     bool safepoint_check_flag = Mutex::_no_safepoint_check_flag;
>>     if (Thread::current()->is_Java_thread()) {
>>       safepoint_check_flag = !Mutex::_no_safepoint_check_flag;
>>     }
>>
>> I understand that we want to use Mutex::_no_safepoint_check_flag 
>> since it is defined. But since there is no 
>> Mutex::_safepoint_check_flag or similar I think we break the 
>> abstraction by doing "safepoint_check_flag = 
>> !Mutex::_no_safepoint_check_flag". This kind of exposes that we are 
>> dealing with a bool. So, given this I think I would prefer to change 
>> the code to just:
>>
>> safepoint_check_flag = Thread::current()->is_Java_thread();
>
> OK. Done, but with a small difference. For Java threads we wish to 
> pass false so that we don't skip the safepoint check so I used "bool 
> no_safepoint_check_flag = !Thread::current()->is_Java_thread();" and 
> can then just pass in the value.
>
> New webrev: http://cr.openjdk.java.net/~johnc/7147724/webrev.2/
>
> Thanks,
>
> JohnC