'Re: RFR: 8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits
From:       Stefan Karlsson <stefan.karlsson () oracle ! com>
Date:       2017-06-30 18:02:43
Message-ID: 89e41c0a-2e8e-1e79-4044-5e134b22a9a5 () oracle ! com
[Download RAW message or body]

On 2017-06-30 19:24, Daniel D. Daugherty wrote:
> One part of this thread caught my eye:
>
> > Is it important that ThreadStackSize can be set to 0? IIRC, there are
> > some code paths that interpret ThreadStackSize == 0 as a way for the
> > heuristics to choose value.
>
> Yes, it is important to be able to specify all three ThreadStackSize
> options with a value of zero. It means use the default size...
>
> http://www.oracle.com/technetwork/articles/java/vmoptions-jsp-140102.html
>
> -XX:ThreadStackSize=512
>
>     Thread Stack Size (in Kbytes). (0 means use default stack size)
>     [Sparc: 512; Solaris x86: 320 (was 256 prior in 5.0 and earlier);
>     Sparc 64 bit: 1024; Linux amd64: 1024 (was 0 in 5.0 and earlier);
>     all others 0.]
>
> This may have been addressed by other comments in the thread. It looks
> like I'm missing a few emails in this thread. (So I have found yet
> another case where Beehive ate an email or two or three...)

Thanks Dan!

I maintained the ability to -XX:ThreadStackSize to 0 in the current webrev.

Thanks,
StefanK

