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

[Erik Helin]

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