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

[Erik Helin]

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/

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
>>>
>>>