RFR(s): 8209139: globalCounter bootstrap issue

[Daniel D. Daugherty]

On 11/8/18 6:02 AM, David Holmes wrote:
> 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.

If the ThreadsList pointed to by _java_threads_list is not acquired
by a ThreadsListHandle, then that ThreadsList can be freed at any
point after it is manually copied.

Dan


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