RFR(M): 8171181: Supporting heap allocation on alternative memory devices

[David Holmes]

On 1/03/2017 12:49 PM, Kharbas, Kishor wrote:
> Hi David!
> Thanks a lot for the review, I will work on your comments and from others. Will provide an update soon!
>
> I only have a few clarifications -
>
> 1. On you comment
> 	> In os::create_file_for_heap:
> 	>   - the warning should include the reason for the failure. Though it is unclear
> 	> whether this should be a warning or a fatal error.
>     My thinking was to let the calling function in VirtualSpace.cpp handle the correct action. And it does terminate the application.

Okay - then you don't need the warning as well. The caller should give a 
detailed error message.

> 2. > In os::map_memory_to_file:
>      >   - report reasons for failure explicitly
>
>      Do you mean the message in "vm_exit_during_initialization(err_msg("Error in mapping Java heap at the given filesystem directory"));" should be more explicit?

I mean including the actual error reason, strerror(errno) for example, 
in the error messages.

Thanks,
David
-----

> Thanks
> Kishor
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Monday, February 27, 2017 5:41 PM
>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(M): 8171181: Supporting heap allocation on alternative
>> memory devices
>>
>> Hi Kishor,
>>
>> I took a quick look at this and have a few initial comments ... but overall it
>> looks pretty straight-forward.
>>
>> On 28/02/2017 9:52 AM, Kharbas, Kishor wrote:
>>> Hello all!
>>> I have proposed a JEP to support allocation of Java heap on new memory
>> devices such as NV-DIMMs like Intel's 3D XPoint.
>>> Such a feature has applications in multi-JVM deployments, big data
>> applications and in-memory databases.
>>> Please find the details here -
>>> https://bugs.openjdk.java.net/browse/JDK-8171181 and the patch for the
>>> implementation is at
>>> http://cr.openjdk.java.net/~kkharbas/8153327_webrev.04
>>> I would appreciate a review and feedback from the community for this JEP.
>>
>> Is there something linux specific about the implementation? If not it should
>> be in os_posix.cpp and be usable by all the POSIX compliant OS.
>>
>> In os::create_file_for_heap:
>>
>>   - you need to use the per-thread signal masking function, pthread_sigmask,
>> not sigprocmask. Also you need restore the signal mask before the error
>> return of -1
>>   - you can't use umask as that might impact files concurrently created by
>> other code in the VM. I expect you will need to create it then fchmod it.
>>   - the warning should include the reason for the failure. Though it is unclear
>> whether this should be a warning or a fatal error.
>>   - check returns codes from library calls - add assertions that they succeed if
>> they should be "impossible" to fail. Though I think perhaps an unlink failure
>> may need to be anticipated.
>>
>> In os::map_memory_to_file:
>>   - report reasons for failure explicitly
>>   - do not assign posix_fallocate to errno!
>>
>> The err_msg function is used for printf-style formatting, so you don't need it
>> for simple string messages. But in some cases your messages should also
>> include strerror output when there are failures.
>>
>> Some tests will also be needed of course.
>>
>> Thanks,
>> David
>>
>>> Thank you!
>>> Kishor
>>>