>
> Dan
>
>
> On 6/29/17 1:33 PM, Stefan Karlsson wrote:
>> Hi Gerard,
>>
>> On 2017-06-29 17:49, Gerard Ziemski wrote:
>>> hi Stefan,
>>>
>>> Thank you for doing this.
>>>
>>> Please see below for my comments.
>>>
>>>> From: Stefan Karlsson <stefan.karlsson@oracle.com>
>>>> Subject: Re: RFR: 8178491: -Xss and -XX:ThreadStackSize argument 
>>>> parsing truncates bits
>>>> Date: June 29, 2017 at 6:33:19 AM CDT
>>>> To: "hotspot-runtime-dev@openjdk.java.net" 
>>>> <hotspot-runtime-dev@openjdk.java.net>
>>>>
>>>> Hi all,
>>>>
>>>> I got a request form Kim to remove the friend macrology in the 
>>>> test. Here's the update webrevs:
>>>> http://cr.openjdk.java.net/~stefank/8178491/webrev.01
>>>> http://cr.openjdk.java.net/~stefank/8178491/webrev.01.delta
>>>>
>>>> StefanK
>>>>
>>>> On 2017-06-28 16:05, Stefan Karlsson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this patch to fix bugs in the -Xss parsing.
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8178491/webrev.00/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178491
>>>>>
>>>>> I found this bug while working on an Enhancement to the align 
>>>>> functions:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178489
>>>>>
>>>>> In arguments.cpp we have the following code in the section that 
>>>>> converts the Xss value to a ThreadStackSize value:
>>>>> round_to((int)long_ThreadStackSize, K) / K) != Flag::SUCCESS)
>>>>>
>>>>> This explicit narrowing cast throws away the upper 32 bits of 
>>>>> long_ThreadStackSize, the resulting int is then promoted to an 
>>>>> intptr_t used as a argument to round_to. This causes weird error 
>>>>> messages and behavior. For example, running:
>>>>> java -Xss2147483648 -version
>>>>>
>>>>> Yields:
>>>>> intx ThreadStackSize=18014398507384832 is outside the allowed 
>>>>> range [ 0 ... 9007199254740987 ]
>>>>>
>>>>> My initial reaction was that this could be solved by simply fixing 
>>>>> the cast, but further inspection of the code parsing -Xss and 
>>>>> ThreadStackSize revealed more issues.
>>>>>
>>>>> Changes in this patch:
>>>>>
>>>>> 1) Extract the -Xss parsing code into a separate, testable 
>>>>> function. Unit tests added.
>>>>>
>>>>> 2) Extended parse_memory_size and check_memory_size, to take the 
>>>>> maximum input value. Unit tests added.
>>>>>
>>>>> 3) Provide an upper limit for the values allowed in the call to 
>>>>> parse_memory_size in the -Xss parsing. The upper limit is 
>>>>> calculated backwards from the the upper limit of the ThreadStackSize.
>>>>>
>>>>> 4) The max input value for ThreadStackSize was artificially 
>>>>> limited by the fact that round_to was used, and that limited 
>>>>> values to fit in an intptr_t, even though size_t is used when the 
>>>>> ThreadStackSize is finally expanded into page size aligned bytes. 
>>>>> This meant that we had an extremly high upper limit, but it wasn't 
>>>>> the absolute upper limit. I changed this to the absolute upper 
>>>>> limit, so that it's easier to reason about why this limit was 
>>>>> chosen. To make this possible, I replaced the usages round_to and 
>>>>> round_down, with align_size_up_ and align_size_down_ macros. In 
>>>>> future patches, when the input parameter types of our align 
>>>>> functions have been fixed, I'll convert the usages of these macros.
>>>>>
>>>>> 5) More asserts to check for unintended overflows.
>>>>>
>>>>> IMHO, It's a bit silly to allow this high values for -Xss, and we 
>>>>> could probably limit it to more practical values. I experimented 
>>>>> with that, but I found that the code and tests became more 
>>>>> unintuitive than they already are. So, I'd like to leave that as a 
>>>>> future enhancement if the maintainers of this code allows it.
>>>>>
>>>>> Tested with JPRT, TooSmallStackSize.java, and 
>>>>> TestThreadStackSizes.java. Any suggestions on how to further test 
>>>>> this?
>>> #1 Re src/share/vm/runtime/arguments.cpp
>>>
>>> +  // The min and max sizes match the values in globals.hpp.
>>> +  const julong min_size = 1000;
>>>
>>> The comment refers the reader to globals.hpp for min, but where 
>>> exactly in globals.hpp is this value?
>>>
>>> I see this line in globals.hpp file:
>>>
>>> +          range(0, intx(align_size_down_(max_uintx, 
>>> os::vm_page_size())/K)) \
>>>
>>> which would imply 0 as the min, and I also see this line in 
>>> arguments.cpp:
>>>
>>> -      ArgsRange errcode = parse_memory_size(tail, 
>>> &long_ThreadStackSize, 1000);
>>>
>>> which uses 1000 as min.
>>>
>>> Should we change this line in globals.hpp, from:
>>>
>>> +          range(0, intx(align_size_down_(max_uintx, 
>>> os::vm_page_size())/K)) \
>>>
>>> to:
>>>
>>> +          range(1000, intx(align_size_down_(max_uintx, 
>>> os::vm_page_size())/K)) \
>>>
>>> to make things consistent? Then the comment can stay as is.
>>
>> No, the comment is confusing.
>>
>> We have two different ranges, one for ThreadStackSize (scaled down by 
>> 1024) and another for -Xss (unscaled). I want to somehow direct the 
>> reader over to the globals.hpp file and read the range for 
>> ThreadStackSize, to be able to understand the ranges in parse_xss. I 
>> think the max values are somewhat understandable, but the min_size = 
>> 1000 isn't. Note, that parse_xss aligns all input values against 
>> 1024, so we could actually set the min_size to 1 instead of 1000. 
>> Maybe we could even allow 0, and at that point the two ranges would 
>> match better. Is it important that ThreadStackSize can be set to 0? 
>> IIRC, there are some code paths that interpret ThreadStackSize == 0 
>> as a way for the heuristics to choose value.
>>
>> I could drop the comment about min size, if we don't want to spend 
>> more time on that part:
>>
>> +  // The max size match the value in globals.hpp.
>>
>>>
>>>
>>> #2 Re src/share/vm/runtime/arguments.cpp
>>>
>>> +    bool silent = option == NULL; // Allow testing to silence error 
>>> messages
>>>
>>> Would adding brackets like:
>>>
>>> +    bool silent = (option == NULL); // Allow testing to silence 
>>> error messages
>>>
>>> make things a bit easier to read?
>>
>> I can make that change.
>>
>>>
>>>
>>> #3 Can you verify with 
>>> "hotspot/test/runtime/CommandLine/OptionsValidation" test?
>>
>> Sure.
>>
>> StefanK
>>>
>>>
>>> cheers
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic