RFR(s): 8209139: globalCounter bootstrap issue

[Robbin Ehn]

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/
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