RFR(s): 8209139: globalCounter bootstrap issue

[Robbin Ehn]

On 11/8/18 11:19 AM, David Holmes wrote:
> On 8/11/2018 7:14 PM, Robbin Ehn wrote:
>> Hi David,
>>
>> On 11/7/18 10:03 PM, David Holmes wrote:
>>> Hi Robbin,
>>>
>>> I wrote two responses to this last night before deciding to "sleep on it". :)
>>
>> :)
>>
>>>
>>> Thanks for the extended explanations - I understand what you are doing more 
>>> clearly now. I see Dan has made a few minor comments and I don't have 
>>> anything additional to add. So Reviewed.
>>
>> Thanks!
>>
>> For reference:
>> Inc:
>> http://cr.openjdk.java.net/~rehn/8209139/v2/inc/webrev/
> 
> Changes seem fine.

Thanks!

> 
> One clarification. I had interpreted previous discussions as meaning that 
> threads other than the initial thread might be able to grab a ThreadsListHandle 
> before they have been added because they would get the bootstrap list. But that 
> seems not to be the case in general - once an added thread exists (I'm not sure 
> of the exact point where this happens) _java_threads_list will cease to point to 
> the bootstrap list and will never point to it again - correct? 

Yes!

> So it still seems 
> to me you could simply initialize _java_threads_list to a zero length list and 
> the check for "is bootstrap" would just be a check if the list length is zero - 
> no? It's the only time you can ever have a zero length threads list.

The problem is that if it's not the bootstrap list and you are not SMR thread
you are not allowed to deference the pointer, that can crash. So if you are not
a SMR thread we need to determine just on the pointer value that this is the
bootstrap threadslist. With current patch that would not crash there, but we
might crash later or hit the assert. I didn't want to add another way we can
crash. The spot we can crash is on during ThreadsSMRSupport::threads_do() in
verify_hazard_ptr_scanned(). This we should fix so we always hit the assert 
after, which would make troubleshooting simpler for people not so familiar to
the protocol.

Thanks, Robbin

> 
> Thanks,
> David
> 
>> Full:
>> http://cr.openjdk.java.net/~rehn/8209139/v2/full/webrev/
>>
>> /Robbin
>>
>>>
>>> My personal preference is to treat the early initialization in the initial 
>>> thread as a special case - using "hacks" like the 
>>> "Threads::number_of_threads() == 0" test. Allowing any thread to be able to 
>>> access a ThreadsList (and by extension GlobalCounter) before it has properly 
>>> initialized may turn out to be useful one day, but it may also mask bugs, as 
>>> other threads should be following strict initialization protocols that don't 
>>> permit non-trivial actions prior to being fully initialized.
>>>
>>> Thanks,
>>> David
>>>
>>> On 7/11/2018 9:53 PM, Robbin Ehn wrote:
>>>> Hi David,
>>>>
>>>> On 2018-11-07 12:10, David Holmes wrote:
>>>>> Hi Robbin,
>>>>>
>>>>> On 7/11/2018 8:01 PM, Robbin Ehn wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> On 2018-11-07 10:40, David Holmes wrote:
>>>>>>> Hi Robbin,
>>>>>>>
>>>>>>> Can't you just use an _initialized flag ?? 
>>>>>>
>>>>>> I can, but I need to changes many places since the ThreadsList is not 
>>>>>> expected
>>>>>> to be null. And I do not see how checking that flag instead checking if 
>>>>>> the list
>>>>>> is the bootstrap would be better.
>>>>>
>>>>>  From the bug there seemed to be only once place that needed this check. 
>>>>> Where are all these other places?
>>>>
>>>> All potential cases that uses a ThreadsList before init.
>>>> These are very had to track down, so I don't know of any others.
>>>>
>>>> I'm adding that exception, so you may have a ThreadsList if you are _not_ part
>>>> of the SMR protocol if that list is the bootstrap list.
>>>>
>>>>>
>>>>>> Or statically initialize
>>>>>>> _java_thread_list to a zero-length list? 
>>>>>>
>>>>>> It's statically initialized to _bootstrap_list. Since I'm going to need to 
>>>>>> check
>>>>>> for a bootstrap list in a non-SMR-thread I can't follow the pointer 
>>>>>> because we
>>>>>> can crash. So I need to know which address to check for, this is the
>>>>>> _bootstrap_list. And since it's static I can't free it.
>>>>>
>>>>> Not following. Can't you initialize _java_thread_list directly to a 
>>>>> zero-length list and that will "just work" because you'll iterate zero 
>>>>> items? There will only ever be one zero-length threads-list right?
>>>>
>>>> The iteration is not the problem, it is the protocol verification.
>>>> Right know you may only have a ThreadsList if you are part of SMR-protocol,
>>>> which means the VM must have done it's init. With the above exception to the
>>>> protocol the validation works for all cases we have today and all cases we 
>>>> might
>>>> have in the future that I can think of.
>>>>
>>>> With this you can use ThreadsListHandle before init and you don't need to pay
>>>> attention to if your code could ever possibly be run before init or in the init
>>>> code. So I'm trying to create robustness because as SymbolTable issue could 
>>>> pass t1-5 without that code path being executed.
>>>>
>>>> That why I tested with a ThreadsListHandle in the init code to make sure we 
>>>> actually do create at least one there.
>>>>
>>>> /Robbin
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>> This seems overly elaborate.
>>>>>>
>>>>>> If we get a SMR bug doing it different could give us wired crashes instead of
>>>>>> clean assert failures. That is why you think this is 'overly elaborate' I 
>>>>>> guess.
>>>>>>
>>>>>>>
>>>>>>> I'm not even concerned about the existing "hack" - checking for 
>>>>>>> "Threads::number_of_threads() == 0" has always been a simple way to see 
>>>>>>> if you are very early in the init sequence.
>>>>>>
>>>>>> Well it only solves the problem for the GlobalCounter.
>>>>>> And in theory we could have added non-JavaThreads to the RCU thread-set they
>>>>>> should be checked.
>>>>>>
>>>>>> Thanks, Robbin
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>> On 7/11/2018 6:56 PM, Robbin Ehn wrote:
>>>>>>>> Hi all, please review.
>>>>>>>>
>>>>>>>> When changing the hashtable for SymbolTable we did a hack to handle the 
>>>>>>>> case
>>>>>>>> when symbols gets inserted before the VM is proper initialized, by doing 
>>>>>>>> nothing
>>>>>>>> in the GlobalCounter if there is no threads. The problem is that the thread
>>>>>>>> creating the VM is not yet added to the VM (non-SMR-thread) and thus
>>>>>>>> ThreadsListHandle do not yet work for that thread. Trying to move the 
>>>>>>>> addition
>>>>>>>> of the creating thread earlier in the bootstrap is troublesome.
>>>>>>>>
>>>>>>>> This removes that hack and do a more generic solution for ThreadsList 
>>>>>>>> instead.
>>>>>>>> To make it safe to verify that we are talking about the bootstrap 
>>>>>>>> ThreadsList from a non-SMR-thread we cannot follow that pointer. Using 
>>>>>>>> null was intrusive to the code I therefore choose to do a static 
>>>>>>>> bootstrap ThreadsList.
>>>>>>>>
>>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8209139
>>>>>>>> Webrev: http://cr.openjdk.java.net/~rehn/8209139/webrev/
>>>>>>>>
>>>>>>>> Tested with use of ThreadsListHandle and GlobalCounter in the 
>>>>>>>> problematic init method, passed t1-2 with use of ThreadsListHandle in 
>>>>>>>> that init method.
>>>>>>>>
>>>>>>>> Thanks, Robbin