RFR (S) 8043070: nmethod::verify_interrupt_point() shouldn't enter safepoint

[Igor Veresov]

> On Nov 16, 2017, at 9:42 PM, dean.long at oracle.com wrote:
> 
> On 11/16/17 6:32 PM, Igor Veresov wrote:
> 
>> 
>>> On Nov 16, 2017, at 3:30 PM, dean.long at oracle.com wrote:
>>> 
>>> Hi Igor.  The changes look good.
>> Thanks!
>> 
>>> Thanks for not changing the values for the other states.  It would have broken AOT :-)
>> Yes, we’d like in_use to be 0 so it could be in BSS, right?
>> 
> 
> And because we compare against 0 right now:
> 
>     3440:       48 8b 05 39 bb 21 00    mov 0x21bb39(%rip),%rax        # 21ef80 <A.meth.state>
>     3447:       48 85 c0                test   %rax,%rax
>     344a:       0f 85 20 b1 00 00       jne    e570 <plt._aot_handle_wrong_method_stub>

Good point, yes.

> 
>>> Are there any situations where make_in_use() might need a compiler/memory barrier?
>>> 
>> I don’t think so, is_in_use() actually returns true for not_installed (now it’s _state <= in_use). So as far as other threads are concerned nothing changed.
>> There is an exotic possibility that the initial store of not_installed might be reordered with the store of in_use on non-TSO, but there is a mutex unlock between those, so I’m not really concerned about that.
>> Are you thinking about any other scenarios?
> 
> I was thinking about make_in_use() happening before the verify() call, but that would mean an invalid optimization by a broken C++ compiler, right?

Yes, I think that would mean it doesn’t track memory effects correctly. No reordering should happen from the point of view of the same thread.

>>> I was thinking about an alternative approach, at least for this case.  We could have NoSafepointVerififer always set _thread->_allow_safepoint_count (right now it doesn't for product builds), then verify_interrupt_point() could check that flag before taking the path that can safepoint.  Or how about just using nmethodLocker as suggested in 8028001?  Or does having a new state help solve other problems?
>> nmethodLocker approach seemed less tidy because I’d have to lock in nmethod::nmethod() and unlock in ciEnv::register_method().
>> We could probably introduce sort of an nmethodHandle that we’d return from new_nmethod() but then we’d have to do reference counting of sorts. Anyway, it felt like an additional state is less trouble.
> 
> OK, a new state does look like less trouble than nmethodLocker or nmethodHandle.


igor

> 
> dl
> 
>> igor
>> 
>>> dl
>>> 
>>> 
>>> On 11/16/17 11:08 AM, Igor Veresov wrote:
>>>> The problems here is that the sweeper can transition a newly created nmethod to not_entrant state before it’s installed. This breaks the logic in nmethod::verify_interrupt_point() called at the end of the installation process. As a result locks might be taken in that can safepoint and safepoints are forbidden during the method install.
>>>> 
>>>> The solution is to introduce an new nmethod state: not_installed. The goal is to be able to prevent certain things like sweeping or parts of the verification happening during the nmethod installation.
>>>> 
>>>> The change passed the failing test and the internal mach5 pre-intergration testing.
>>>> 
>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8043070/webrev.00/
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8043070
>>>> 
>>>> Note, this also fixes: https://bugs.openjdk.java.net/browse/JDK-8028001
>>>> 
>>>> Thanks!
>>>> igor
>