RFR(s): 8209139: globalCounter bootstrap issue

[David Holmes]

On 8/11/2018 8:42 PM, Robbin Ehn wrote:
> 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.

Ah I see - the list pointed to by _java_threads_list could be 
deallocated as soon as you've read it.

Thanks,
David

  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