Is _thread_in_native safe for handshake/safepoint?

[Reingruber, Richard]

Hi Robbin,

> On 2020-10-27 17:43, Reingruber, Richard wrote:
> > Yeah, avoiding register flushes would be a reason but practically always the
> > stack is made walkable before changing to _thread_in_native (also in 11u which
> > still supports SPARC). That's why I asked myself if there are code paths which
> > don't do that.

> The check was added 2004 together with:

> "
> ...
> The code and mechanism known as SafepointPolling replaces this method of 
> bringing threads executing in compiled code to a safepoint. Code is 
> generated in the compiled code to poll for whether a safepoint should be 
> reached.
> ...
> "

> It's unclear if this check was needed or added as a safety.

That's a while ago...

> > But what needs to be fixed in ThreadInVMfromNative? I suppose the (java part of
> > the) stack is walkable when the instance is created or there is no frame
> > on stack. This could be asserted like in line 657 above. Then it is still
> > walkable when the instance is destructed, isn't it?

> I was not 100% sure of this, but I cannot find any place where we would 
> return to native and not have it proper set (~ThreadInVMfromNative).
> Let's fix the JavaThreadInVMAndNative and do some heavy testing.

Sounds like a plan.

Thanks again for looking into this,
Richard.

-----Original Message-----
From: Robbin Ehn <robbin.ehn at oracle.com> 
Sent: Donnerstag, 29. Oktober 2020 11:12
To: Reingruber, Richard <richard.reingruber at sap.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: Is _thread_in_native safe for handshake/safepoint?

Hi Richard,

On 2020-10-27 17:43, Reingruber, Richard wrote:
> Yeah, avoiding register flushes would be a reason but practically always the
> stack is made walkable before changing to _thread_in_native (also in 11u which
> still supports SPARC). That's why I asked myself if there are code paths which
> don't do that.

The check was added 2004 together with:

"
...
The code and mechanism known as SafepointPolling replaces this method of 
bringing threads executing in compiled code to a safepoint. Code is 
generated in the compiled code to poll for whether a safepoint should be 
reached.
...
"

It's unclear if this check was needed or added as a safety.

> But what needs to be fixed in ThreadInVMfromNative? I suppose the (java part of
> the) stack is walkable when the instance is created or there is no frame
> on stack. This could be asserted like in line 657 above. Then it is still
> walkable when the instance is destructed, isn't it?

I was not 100% sure of this, but I cannot find any place where we would 
return to native and not have it proper set (~ThreadInVMfromNative).
Let's fix the JavaThreadInVMAndNative and do some heavy testing.

/Robbin

> 
>> We are looking into some simplifications of transitions, since after
>> "8203469: Faster safepoints" blocked and native are almost identical.
> 
>> Essential two super-states thread-safe (blocked/native) and
>> thread-unsafe(all other). Having the same 'rules' for walkable stack
>> certainly helps.
> 
> I see.
> 
>> Does this answer your question(s)?
> 
> To some extent. Unfortunately I haven't understood your answers
> completely. Anyways thanks!
> 
> Cheers, Richard.
> 
> -----Original Message-----
> From: Robbin Ehn <robbin.ehn at oracle.com>
> Sent: Dienstag, 27. Oktober 2020 09:16
> To: Reingruber, Richard <richard.reingruber at sap.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: Is _thread_in_native safe for handshake/safepoint?
> 
> Hi Richard,
> 
> I think the main reason for not making the stack walkable is to avoid
> register flushes. Since we do not have such hardware anymore in mainline
> I see no reason to skip making the stack walkable.
> 
> I find two places, JFR and ~ThreadInVMfromNative().
> The JavaThreadInVMAndNative should be replaced.
> In case of ThreadInVMfromNative I guess the idea is that when going from
> java -> native we already did the stack walkable and then calling e.g.
> JNI we avoid it. And you are probably going to do many JNI calls.
> 
> So fixing those two maybe we can simplify to:
>   >   650	  case _thread_in_native:
>   >   654	  case _thread_blocked:
>   >   657	    assert(!thread->has_last_Java_frame() ||
> thread->frame_anchor()->walkable(), "blocked and not walkable");
>   >   658	    return true;
> 
> Which would be very nice.
> 
> We are looking into some simplifications of transitions, since after
> "8203469: Faster safepoints" blocked and native are almost identical.
> 
> Essential two super-states thread-safe (blocked/native) and
> thread-unsafe(all other). Having the same 'rules' for walkable stack
> certainly helps.
> 
> Does this answer your question(s)?
> 
> Thanks, Robbin
> 
> On 2020-10-22 16:22, Reingruber, Richard wrote:
>> Hi,
>>
>> I don't understand the method safepoint_safe_with() in safepoint.cpp
>> Maybe somebody has a quick answer to the questions below...
>>
>>    648	static bool safepoint_safe_with(JavaThread *thread, JavaThreadState state) {
>>    649	  switch(state) {
>>    650	  case _thread_in_native:
>>    651	    // native threads are safe if they have no java stack or have walkable stack
>>    652	    return !thread->has_last_Java_frame() || thread->frame_anchor()->walkable();
>>    653	
>>    654	  case _thread_blocked:
>>    655	    // On wait_barrier or blocked.
>>    656	    // Blocked threads should already have walkable stack.
>>    657	    assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "blocked and not walkable");
>>    658	    return true;
>>    659	
>>    660	  default:
>>    661	    return false;
>>    662	  }
>>    663	}
>>
>> I thought _thread_in_native and _thread_blocked are both equally safe for
>> handshakes/safepoints but that method makes a distinction. Looking at line 652
>> it seems possible that a thread is _thread_in_native without a walkable
>> stack. The thread is then of course considered not safe. Native wrappers, native
>> interpreter entries, ThreadToNativeFromVM change the state to _thread_in_native
>> after making the stack walkable though (I looked also at the old sparc versions).
>>
>> So how can a thread be _thread_in_native without a walkable stack? And why does
>> that state make sense when it blocks handshakes/safepoints?
>>
>> I found JavaThreadInVMAndNative which seems to set the state to
>> _thread_in_native without making the stack walkable. It seems though that from
>> there ThreadInVMfromNative is reachable which can trigger an assertion in
>> JavaThread::check_safepoint_and_suspend_for_native_trans() if the stack isn't
>> walkable.
>>
>> JfrRecorderService::invoke_safepoint_write() : void
>> 	JfrRecorderService::write() : void
>> 		JfrRecorderService::finalize_current_chunk() : void
>> 			JfrRecorderService::chunk_rotation() : void
>> 				JfrRecorderService::rotate(int) : void
>> 					JfrEmergencyDump::on_vm_shutdown(bool) : void
>>
>> Thanks, Richard.
>>