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

[Erik Helin]

Thanks StefanK!

Erik

On 2014-09-12 15:18, Stefan Karlsson wrote:
> 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
>>>>>
>>>>>
>