RFR: Fix jtreg test runtime/StackGuardPages/

[Mikael Vidstedt]

Looks good, thanks for making the changes!

Send to me and I’ll push.

Cheers,
Mikael

> On Apr 6, 2018, at 1:23 AM, Siebenborn, Axel <axel.siebenborn at sap.com> wrote:
> 
> Hi David,
> You're right, it's better not to use a *_np function.
> 
> I prepared a new webrev:
> http://cr.openjdk.java.net/~asiebenborn/jtreg_StackGuardPages/webrev_c/
> 
> Thanks,
> Axel
> 
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Freitag, 6. April 2018 09:13
>> To: Siebenborn, Axel <axel.siebenborn at sap.com>; Mikael Vidstedt
>> <mikael.vidstedt at oracle.com>
>> Cc: portola-dev at openjdk.java.net
>> Subject: Re: RFR: Fix jtreg test runtime/StackGuardPages/
>> 
>> On 6/04/2018 4:41 PM, Siebenborn, Axel wrote:
>>> Hi,
>>> As discussed off-list with Mikael, I get a reasonable stack size for
>> pthread_create by using JNI_GetDefaultJavaVMInitArgs.
>>> This way, we have a stack suitable to run java code.  The stacksize  is not
>> hardcoded in the test and platform independent.
>> 
>> The test isn't platform independent when it uses a *_np function.
>> 
>> I'm not sure why you are changing the default attributes used by
>> pthread_create instead of just passing a suitable attr object when
>> creating the new thread ref. pthread_attr_setstacksize ??
>> 
>> Cheers,
>> David
>> 
>>> I updated the webrev:
>>> 
>> http://cr.openjdk.java.net/~asiebenborn/jtreg_StackGuardPages/webrev_a
>> /
>>> 
>>> Regards,
>>> Axel
>>> 
>>>> -----Original Message-----
>>>> From: Mikael Vidstedt [mailto:mikael.vidstedt at oracle.com]
>>>> Sent: Dienstag, 3. April 2018 19:31
>>>> To: Siebenborn, Axel <axel.siebenborn at sap.com>
>>>> Cc: portola-dev at openjdk.java.net
>>>> Subject: Re: RFR: Fix jtreg test runtime/StackGuardPages/
>>>> 
>>>> 
>>>> The total stack size is 80kb, of which 40kb/10 pages worth is used for
>> shadow
>>>> pages (48kb/12 pages in debug mode). That leaves 80-48=32kb stack for
>>>> execution, which admittedly is not a lot in today’s world. However, I
>> would
>>>> have thought that the initialization would still have room to play in that,
>> but it
>>>> seems not?
>>>> 
>>>> I also assume that the reason why this isn’t a problem for the normal
>>>> launcher case is that the launcher doesn’t use the primordial. Perhaps the
>>>> right way of fixing this is buy having the test specific launcher here mimic
>>>> what the real launcher does (something not entirely unlike
>>>> ContinueInNewThread)?
>>>> 
>>>> Cheers,
>>>> Mikael
>>>> 
>>>>> On Apr 3, 2018, at 5:30 AM, Siebenborn, Axel
>> <axel.siebenborn at sap.com>
>>>> wrote:
>>>>> 
>>>>> Hi Mikael,
>>>>> Both  tests fails because
>>>>> res = (*_jvm)->AttachCurrentThread(_jvm, (void **)&env, NULL);
>>>>> returns with an error (res != JNI_OK).
>>>>> 
>>>>> AttachCurrentThread uses the current thread as JavaThread and
>> allocates a
>>>> java level Thread object and needs to run initializing java code
>>>> (JavaThread::allocate_threadObj).
>>>>> In order to run java code, the remaining stack space is checked. There
>> must
>>>> be sufficient  space for an interpreter frame, the java frame and shadow
>>>> pages ( JavaCalls::call_helper() ). The default for the number of shadow
>>>> pages is 10 for a release build. If this check fails a StackOverflowException
>> is
>>>> thrown.
>>>>> As we return with a pending exception the attach fails and we return
>>>> JNI_ERR.
>>>>> 
>>>>> This is a problem as we call AttachCurrentThread from a thread that we
>>>> created with the default stacksize. Threads created by the jvm are
>> created
>>>> with a much higher stacksize.
>>>>> 
>>>>> webrev_a fixes the test by providing an argument for the stacksize to
>>>> pthread_create.
>>>>> 
>>>>> webrev_b reduces the amount of shadow pages.
>>>>> 
>>>>> Does that make things clearer?
>>>>> 
>>>>> Regards,
>>>>> Axel
>>>>> 
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Mikael Vidstedt [mailto:mikael.vidstedt at oracle.com]
>>>>>> Sent: Dienstag, 3. April 2018 00:01
>>>>>> To: Siebenborn, Axel <axel.siebenborn at sap.com>
>>>>>> Cc: portola-dev at openjdk.java.net
>>>>>> Subject: Re: RFR: Fix jtreg test runtime/StackGuardPages/
>>>>>> 
>>>>>> 
>>>>>> Alex,
>>>>>> 
>>>>>> Can you please add some additional color/details to why it fails? Is it the
>>>>>> native or java variant that fails (or both)?
>>>>>> 
>>>>>> I understand that the stack is smaller with musl (I ran into that problem
>> in
>>>> a
>>>>>> different context), but I’m not sure I see why Thread.init() would fail
>> just
>>>>>> because of that. Help? :)
>>>>>> 
>>>>>> Cheers,
>>>>>> Mikael
>>>>>> 
>>>>>>> On Mar 22, 2018, at 3:37 AM, Siebenborn, Axel
>>>>>> <axel.siebenborn at sap.com> wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> the test  runtime/StackGuardPages/ fails, as AttachCurrentThread()
>>>> returns
>>>>>> with an error. The reason is a stackoverflow while trying to run java
>>>>>> Thread.init().
>>>>>>> The default pthread stack size is 80 K, much less than on other Unixes.
>>>>>>> 
>>>>>>> The first fix is to check the pthread default stacksize and increase it if
>> it's
>>>>>> less than 160K.
>>>>>>> 
>>>>>>> Webrev a:
>>>>>>> 
>>>>>> 
>>>> 
>> http://cr.openjdk.java.net/~asiebenborn/jtreg_StackGuardPages/webrev_a
>>>>>> /
>>>>>>> 
>>>>>>> However, the reason we need that much stack, just to run a single
>> java
>>>>>> method, is the large amount of shadow pages.
>>>>>>> The second fix is just to add a flag to JNI_CreateJavaVM, to set the
>>>> number
>>>>>> of shadow pages to a lower value. However, a change in
>> globals_x86.hpp
>>>> is
>>>>>> needed to allow smaller minimum for that value. Here I miss a macro
>> like
>>>>>> MUSL_OLNLY(code).
>>>>>>> 
>>>>>>> Webrev b:
>>>>>>> 
>>>>>> 
>>>> 
>> http://cr.openjdk.java.net/~asiebenborn/jtreg_StackGuardPages/webrev_
>>>>>> b/
>>>>>>> 
>>>>>>> I'm unsure which change to use.
>>>>>>> What do you think?
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Axel
>>>>>>> 
>>>>>>> 
>>>>> 
>>>