RFR: 8049536: os::commit_memory on Solaris uses aligment_hint as page size

[Stefan Karlsson]

On 2014-09-12 15:08, Erik Helin wrote:
> All,
>
> quick feedback from StefanK:
> - forgot to cast vm_page_size() to size_t in one place
> - print caddr_t with PRT_FORMAT in error message
>
> Webrevs:
> - full: http://cr.openjdk.java.net/~ehelin/8049536/webrev.03/
>  incremental: http://cr.openjdk.java.net/~ehelin/8049536/webrev.02-03/

Looks good.

thanks,
StefanK

>
> Thanks,
> Erik
>
> On 2014-09-12 14:52, Erik Helin wrote:
>> Hi,
>>
>> I've uploaded a new patch that also contains a test:
>> - full: http://cr.openjdk.java.net/~ehelin/8049536/webrev.02/
>> - incremental: http://cr.openjdk.java.net/~ehelin/8049536/webrev.01-02/
>>
>> The test only provokes the bug together with the patch for JDK-8027915
>> and without this patch. Since we don't want to break nightly testing, it
>> is better to push this patch first and then push the patch for
>> JDK-8027915 (even though the test isn't that meaningful in this patch).
>>
>> Thanks,
>> Erik
>>
>> On 2014-09-11 18:47, Erik Helin wrote:
>>> Hi Thomas,
>>>
>>> thanks for having a look!
>>>
>>> On 2014-09-11 17:23, Thomas Schatzl wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Thu, 2014-09-11 at 16:15 +0200, Erik Helin wrote:
>>>>> Hi all,
>>>>>
>>>>> this small patch fixes a problem with the arguments to the syscall
>>>>> memcntl on Solaris.
>>>>>
>>>>> When memcntl is used with MHA_MAPSIZE_VA to tell the system the
>>>>> preferred page size for a memory area, the mha_pagesize member of the
>>>>> struct memcntl_mha argument must be a valid page size (a valid page
>>>>> size
>>>>> if one of the sizes returned by the syscall getpagesizes).
>>>>>
>>>>> One variant of the function os::commit_memory has an alignment_hint
>>>>> parameter and the code in os::Solaris::commit_memory_impl sometimes
>>>>> passes this alignment_hint as the mha_pagesize to memcntl. The 
>>>>> issue is
>>>>> that there is no check that this alignment_hint actually is a valid
>>>>> page
>>>>> size.
>>>>>
>>>>
>>>> Where does this wrong alignment come from? Only from the user via
>>>> -XX:LargePageAlignmentInBytes?
>>>
>>> No, the alignment hint comes from
>>> CollectorPolicy::generation_alignment(), which is passed to the 
>>> parallel
>>> scavenge code as the generation alignment when setting up the heap.
>>>
>>> On 2014-09-11 17:23, Thomas Schatzl wrote:
>>>> If so, wouldn't it be better to make this adjustment during argument
>>>> processing? Then the VM could inform the user about the change to some
>>>> degree, either per message, or by overriding the given value
>>>> (using FLAG_SET_xxx).
>>>>
>>>> The alignment_hint should still be checked for validity when it is
>>>> used, but I think if the method gets a wrong value for the hint from
>>>> anywhere other than the user, it imo should be an error. Not silently
>>>> adjusted which potentially leads to hard to find performance problems.
>>>
>>> As we discussed on IM, it seems like the alignment_hint is meant to be
>>> used like a hint, and then the OS specific code can decide the page 
>>> size
>>> that is most suitable. In os_linux.cpp for example, the alignment_hint
>>> is only used for checking if the caller might want to use large pages
>>> (not the actual page size). BSD and Windows ignores the hint.
>>>
>>> In the Solaris code today, the page size is adjusted if the
>>> alignment_hint is larger than the largest page size, otherwise the
>>> alignment_hint is treated an exact page size.
>>>
>>> I've added a comment to the code explaining that memcntl requires an
>>> exact page.
>>>
>>> Please see new webrevs:
>>> - full: http://cr.openjdk.java.net/~ehelin/8049536/webrev.01/
>>> - incremental: http://cr.openjdk.java.net/~ehelin/8049536/webrev.00-01/
>>>
>>> On 2014-09-11 17:23, Thomas Schatzl wrote:
>>>>> Testing:
>>>>> - JPRT
>>>>> - Tested with the patch for
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8027915
>>>>>     which immediately provokes the bug on Solaris SPARC when just
>>>>> running
>>>>>     java -version. Now it works fine.
>>>>
>>>> Can you add a test case that checks there is no "MPSS failed" message
>>>> with
>>>> various LargePageAlignmentInBytes values?
>>>
>>> Sure, although I no longer managed to reproduce the bug with only
>>> java -Xms32m -Xmx128m -XX:LargePageSizeInBytes=256m -version
>>>
>>> Let me see if I can come up with some flags that provokes the bug even
>>> without my fix for JDK-8027915.
>>>
>>> Thanks,
>>> Erik
>>>
>>>> Thanks,
>>>>    Thomas
>>>>
>>>>