RFR(s): 8209139: globalCounter bootstrap issue

[David Holmes]

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.

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? 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.

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