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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Kharbas, Kishor
Hello all!


I have an updated patch for https://bugs.openjdk.java.net/browse/JDK-8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
I have lost the old email chain so had to start a fresh one. The archived conversation can be found at - http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-March/022733.html


1.       I have worked on all the comments and fixed the bugs. Mainly bugs fixed are related to sigprocmask() and changed the implementation such that 'fd' is not passed all the way down the call stack. Thus minimizing function signature changes.


2.       Patch supports all OS'es. Consolidated all Posix compliant OS's implementation in os_posix.cpp.



3.       The patch is tested on Windows and Linux. Working on testing it on other OS'es.


Let me know if this version looks clean and correct.

Thanks
Kishor
Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
Hi Kishor,

  initial review using the webrevs, and the comment thread in hotspot-
runtime-dev (readded to cc because questions were left unanswered
there).

The code also touches runtime code quite a bit, so I would appreciate
comments by the runtime team.

On Thu, 2017-10-19 at 15:00 +0000, Kharbas, Kishor wrote:
> Hi Sangheon,
>  
> Thanks for the review and comments.
> I created two webrevs:
> webrev.07 : Is the rebase of webrev.06 on jdk10    (http://cr.openjdk
> .java.net/~kkharbas/8171181/webrev.07/)
> webrev.08 : Is incremental webrev over 07.              (http://cr.op
> enjdk.java.net/~kkharbas/8171181/webrev.08/)
>  

Some first comments on the webrev follow:

 - could you please provide both a full and incremental webrev for your
changes? The former help the reviewers late to the party, the
incremental ones the people already working with you.

 - the patch has not been rebased to jdk10/hs as Sangheon suggested.
Probably you rebased to jdk10/jdk10?

os_posix.cpp:

 - os::create_file_for_heap: the malloc allocates one byte too little,
creating an automatic write outside the buffer at the strcat() method.

 - os::create_file_for_heap does not use the size parameter. Please
remove.

 - in several places: improve the output of the error messages to have
meaning for the user. Not just "malloc failed"; there were already some
suggestions in the referenced thread at  http://mail.openjdk.java.net/p
ipermail/hotspot-runtime-dev/2017-March/022733.html .

Same for the asserts, and particularly for them - they will implicitly
terminate the VM, so e.g. a "fchmod error" is definitely too little. At
least error no/output of os::strerror() etc. would be required for
quick diagnosis.

I read in some of the emails that you intend to let the caller print a
good error message. I fear that the caller might not have the necessary
information any more to do that in a useful way.

 - in that same thread there has also been the question about the need
for blocking the signals for the process that has gone unanswered.

 - in that same thread there has also been the question about why the
code sets permissions to 0600 manually; this is because of glibc 2.0.6
compatibility.
Glibc 2.06 has been released 1997-12-29. I looked a bit through glibc
requirements for the Oracle JDK, and while I could not find exact
requirements, some posts indicate the need for GLIBC_2.4 at minimum
(for jdk7) which is from 2003. I recommend removing this, it seems
unnecessary and completely outdated.

 - os::malloc should be paired with os::free in
os::create_file_for_heap() (2x)

 - I am not sure whether the methods that deal with mapping memory to a
file should have "dax" in their name. The code seems to be pretty
generically map memory into a file.

 - I am not sure if the current approach to FS errors after mkstemp is
very nice. Please at least print a meaningful warning message to the
log.

 - please also specify the meaning of the return value for
os::create_file_for_heap() in the documentation.

 - there is a missing "p" in the name of os::reserve_mmaped_memory().

 - os::util_posix_fallocate(): s/continous/continuous

 - os::map_memory_to_dax_file(): according to man pages,
posix_fallocate does not set errno, but the code checking its return
(or util_posix_fallocate()) expects it to do so.

 - os::attempt_reserve_memory_at(), in the call to
pd_attempt_reserve_memory_at() for AIX, in the third parameter, please
use the exact name of the parameter in the comment.

 - os::attempt_reserve_memory_at(), the second "if" needs a blank
before the bracket.

 - os::attempt_reserve_memory_at(), in the comment, s/mmemory/memory

 - in that same method, the #if..#endif block should be aligned like
other similar blocks.

 - in that same method, the "else" should be on the same line as the
closing bracket of the if-body.

os_windows.cpp:

os::create_file_for_heap():
 - same bug about length of allocation the _alloca call. Not completely
sure about why use alloca (or not use alloca in os_posix.cpp).

 - this version calls os::native_path(). It seems strange to not do
that as well in the others (although it does nothing).

 - os::reserve_memory_aligned(): an "else" should be moved into the
same line as the closing bracket.

universe.cpp:

 - Universe::reserve_heap(): I do not think this functionality has
anything to do with compressed oops, so the log message should not have
the coops tag.
The log message could/should have more information about the file
location. I also recommend using "Java heap" instead of "Heap" here, as
the latter is ambiguous.

virtualspace.cpp:

 - in failed_to_reserve_as_requested: "else" indentation. Also, the
"else { if (..." could be shortened to "} else if (..." to avoid the
extra indentation level. (2x)

 - ReservedSpace::initialize(): s/upto/up to

 - ReservedSpace::initialize(): remove the coops tag here as well.
Also, the info message might be more similar to the comment above where
it says that large page support is up to the file system, and the flag
ignored.

 - the code contains the snippet

if (_fd_for_heap != -1) {
  if (!os::unmap_memory...
} else {
  if (!os::release_memory...
}

at least twice. Maybe this code snippet could be extracted into a new
method.

 - (ignore if you want) ReservedHeapSpace::ReservedHeapSpace, at the
end - maybe the condition is easier to read if it looks like:

if (_fd_for_heap != -1) {
  os::close(_fd_for_heap);
}

than it is now.

 - virtualspace.hpp: in the changed constructor signature, please add a
comment about what backing_fs_for_heap actually means (and what happens
if set).

arguments.cpp:

 - Arguments::finalize_vm_init_args: maybe at the place where the
change adds the warning/info message about NUMA support being specific
to the file system; also add the same warning about UseLargePages that
is located elsewhere.

Like "UseXXXX may not be supported in some specific file system
implementations." - or just ignore as Andrew said in the other email
thread.

Note that I am not sure that the selected place is the correct place
for such warning about incompatible flags (and maybe
UseNUMA/UseLargePages should be forcibly disabled here? But then again,
it does not hurt just having it enabled?).

I will let the runtime team comment as a lot of that argument
processing changed in jdk9.

Maybe Arguments::check_vm_args_consistency() is better.

There is similar code in Arguments::adjust_after_os()...

os.cpp:

 - os::reserve_memory: "else" indentation

 - os::reserve_memory: I do not follow that comment - it explains the
code as written, not why, and what map_memory_to_dax_file() has to do
with AIX and SHM. It uses an outdated method name, and also s/your/our.

Probably the whole comment can be removed.

os.hpp:

 - indentation of the new #if defined clause

 - here I may probably be speaking wrongly on behalf of the runtime
team, but os.hpp, as an abstraction of the OS should probably not have
platform specific ifdefs in it.

Thanks,
  Thomas

> In webrev08 I have addressed all the comments, except for ones below:
>  
> ---------------------------------------
> src/os/aix/vm/os_aix.cpp
>
> 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> requested_addr, bool use_SHM) {
> - Question. Why os_aix has additional parameter at
> pd_attemp_reserve_memory_at()? Probably only AIX has shmated memory
> implementation?
>          A: Yes that’s correct.
>  
> --------------------------------------
> 137       log_debug(gc, heap, coops)("UseLargePages can't be set with
> HeapDir option.");
> - Is it enough to print log message instead of warning message? i.e.
> Don't we need something at arguments.cpp:3656, similar to NUMA flags?
>        A: If don’t disable UseLargePages like UseNUMA because large
> pages can be used for other allocation such as CodeCache.
>  
> ------------------------------------
>  
> 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> alignment, bool large, const char* backing_fs_for_heap)
> - ReservedSpace has '_backing_fd' but the constructor doesn't take it
> as a parameter and only ReservedHeapSpace uses it. This seems not
> ideal, couldn't make it better? I know actual logic is at
> ReservedSpace so it is not convenient to add _backing_fs_for_heap at
> ReservedHeapSapce.
>       A: ReservedHeapSpace depends on few functions in ReservedSpace
> such as initialize(), release(). So instead of passing it as
> argument, I set it as a propert of ReservedSpace.
>  
> -----------------------------------
> 1663
> - You removed os::attempt_reserve_memory_at() from os.cpp and split
> into os_posix.cpp and os_windows.cpp.
>   But I think you should remain os::attempt_reserve_memory_at() at
> os.cpp and implement os specific stuffs at each os_xxx.cpp files for
> pd_xxx. Of cource move MemTracker function call as well.
>         A: I do it this way to reduce the redundant code, If I
> implement in OS specific files in pd_xxx(), the code to replace
> reserved mapping with file mapping
> (replace_existing_mapping_with_dax_file_mapping()) will be redundant.
>              Still if you feel I will do the change and see how it
> looks.
>  
> Regards
> Kishor
>  
>  
> From: sangheon.kim [mailto:[hidden email]
> Sent: Tuesday, September 26, 2017 3:18 PM
> To: Kharbas, Kishor <[hidden email]>; 'hotspot-gc-dev@openj
> dk.java.net' <[hidden email]>
> Subject: Re: RFR(M): 8171181: Supporting heap allocation on
> alternative memory devices
>  
> Hi Kishor,
>
> On 07/20/2017 06:34 PM, Kharbas, Kishor wrote:
> I have a new version of this patch at http://cr.openjdk.java.net/~kkh
> arbas/8171181/webrev.06/
>  
> This version has been tested on Windows, Linux, Solaris and Mac OS. I
> could not get access to AIX for testing.
> I used tmpfs to test the functionality. Cases that were tested were.
> 1.       Allocation of heap using file mapping when –XX:HeapDir=
> option is used.
> 2.       Creation of nameless temporary file for Heap allocation
> which prevents access to file using its name.
> 3.       Correct deletion and freeing up of space allocated for file
> under different exit conditions.
> 4.       Error handling when path specified is not present, heap size
> is more than size of file system, etc.
> Overall seems good.
> I tried to list some missing part.
>
> 1. Please rebase with consolidated repository. (jdk10/hs)
> 2. Build failure on Solaris.
>     (Sorry no build error log file, as I tested a few weeks ago, it
> is deleted)
> 3. Have you tested with various heap reserve path using
> HeapBaseMinAddress flag? i.e. to test code path of
> ReservedHeapSpace::try_reserve_heap() and try_reserve_range().
> 4. How about adding HeapDir allocation success message? e.g.
> gc+heap+coops=info
> 5. Adding JTREG test. Probably we would need to verify this
> allocation is succeeded via #4 added allocation success message.
> 6. CSR (Compatibility & Specification Review). At some point, you
> need to file another CR of 'CSR' type to add a new flag of 'HeapDir'.
> 7. It will be much appreciated if you provide incremental webrev. I
> think 06(this version) vs. 07(rebase version) would be hard to get.
> Probably from 08 version.
>
> Here's my comments.
> -----------------------------
> src/os/aix/vm/os_aix.cpp
>
> 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> requested_addr, bool use_SHM) {
> - Question. Why os_aix has additional parameter at
> pd_attemp_reserve_memory_at()? Probably only AIX has shmated memory
> implementation?
>
> -----------------------------
> src/os/posix/vm/os_posix.cpp
>
> 148   char *fullname = (char*)::malloc(strlen(dir) +
> sizeof(name_template));
> - Use os::malloc()
>
> 196   int flags;
> 197 
> 198   flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;
> - Combining 196 and 198 seems better to me.
>
> 200     assert((uintptr_t)requested_addr % os::Linux::page_size() ==
> 0, "unaligned address");
> - Linux dependency on posix file which makes build error on Solaris.
> Probably os::vm_page_size().
>
> 207   addr = (char*)::mmap(requested_addr, bytes, PROT_NONE,
> 208     flags, -1, 0);
> - Missing some spaces? Alignment seems odd to me.
>
> 226     if (ret == -1)
> - Probably you wanted to add handling code? If not, just return ret.
>
> 252   if (addr == MAP_FAILED || (base != NULL && addr != base)) {
> 253     if (addr != MAP_FAILED) {
> 254       if (!os::release_memory(addr, size)) {
> 255         warning("Could not release memory on unsuccessful file
> mapping");
> 256       }
> 257     }
> 258     return NULL;
> 259   }
> - Splitting MAP_FAILED case and another gives better readability to
> me. But this is your call.
>
> 269 
> - Extra line.
>
> 284   if (result != NULL && file_desc != -1) {
> 285     if (replace_existing_mapping_with_dax_file_mapping(result,
> bytes, file_desc) == NULL) {
> 286       vm_exit_during_initialization(err_msg("Error in mapping
> Java heap at the given filesystem directory"));
> 287     }
> 288    
> MemTracker::record_virtual_memory_reserve_and_commit((address)result,
> bytes, CALLER_PC);
> 289     return result;
> 290   }
> 291   if (result != NULL) {
> 292     MemTracker::record_virtual_memory_reserve((address)result,
> bytes, CALLER_PC);
> 293   }
> - Combining line 284 and 291 seems better to me.
> 284   if (result != NULL) {
>         if (file_desc != -1) {
>           if (replace_existing_mapping_with_dax_file_mapping(result,
> bytes, file_desc) == NULL) {
>             vm_exit_during_initialization(err_msg("Error in mapping
> Java heap at the given filesystem directory"));
>           }
>          
> MemTracker::record_virtual_memory_reserve_and_commit((address)result,
> bytes, CALLER_PC);
>         } else {
>           MemTracker::record_virtual_memory_reserve((address)result,
> bytes, CALLER_PC);
>         }
>       }
>       return result;
>
> -----------------------------
> src/os/windows/vm/os_windows.cpp
> 3141 // if 'base' is not NULL, function will return NULL if it cannot
> get 'base'
> - Start with uppercase.
>
> 3142 //
> - This line seems redundant.
>
> 3151       vm_exit_during_initialization(err_msg("Could not allocate
> sufficient disk space for heap"));
> - heap -> Java heap (same as line 3153)
>
> 3168   assert(base != NULL, "base cannot be NULL");
> - 'base' -> 'Base' or 'Base address'.
>
> 3172 
> - Redundant line.
>
> 3230     }
> 3231     else {
> -> } else {
>
> 3278   return reserve_memory(bytes, requested_addr, 0);
> - Is it correct to use '0' not '-1'? It would be better to explain
> why we use hard-coded value here.
>
> -----------------------------
> src/share/vm/memory/universe.cpp
> - No comments
>
> -----------------------------
> src/share/vm/memory/virtualspace.cpp
> - copyright update
>
> 74                                            const size_t size, bool
> special, bool is_file_mapped= false)
> - Need space between 'is_file_mapped' and '='.
>
> 92           fatal("os::release_memory failed");
> - Typo, 'os::unmap_memory failed'.
>
> 129   // If there is a backing file directory for this VirtualSpace
> then whether
> - This is not VirtualSpace. Probably just 'space'.
>
> 130   // large pages are allocated is upto the filesystem the dir
> resides in.
> - 'dir'? Probably 'backing file for Java heap'.
>
> 137       log_debug(gc, heap, coops)("UseLargePages can't be set with
> HeapDir option.");
> - Is it enough to print log message instead of warning message? i.e.
> Don't we need something at arguments.cpp:3656, similar to NUMA flags?
>
> 191         // unmap_memory will do extra work esp. in Windows
> - esp. -> especially
>
> 282       }
> 283       else {
> -> } else {
>
> 346   // If there is a backing file directory for this VirtualSpace
> then whether
> - Again this is not VirtualSpace, so just 'space'.
>
> 352     if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||
> 353       !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {
> - Wrong alignment at line 353. Consider to make same as line 380.
>
> 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> alignment, bool large, const char* backing_fs_for_heap)
> - ReservedSpace has '_backing_fd' but the constructor doesn't take it
> as a parameter and only ReservedHeapSpace uses it. This seems not
> ideal, couldn't make it better? I know actual logic is at
> ReservedSpace so it is not convenient to add _backing_fs_for_heap at
> ReservedHeapSapce.
>
> -----------------------------
> src/share/vm/memory/virtualspace.hpp
> 40   int    _backing_fd;
> - I would prefer to have better name to describe. 
>   e.g. as command-line option name is 'HeapDir', _heap_fd or
> _fd_for_heap(similar to below)?
>
> 115   ReservedHeapSpace(size_t size, size_t forced_base_alignment,
> bool large, const char* backingFSforHeap = NULL);
> - Snake case. How about 'fs_for_heap' or 'heap_fs'?
>
> -----------------------------
> src/share/vm/runtime/arguments.cpp
> 3655     FLAG_SET_CMDLINE(bool, UseNUMA, false);
> - (questions) Don't need to add a warning message for
> UseLargePagesSame=true as commented virtualspace.cpp:137?
>
> -----------------------------
> src/share/vm/runtime/globals.hpp
> - No comments
>
> -----------------------------
> src/share/vm/runtime/os.cpp
>
> 1632 
> - Extra line.
>
> 1642   }
> 1643   else {
> -> } else {
>
> 1663
> - You removed os::attempt_reserve_memory_at() from os.cpp and split
> into os_posix.cpp and os_windows.cpp.
>   But I think you should remain os::attempt_reserve_memory_at() at
> os.cpp and implement os specific stuffs at each os_xxx.cpp files for
> pd_xxx. Of cource move MemTracker function call as well.
>
> -----------------------------
> src/share/vm/runtime/os.hpp
>
> 349   // replace existing reserved memory with file mapping
> - Start with uppercase letter.
>
> Thanks,
> Sangheon
>
>
>
>  
> - Kishor
>  
> From: Kharbas, Kishor 
> Sent: Tuesday, July 11, 2017 6:40 PM
> To: '[hidden email]' <[hidden email]
> t>
> Cc: Kharbas, Kishor <[hidden email]>
> Subject: RFR(M): 8171181: Supporting heap allocation on alternative
> memory devices
>  
> Greetings,
>  
> I have an updated patch for JEP https://bugs.openjdk.java.net/browse/
> JDK-8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> This patch fixes the bugs pointed earlier and other suggestions to
> make the code less intrusive.
>  
> I have also sent this to ‘hotspot-runtime-dev’ mailing list (included
> below).
>  
> I would appreciate comments and feedback.
>  
> Thanks
> Kishor
>  
> From: Kharbas, Kishor 
> Sent: Monday, July 10, 2017 1:53 PM
> To: [hidden email]
> Cc: Kharbas, Kishor <[hidden email]>
> Subject: RFR(M): 8171181: Supporting heap allocation on alternative
> memory devices
>  
> Hello all!
>  
> I have an updated patch for https://bugs.openjdk.java.net/browse/JDK-
> 8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> I have lost the old email chain so had to start a fresh one. The
> archived conversation can be found at - http://mail.openjdk.java.net/
> pipermail/hotspot-runtime-dev/2017-March/022733.html
>  
> 1.       I have worked on all the comments and fixed the bugs. Mainly
> bugs fixed are related to sigprocmask() and changed the
> implementation such that ‘fd’ is not passed all the way down the call
> stack. Thus minimizing function signature changes.
>  
> 2.       Patch supports all OS’es. Consolidated all Posix compliant
> OS’s implementation in os_posix.cpp.
>  
> 3.       The patch is tested on Windows and Linux. Working on testing
> it on other OS’es.
>  
> Let me know if this version looks clean and correct.
>  
> Thanks
> Kishor
>  

Reply | Threaded
Open this post in threaded view
|

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

Kharbas, Kishor
Hi Sangheon, Thomas,

Thanks for the review. Here is the latest webrev based on your comments and my replies to both are inline (I copied Sangheon's email here at the top above Thomas's reply)
http://cr.openjdk.java.net/~kkharbas/8171181/webrev.10_to_09
http://cr.openjdk.java.net/~kkharbas/8171181/webrev.10

Please let me know your question/comments.

Thanks
Kishor

>Hi Kishor,
>On 10/19/2017 08:00 AM, Kharbas, Kishor wrote:
>Hi Sangheon,
 
>Thanks for the review and comments.
>I created two webrevs:
>webrev.07 : Is the rebase of webrev.06 on jdk10    (http://cr.openjdk.java.net/~kkharbas/8171181/webrev.07/)
>webrev.08 : Is incremental webrev over 07.              (http://cr.openjdk.java.net/~kkharbas/8171181/webrev.08/)
>1. I couldn't apply webrev.07 patch cleanly. Could you test?
>    Just minor nit: 07 seems not only rebase of webrev.06. i.e. there seems to have some changes from my comment of alignments.
[Kharbas, Kishor] My bad, I had rebased the patch on jdk10/jdk10. The further ones are on jdk10/hs

>2. The description in the JEP uses 'AllocateHeapAt' while the patch uses 'HeapDir'.
[Kharbas, Kishor] I changed the code. Thanks

>-----------------------------
>src/os/posix/vm/os_posix.cpp
>159   (void)strcpy(fullname, dir);
>- Please use strncpy instead.

[Kharbas, Kishor] Done

>160   (void)strcat(fullname, name_template);
>- Please use strncat instead.

[Kharbas, Kishor] Done.

>180     ::free(fullname);
>- os::free().
[Kharbas, Kishor] Done.

>196   ::free(fullname);
>- os::free().
[Kharbas, Kishor] Done.

>-----------------------------
>src/os/windows/vm/os_windows.cpp
>2885   char *fullname = (char*)_alloca(strlen(dir) + sizeof(name_template));
>- Missing allocation failure handling.
>- Please use _malloca instead.
>  * This function is deprecated because a more secure version is available; see _malloca.
>    https://msdn.microsoft.com/en-us/library/wb1s57t5.aspx

[Kharbas, Kishor] Done.

>2886   (void)strcpy(fullname, dir);
>- Please use strncpy instead.
[Kharbas, Kishor] Done.

>2887   (void)strcat(fullname, name_template);
>- Please use strncat instead.
[Kharbas, Kishor] Done.

>-----------------------------
>src/share/vm/runtime/arguments.cpp
>3726     }

>3727   }
>- My previous comment was to add explicit explanation of disabling UseNUMA and UseNUMAInterleaving.

>4636     if(!FLAG_IS_DEFAULT(HeapDir)) {
>- Previsouly we checked UseNUMA and UseNUMAInterleaving and then disabled those. IMHO, I think previous one seems better with more explanation that we are going to disable those options. i.e. >Similar to os_linux.cpp:4869, a warning message with disabling codes.

[Kharbas, Kishor] Based on our discussion offline, I think we agree that it’s OK to disable UseNUMA and leave UseNUMAInterleaving ON since other areas of JVM such as CodeCache can use UseNUMAInterleaving.

>4638     }
>4639     else if (UseParallelGC || UseParallelOldGC) {
>- One line please if this change is still needed.
  -> } else if {

>-----------------------------
>src/share/vm/runtime/os.cpp
>1678
>- Extra line.
[Kharbas, Kishor] Done.

>1688   }
>1689   else {
>-> } else {

[Kharbas, Kishor] Done.

>My answers are inlined.
 
>In webrev08 I have addressed all the comments, except for ones below:
 
>---------------------------------------
>src/os/aix/vm/os_aix.cpp

>2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char* requested_addr, bool use_SHM) {
>- Question. Why os_aix has additional parameter at pd_attemp_reserve_memory_at()? Probably only AIX has shmated memory implementation?
>         A: Yes that’s correct.
>Okay.
>--------------------------------------
>137       log_debug(gc, heap, coops)("UseLargePages can't be set with HeapDir option.");
>- Is it enough to print log message instead of warning message? i.e. Don't we need something at arguments.cpp:3656, similar to NUMA flags?
>       A: If don’t disable UseLargePages like UseNUMA because large pages can be used for other allocation such as CodeCache.
>I meant to changing log_debug() to log_warning().
>If UseNUMA is enabled, we print warning at arguments.cpp:3725 while UseLargePages is enabled, we print debug message.
>In addition, is this enough? Don't we need to force disabling UseLargePages?
>What happens if both options are enabled?
>------------------------------------
>603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t alignment, bool large, const char* backing_fs_for_heap)
>- ReservedSpace has '_backing_fd' but the constructor doesn't take it as a parameter and only ReservedHeapSpace uses it. This seems not ideal, couldn't make it better? I know actual logic is at >ReservedSpace so it is not convenient to add _backing_fs_for_heap at ReservedHeapSapce.
>      A: ReservedHeapSpace depends on few functions in ReservedSpace such as initialize(), release(). So instead of passing it as argument, I set it as a propert of ReservedSpace.
>Okay.
-----------------------------------
>1663
>- You removed os::attempt_reserve_memory_at() from os.cpp and split into os_posix.cpp and os_windows.cpp.
>  But I think you should remain os::attempt_reserve_memory_at() at os.cpp and implement os specific stuffs at each os_xxx.cpp files for pd_xxx. Of cource move MemTracker function call as well.
>        A: I do it this way to reduce the redundant code, If I implement in OS specific files in pd_xxx(), the code to replace reserved mapping with file mapping >(replace_existing_mapping_with_dax_file_mapping()) will be redundant.
>             Still if you feel I will do the change and see how it looks.
>I would prefer to have pd_xxx() even though there's some redundant code.

[Kharbas, Kishor] We also had a discussion about this offline, I will address this in the next webrev.

>Thanks,
>Sangheon

> -----Original Message-----
> From: Thomas Schatzl [mailto:[hidden email]]
> Sent: Tuesday, October 24, 2017 8:39 AM
> To: Kharbas, Kishor <[hidden email]>; sangheon.kim
> <[hidden email]>; '[hidden email]'
> <[hidden email]>
> Cc: [hidden email]
> Subject: Re: RFR(M): 8171181: Supporting heap allocation on alternative
> memory devices
>
> Hi Kishor,
>
>   initial review using the webrevs, and the comment thread in hotspot-
> runtime-dev (readded to cc because questions were left unanswered there).
>
> The code also touches runtime code quite a bit, so I would appreciate
> comments by the runtime team.
>
> On Thu, 2017-10-19 at 15:00 +0000, Kharbas, Kishor wrote:
> > Hi Sangheon,
> >
> > Thanks for the review and comments.
> > I created two webrevs:
> > webrev.07 : Is the rebase of webrev.06 on jdk10    (http://cr.openjdk
> > .java.net/~kkharbas/8171181/webrev.07/)
> > webrev.08 : Is incremental webrev over 07.              (http://cr.op
> > enjdk.java.net/~kkharbas/8171181/webrev.08/)
> >
>
> Some first comments on the webrev follow:
>
>  - could you please provide both a full and incremental webrev for your
> changes? The former help the reviewers late to the party, the incremental
> ones the people already working with you.
>
[Kharbas, Kishor] Yes. I will keep this in mind.

>  - the patch has not been rebased to jdk10/hs as Sangheon suggested.
> Probably you rebased to jdk10/jdk10?
>
[Kharbas, Kishor] My bad, it was rebased to jdk10/jdk10. New webrev's will be based on jdk10/hs.

> os_posix.cpp:
>
>  - os::create_file_for_heap: the malloc allocates one byte too little, creating
> an automatic write outside the buffer at the strcat() method.
>
[Kharbas, Kishor] Done.

>  - os::create_file_for_heap does not use the size parameter. Please remove.
>
[Kharbas, Kishor] Done.

>  - in several places: improve the output of the error messages to have
> meaning for the user. Not just "malloc failed"; there were already some
> suggestions in the referenced thread at  http://mail.openjdk.java.net/p
> ipermail/hotspot-runtime-dev/2017-March/022733.html .
>
> Same for the asserts, and particularly for them - they will implicitly terminate
> the VM, so e.g. a "fchmod error" is definitely too little. At least error
> no/output of os::strerror() etc. would be required for quick diagnosis.
>
> I read in some of the emails that you intend to let the caller print a good error
> message. I fear that the caller might not have the necessary information any
> more to do that in a useful way.
>
[Kharbas, Kishor] Done. Moved macro assert_with_errror() to top of file so I can use in my code.

>  - in that same thread there has also been the question about the need for
> blocking the signals for the process that has gone unanswered.
>
[Kharbas, Kishor] Let me get back on this. The implementation at pmem.io which I use as reference for file operations does blocks signals. I will check if there is more to it than just a 'good practice'.

>  - in that same thread there has also been the question about why the code
> sets permissions to 0600 manually; this is because of glibc 2.0.6 compatibility.
> Glibc 2.06 has been released 1997-12-29. I looked a bit through glibc
> requirements for the Oracle JDK, and while I could not find exact
> requirements, some posts indicate the need for GLIBC_2.4 at minimum (for
> jdk7) which is from 2003. I recommend removing this, it seems unnecessary
> and completely outdated.
>
[Kharbas, Kishor] Agree. Removed it.

>  - os::malloc should be paired with os::free in
> os::create_file_for_heap() (2x)
>
[Kharbas, Kishor] Done.
 
>  - I am not sure whether the methods that deal with mapping memory to a
> file should have "dax" in their name. The code seems to be pretty generically
> map memory into a file.
>
[Kharbas, Kishor] I used 'dax' in the name because to get direct mapping to DAX device you need to use 'MAP_SHARED'. That makes this map function different than generic map function. But it’s a minor point. I can change the name if it's still desired./mkstem/

>  - I am not sure if the current approach to FS errors after mkstemp is very
> nice. Please at least print a meaningful warning message to the log.
>
[Kharbas, Kishor] Added a warning() message when mkstemp could not create a file.

>  - please also specify the meaning of the return value for
> os::create_file_for_heap() in the documentation.
>
[Kharbas, Kishor] Added more information in the comment before function's declaration.

>  - there is a missing "p" in the name of os::reserve_mmaped_memory().
>
[Kharbas, Kishor] Done.

>  - os::util_posix_fallocate(): s/continous/continuous
>
[Kharbas, Kishor] Done.

>  - os::map_memory_to_dax_file(): according to man pages, posix_fallocate
> does not set errno, but the code checking its return (or
> util_posix_fallocate()) expects it to do so.
>
[Kharbas, Kishor] I removed printing of error string. Can the return error value of posix_fallocate() we used as argument to os::strerror(errno)?

>  - os::attempt_reserve_memory_at(), in the call to
> pd_attempt_reserve_memory_at() for AIX, in the third parameter, please
> use the exact name of the parameter in the comment.
>
[Kharbas, Kishor] Done.

>  - os::attempt_reserve_memory_at(), the second "if" needs a blank before
> the bracket.
>
[Kharbas, Kishor] Done.

>  - os::attempt_reserve_memory_at(), in the comment,
> s/mmemory/memory
>
[Kharbas, Kishor] Done.

>  - in that same method, the #if..#endif block should be aligned like other
> similar blocks.
>
[Kharbas, Kishor] Done.

>  - in that same method, the "else" should be on the same line as the closing
> bracket of the if-body.
>
[Kharbas, Kishor] Done.

> os_windows.cpp:
>
> os::create_file_for_heap():
>  - same bug about length of allocation the _alloca call. Not completely sure
> about why use alloca (or not use alloca in os_posix.cpp).
>
[Kharbas, Kishor] Changed to have uniform implementation.

>  - this version calls os::native_path(). It seems strange to not do that as well
> in the others (although it does nothing).
>
[Kharbas, Kishor] Added in os_posix.cpp to make it uniform.

>  - os::reserve_memory_aligned(): an "else" should be moved into the same
> line as the closing bracket.
>
[Kharbas, Kishor] Done.

> universe.cpp:
>
>  - Universe::reserve_heap(): I do not think this functionality has anything to
> do with compressed oops, so the log message should not have the coops tag.
> The log message could/should have more information about the file location.
> I also recommend using "Java heap" instead of "Heap" here, as the latter is
> ambiguous.
>
[Kharbas, Kishor] Done. Added more info to log message.

> virtualspace.cpp:
>
>  - in failed_to_reserve_as_requested: "else" indentation. Also, the "else { if
> (..." could be shortened to "} else if (..." to avoid the extra indentation level.
> (2x)
>
[Kharbas, Kishor] Done.

>  - ReservedSpace::initialize(): s/upto/up to
>
[Kharbas, Kishor] Done.

>  - ReservedSpace::initialize(): remove the coops tag here as well.
> Also, the info message might be more similar to the comment above where it
> says that large page support is up to the file system, and the flag ignored.
>
[Kharbas, Kishor] Done.

>  - the code contains the snippet
>
> if (_fd_for_heap != -1) {
>   if (!os::unmap_memory...
> } else {
>   if (!os::release_memory...
> }
>
> at least twice. Maybe this code snippet could be extracted into a new
> method.
>
[Kharbas, Kishor] Defined a new static function unmap_or_release_memory()

>  - (ignore if you want) ReservedHeapSpace::ReservedHeapSpace, at the end
> - maybe the condition is easier to read if it looks like:
>
> if (_fd_for_heap != -1) {
>   os::close(_fd_for_heap);
> }
>
> than it is now.
[Kharbas, Kishor] I agree. Done.
>
>  - virtualspace.hpp: in the changed constructor signature, please add a
> comment about what backing_fs_for_heap actually means (and what
> happens if set).
>
[Kharbas, Kishor] Done.

> arguments.cpp:
>
>  - Arguments::finalize_vm_init_args: maybe at the place where the change
> adds the warning/info message about NUMA support being specific to the
> file system; also add the same warning about UseLargePages that is located
> elsewhere.
>
> Like "UseXXXX may not be supported in some specific file system
> implementations." - or just ignore as Andrew said in the other email thread.
>
> Note that I am not sure that the selected place is the correct place for such
> warning about incompatible flags (and maybe UseNUMA/UseLargePages
> should be forcibly disabled here? But then again, it does not hurt just having
> it enabled?).
>
> I will let the runtime team comment as a lot of that argument processing
> changed in jdk9.
>
> Maybe Arguments::check_vm_args_consistency() is better.
>
> There is similar code in Arguments::adjust_after_os()...
>
[Kharbas, Kishor] In progress. I will get back on these comments in next reply.

> os.cpp:
>
>  - os::reserve_memory: "else" indentation
>
>  - os::reserve_memory: I do not follow that comment - it explains the code as
> written, not why, and what map_memory_to_dax_file() has to do with AIX
> and SHM. It uses an outdated method name, and also s/your/our.
>
> Probably the whole comment can be removed.
>
[Kharbas, Kishor] I changed the comment. In the beginning I had called pd_reserve_memory() followed by replace_existing_mapping_with_dax_file_mapping(), but later realized that AIX can allocate from SHM, in which case it's not straight forward to replace the mapping. So I leave this comment to make sure in future I (or someone else) does not ponder over the same thing.

> os.hpp:
>
>  - indentation of the new #if defined clause
>
>  - here I may probably be speaking wrongly on behalf of the runtime team,
> but os.hpp, as an abstraction of the OS should probably not have platform
> specific ifdefs in it.
>
[Kharbas, Kishor] Change the indentation. I will address abstraction issue in next reply.

> Thanks,
>   Thomas
>
> > In webrev08 I have addressed all the comments, except for ones below:
> >
> > ---------------------------------------
> > src/os/aix/vm/os_aix.cpp
> >
> > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> > requested_addr, bool use_SHM) {
> > - Question. Why os_aix has additional parameter at
> > pd_attemp_reserve_memory_at()? Probably only AIX has shmated
> memory
> > implementation?
> >          A: Yes that’s correct.
> >
> > --------------------------------------
> > 137       log_debug(gc, heap, coops)("UseLargePages can't be set with
> > HeapDir option.");
> > - Is it enough to print log message instead of warning message? i.e.
> > Don't we need something at arguments.cpp:3656, similar to NUMA flags?
> >        A: If don’t disable UseLargePages like UseNUMA because large
> > pages can be used for other allocation such as CodeCache.
> >
> > ------------------------------------
> >
> > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> > alignment, bool large, const char* backing_fs_for_heap)
> > - ReservedSpace has '_backing_fd' but the constructor doesn't take it
> > as a parameter and only ReservedHeapSpace uses it. This seems not
> > ideal, couldn't make it better? I know actual logic is at
> > ReservedSpace so it is not convenient to add _backing_fs_for_heap at
> > ReservedHeapSapce.
> >       A: ReservedHeapSpace depends on few functions in ReservedSpace
> > such as initialize(), release(). So instead of passing it as argument,
> > I set it as a propert of ReservedSpace.
> >
> > -----------------------------------
> > 1663
> > - You removed os::attempt_reserve_memory_at() from os.cpp and split
> > into os_posix.cpp and os_windows.cpp.
> >   But I think you should remain os::attempt_reserve_memory_at() at
> > os.cpp and implement os specific stuffs at each os_xxx.cpp files for
> > pd_xxx. Of cource move MemTracker function call as well.
> >         A: I do it this way to reduce the redundant code, If I
> > implement in OS specific files in pd_xxx(), the code to replace
> > reserved mapping with file mapping
> > (replace_existing_mapping_with_dax_file_mapping()) will be redundant.
> >              Still if you feel I will do the change and see how it
> > looks.
> >
> > Regards
> > Kishor
> >
> >
> > From: sangheon.kim [mailto:[hidden email]]
> > Sent: Tuesday, September 26, 2017 3:18 PM
> > To: Kharbas, Kishor <[hidden email]>; 'hotspot-gc-dev@openj
> > dk.java.net' <[hidden email]>
> > Subject: Re: RFR(M): 8171181: Supporting heap allocation on
> > alternative memory devices
> >
> > Hi Kishor,
> >
> > On 07/20/2017 06:34 PM, Kharbas, Kishor wrote:
> > I have a new version of this patch at http://cr.openjdk.java.net/~kkh
> > arbas/8171181/webrev.06/
> >
> > This version has been tested on Windows, Linux, Solaris and Mac OS. I
> > could not get access to AIX for testing.
> > I used tmpfs to test the functionality. Cases that were tested were.
> > 1.       Allocation of heap using file mapping when –XX:HeapDir=
> > option is used.
> > 2.       Creation of nameless temporary file for Heap allocation which
> > prevents access to file using its name.
> > 3.       Correct deletion and freeing up of space allocated for file
> > under different exit conditions.
> > 4.       Error handling when path specified is not present, heap size
> > is more than size of file system, etc.
> > Overall seems good.
> > I tried to list some missing part.
> >
> > 1. Please rebase with consolidated repository. (jdk10/hs) 2. Build
> > failure on Solaris.
> >     (Sorry no build error log file, as I tested a few weeks ago, it is
> > deleted) 3. Have you tested with various heap reserve path using
> > HeapBaseMinAddress flag? i.e. to test code path of
> > ReservedHeapSpace::try_reserve_heap() and try_reserve_range().
> > 4. How about adding HeapDir allocation success message? e.g.
> > gc+heap+coops=info
> > 5. Adding JTREG test. Probably we would need to verify this allocation
> > is succeeded via #4 added allocation success message.
> > 6. CSR (Compatibility & Specification Review). At some point, you need
> > to file another CR of 'CSR' type to add a new flag of 'HeapDir'.
> > 7. It will be much appreciated if you provide incremental webrev. I
> > think 06(this version) vs. 07(rebase version) would be hard to get.
> > Probably from 08 version.
> >
> > Here's my comments.
> > -----------------------------
> > src/os/aix/vm/os_aix.cpp
> >
> > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char*
> > requested_addr, bool use_SHM) {
> > - Question. Why os_aix has additional parameter at
> > pd_attemp_reserve_memory_at()? Probably only AIX has shmated
> memory
> > implementation?
> >
> > -----------------------------
> > src/os/posix/vm/os_posix.cpp
> >
> > 148   char *fullname = (char*)::malloc(strlen(dir) +
> > sizeof(name_template));
> > - Use os::malloc()
> >
> > 196   int flags;
> > 197
> > 198   flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;
> > - Combining 196 and 198 seems better to me.
> >
> > 200     assert((uintptr_t)requested_addr % os::Linux::page_size() ==
> > 0, "unaligned address");
> > - Linux dependency on posix file which makes build error on Solaris.
> > Probably os::vm_page_size().
> >
> > 207   addr = (char*)::mmap(requested_addr, bytes, PROT_NONE,
> > 208     flags, -1, 0);
> > - Missing some spaces? Alignment seems odd to me.
> >
> > 226     if (ret == -1)
> > - Probably you wanted to add handling code? If not, just return ret.
> >
> > 252   if (addr == MAP_FAILED || (base != NULL && addr != base)) {
> > 253     if (addr != MAP_FAILED) {
> > 254       if (!os::release_memory(addr, size)) {
> > 255         warning("Could not release memory on unsuccessful file
> > mapping");
> > 256       }
> > 257     }
> > 258     return NULL;
> > 259   }
> > - Splitting MAP_FAILED case and another gives better readability to
> > me. But this is your call.
> >
> > 269
> > - Extra line.
> >
> > 284   if (result != NULL && file_desc != -1) {
> > 285     if (replace_existing_mapping_with_dax_file_mapping(result,
> > bytes, file_desc) == NULL) {
> > 286       vm_exit_during_initialization(err_msg("Error in mapping Java
> > heap at the given filesystem directory"));
> > 287     }
> > 288
> >
> MemTracker::record_virtual_memory_reserve_and_commit((address)result
> ,
> > bytes, CALLER_PC);
> > 289     return result;
> > 290   }
> > 291   if (result != NULL) {
> > 292     MemTracker::record_virtual_memory_reserve((address)result,
> > bytes, CALLER_PC);
> > 293   }
> > - Combining line 284 and 291 seems better to me.
> > 284   if (result != NULL) {
> >         if (file_desc != -1) {
> >           if (replace_existing_mapping_with_dax_file_mapping(result,
> > bytes, file_desc) == NULL) {
> >             vm_exit_during_initialization(err_msg("Error in mapping
> > Java heap at the given filesystem directory"));
> >           }
> >
> >
> MemTracker::record_virtual_memory_reserve_and_commit((address)result
> ,
> > bytes, CALLER_PC);
> >         } else {
> >           MemTracker::record_virtual_memory_reserve((address)result,
> > bytes, CALLER_PC);
> >         }
> >       }
> >       return result;
> >
> > -----------------------------
> > src/os/windows/vm/os_windows.cpp
> > 3141 // if 'base' is not NULL, function will return NULL if it cannot
> > get 'base'
> > - Start with uppercase.
> >
> > 3142 //
> > - This line seems redundant.
> >
> > 3151       vm_exit_during_initialization(err_msg("Could not allocate
> > sufficient disk space for heap"));
> > - heap -> Java heap (same as line 3153)
> >
> > 3168   assert(base != NULL, "base cannot be NULL");
> > - 'base' -> 'Base' or 'Base address'.
> >
> > 3172
> > - Redundant line.
> >
> > 3230     }
> > 3231     else {
> > -> } else {
> >
> > 3278   return reserve_memory(bytes, requested_addr, 0);
> > - Is it correct to use '0' not '-1'? It would be better to explain why
> > we use hard-coded value here.
> >
> > -----------------------------
> > src/share/vm/memory/universe.cpp
> > - No comments
> >
> > -----------------------------
> > src/share/vm/memory/virtualspace.cpp
> > - copyright update
> >
> > 74                                            const size_t size, bool
> > special, bool is_file_mapped= false)
> > - Need space between 'is_file_mapped' and '='.
> >
> > 92           fatal("os::release_memory failed");
> > - Typo, 'os::unmap_memory failed'.
> >
> > 129   // If there is a backing file directory for this VirtualSpace
> > then whether
> > - This is not VirtualSpace. Probably just 'space'.
> >
> > 130   // large pages are allocated is upto the filesystem the dir
> > resides in.
> > - 'dir'? Probably 'backing file for Java heap'.
> >
> > 137       log_debug(gc, heap, coops)("UseLargePages can't be set with
> > HeapDir option.");
> > - Is it enough to print log message instead of warning message? i.e.
> > Don't we need something at arguments.cpp:3656, similar to NUMA flags?
> >
> > 191         // unmap_memory will do extra work esp. in Windows
> > - esp. -> especially
> >
> > 282       }
> > 283       else {
> > -> } else {
> >
> > 346   // If there is a backing file directory for this VirtualSpace
> > then whether
> > - Again this is not VirtualSpace, so just 'space'.
> >
> > 352     if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||
> > 353       !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {
> > - Wrong alignment at line 353. Consider to make same as line 380.
> >
> > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> > alignment, bool large, const char* backing_fs_for_heap)
> > - ReservedSpace has '_backing_fd' but the constructor doesn't take it
> > as a parameter and only ReservedHeapSpace uses it. This seems not
> > ideal, couldn't make it better? I know actual logic is at
> > ReservedSpace so it is not convenient to add _backing_fs_for_heap at
> > ReservedHeapSapce.
> >
> > -----------------------------
> > src/share/vm/memory/virtualspace.hpp
> > 40   int    _backing_fd;
> > - I would prefer to have better name to describe.
> >   e.g. as command-line option name is 'HeapDir', _heap_fd or
> > _fd_for_heap(similar to below)?
> >
> > 115   ReservedHeapSpace(size_t size, size_t forced_base_alignment,
> > bool large, const char* backingFSforHeap = NULL);
> > - Snake case. How about 'fs_for_heap' or 'heap_fs'?
> >
> > -----------------------------
> > src/share/vm/runtime/arguments.cpp
> > 3655     FLAG_SET_CMDLINE(bool, UseNUMA, false);
> > - (questions) Don't need to add a warning message for
> > UseLargePagesSame=true as commented virtualspace.cpp:137?
> >
> > -----------------------------
> > src/share/vm/runtime/globals.hpp
> > - No comments
> >
> > -----------------------------
> > src/share/vm/runtime/os.cpp
> >
> > 1632
> > - Extra line.
> >
> > 1642   }
> > 1643   else {
> > -> } else {
> >
> > 1663
> > - You removed os::attempt_reserve_memory_at() from os.cpp and split
> > into os_posix.cpp and os_windows.cpp.
> >   But I think you should remain os::attempt_reserve_memory_at() at
> > os.cpp and implement os specific stuffs at each os_xxx.cpp files for
> > pd_xxx. Of cource move MemTracker function call as well.
> >
> > -----------------------------
> > src/share/vm/runtime/os.hpp
> >
> > 349   // replace existing reserved memory with file mapping
> > - Start with uppercase letter.
> >
> > Thanks,
> > Sangheon
> >
> >
> >
> >
> > - Kishor
> >
> > From: Kharbas, Kishor
> > Sent: Tuesday, July 11, 2017 6:40 PM
> > To: '[hidden email]' <[hidden email]
> > t>
> > Cc: Kharbas, Kishor <[hidden email]>
> > Subject: RFR(M): 8171181: Supporting heap allocation on alternative
> > memory devices
> >
> > Greetings,
> >
> > I have an updated patch for JEP https://bugs.openjdk.java.net/browse/
> > JDK-8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> > This patch fixes the bugs pointed earlier and other suggestions to
> > make the code less intrusive.
> >
> > I have also sent this to ‘hotspot-runtime-dev’ mailing list (included
> > below).
> >
> > I would appreciate comments and feedback.
> >
> > Thanks
> > Kishor
> >
> > From: Kharbas, Kishor
> > Sent: Monday, July 10, 2017 1:53 PM
> > To: [hidden email]
> > Cc: Kharbas, Kishor <[hidden email]>
> > Subject: RFR(M): 8171181: Supporting heap allocation on alternative
> > memory devices
> >
> > Hello all!
> >
> > I have an updated patch for https://bugs.openjdk.java.net/browse/JDK-
> > 8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> > I have lost the old email chain so had to start a fresh one. The
> > archived conversation can be found at - http://mail.openjdk.java.net/
> > pipermail/hotspot-runtime-dev/2017-March/022733.html
> >
> > 1.       I have worked on all the comments and fixed the bugs. Mainly
> > bugs fixed are related to sigprocmask() and changed the implementation
> > such that ‘fd’ is not passed all the way down the call stack. Thus
> > minimizing function signature changes.
> >
> > 2.       Patch supports all OS’es. Consolidated all Posix compliant
> > OS’s implementation in os_posix.cpp.
> >
> > 3.       The patch is tested on Windows and Linux. Working on testing
> > it on other OS’es.
> >
> > Let me know if this version looks clean and correct.
> >
> > Thanks
> > Kishor
> >

Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
Hi,

On Thu, 2017-10-26 at 04:33 +0000, Kharbas, Kishor wrote:

> I got delivery failure to the individual email addresses, so
> forwarding the email.
>
> Regards
> Kishor
>
> -----Original Message-----
> From: Kharbas, Kishor 
> Sent: Wednesday, October 25, 2017 9:28 PM
> To: Thomas Schatzl <[hidden email]>; sangheon.kim <sangheo
> [hidden email]>; '[hidden email]' <hotspot-gc-dev@
> openjdk.java.net>
> Cc: [hidden email]; Kharbas, Kishor <kishor.kha
> [hidden email]>
> Subject: RE: RFR(M): 8171181: Supporting heap allocation on
> alternative memory devices
>
> Hi Sangheon, Thomas,
>
> Thanks for the review. Here is the latest webrev based on your
> comments and my replies to both are inline (I copied Sangheon's email
> here at the top above Thomas's reply)
> http://cr.openjdk.java.net/~kkharbas/8171181/webrev.10_to_09
> http://cr.openjdk.java.net/~kkharbas/8171181/webrev.10
>
> Please let me know your question/comments.

Just minor nits for now:

- virtualspace.cpp:72: s/Healper/Helper

- TestAllocateHeapAt.java:2: This is a new file, so I guess copyright
date should not start from 2013.

Thanks for all these changes.

Thanks,
  Thomas

>
> Thanks
> Kishor
>
> > Hi Kishor,
> > On 10/19/2017 08:00 AM, Kharbas, Kishor wrote:
> > Hi Sangheon,
>
>  
> > Thanks for the review and comments.
> > I created two webrevs:
> > webrev.07 : Is the rebase of webrev.06 on jdk10    (http://cr.openj
> > dk.java.net/~kkharbas/8171181/webrev.07/)
> > webrev.08 : Is incremental webrev over 07.              (http://cr.
> > openjdk.java.net/~kkharbas/8171181/webrev.08/)
> > 1. I couldn't apply webrev.07 patch cleanly. Could you test? 
> >    Just minor nit: 07 seems not only rebase of webrev.06. i.e.
> > there seems to have some changes from my comment of alignments.
>
> [Kharbas, Kishor] My bad, I had rebased the patch on jdk10/jdk10. The
> further ones are on jdk10/hs
>
> > 2. The description in the JEP uses 'AllocateHeapAt' while the patch
> > uses 'HeapDir'.
>
> [Kharbas, Kishor] I changed the code. Thanks
>
> > -----------------------------
> > src/os/posix/vm/os_posix.cpp
> > 159   (void)strcpy(fullname, dir);
> > - Please use strncpy instead.
>
> [Kharbas, Kishor] Done
>
> > 160   (void)strcat(fullname, name_template);
> > - Please use strncat instead.
>
> [Kharbas, Kishor] Done.
>
> > 180     ::free(fullname);
> > - os::free().
>
> [Kharbas, Kishor] Done.
>
> > 196   ::free(fullname);
> > - os::free().
>
> [Kharbas, Kishor] Done.
>
> > -----------------------------
> > src/os/windows/vm/os_windows.cpp
> > 2885   char *fullname = (char*)_alloca(strlen(dir) +
> > sizeof(name_template));
> > - Missing allocation failure handling.
> > - Please use _malloca instead.
> >  * This function is deprecated because a more secure version is
> > available; see _malloca.
> >    https://msdn.microsoft.com/en-us/library/wb1s57t5.aspx
>
> [Kharbas, Kishor] Done.
>
> > 2886   (void)strcpy(fullname, dir);
> > - Please use strncpy instead.
>
> [Kharbas, Kishor] Done.
>
> > 2887   (void)strcat(fullname, name_template);
> > - Please use strncat instead.
>
> [Kharbas, Kishor] Done.
>
> > -----------------------------
> > src/share/vm/runtime/arguments.cpp
> > 3726     }
> > 3727   }
> > - My previous comment was to add explicit explanation of disabling
> > UseNUMA and UseNUMAInterleaving.
> > 4636     if(!FLAG_IS_DEFAULT(HeapDir)) {
> > - Previsouly we checked UseNUMA and UseNUMAInterleaving and then
> > disabled those. IMHO, I think previous one seems better with more
> > explanation that we are going to disable those options. i.e.
> > >Similar to os_linux.cpp:4869, a warning message with disabling
> > codes.
>
> [Kharbas, Kishor] Based on our discussion offline, I think we agree
> that it’s OK to disable UseNUMA and leave UseNUMAInterleaving ON
> since other areas of JVM such as CodeCache can use
> UseNUMAInterleaving.
>
> > 4638     }
> > 4639     else if (UseParallelGC || UseParallelOldGC) {
> > - One line please if this change is still needed. 
>
>   -> } else if {
>
> > -----------------------------
> > src/share/vm/runtime/os.cpp
> > 1678
> > - Extra line.
>
> [Kharbas, Kishor] Done.
>
> > 1688   }
> > 1689   else {
> > -> } else {
>
> [Kharbas, Kishor] Done.
>
> > My answers are inlined.
>
>  
> > In webrev08 I have addressed all the comments, except for ones
> > below:
>
>  
> > ---------------------------------------
> > src/os/aix/vm/os_aix.cpp
> > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char* 
> > requested_addr, bool use_SHM) {
> > - Question. Why os_aix has additional parameter at
> > pd_attemp_reserve_memory_at()? Probably only AIX has shmated memory
> > implementation?
> >         A: Yes that’s correct.
> > Okay.
> > --------------------------------------
> > 137       log_debug(gc, heap, coops)("UseLargePages can't be set
> > with HeapDir option.");
> > - Is it enough to print log message instead of warning message?
> > i.e. Don't we need something at arguments.cpp:3656, similar to NUMA
> > flags?
> >       A: If don’t disable UseLargePages like UseNUMA because large
> > pages can be used for other allocation such as CodeCache.
> > I meant to changing log_debug() to log_warning().
> > If UseNUMA is enabled, we print warning at arguments.cpp:3725 while
> > UseLargePages is enabled, we print debug message.
> > In addition, is this enough? Don't we need to force disabling
> > UseLargePages?
> > What happens if both options are enabled?
> > ------------------------------------
> > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t
> > alignment, 
> > bool large, const char* backing_fs_for_heap)
> > - ReservedSpace has '_backing_fd' but the constructor doesn't take
> > it as a parameter and only ReservedHeapSpace uses it. This seems
> > not ideal, couldn't make it better? I know actual logic is at
> > >ReservedSpace so it is not convenient to add _backing_fs_for_heap
> > at ReservedHeapSapce.
> >      A: ReservedHeapSpace depends on few functions in ReservedSpace
> > such as initialize(), release(). So instead of passing it as
> > argument, I set it as a propert of ReservedSpace.
> > Okay.
>
> -----------------------------------
> > 1663
> > - You removed os::attempt_reserve_memory_at() from os.cpp and split
> > into os_posix.cpp and os_windows.cpp.
> >  But I think you should remain os::attempt_reserve_memory_at() at
> > os.cpp and implement os specific stuffs at each os_xxx.cpp files
> > for pd_xxx. Of cource move MemTracker function call as well.
> >        A: I do it this way to reduce the redundant code, If I
> > implement in OS specific files in pd_xxx(), the code to replace
> > reserved mapping with file mapping
> > >(replace_existing_mapping_with_dax_file_mapping()) will be
> > redundant.
> >             Still if you feel I will do the change and see how it
> > looks.
> > I would prefer to have pd_xxx() even though there's some redundant
> > code.
>
> [Kharbas, Kishor] We also had a discussion about this offline, I will
> address this in the next webrev.
>
> > Thanks,
> > Sangheon
> > -----Original Message-----
> > From: Thomas Schatzl [mailto:[hidden email]]
> > Sent: Tuesday, October 24, 2017 8:39 AM
> > To: Kharbas, Kishor <[hidden email]>; sangheon.kim 
> > <[hidden email]>; '[hidden email]'
> > <[hidden email]>
> > Cc: [hidden email]
> > Subject: Re: RFR(M): 8171181: Supporting heap allocation on 
> > alternative memory devices
> >
> > Hi Kishor,
> >
> >   initial review using the webrevs, and the comment thread in
> > hotspot- 
> > runtime-dev (readded to cc because questions were left unanswered
> > there).
> >
> > The code also touches runtime code quite a bit, so I would
> > appreciate 
> > comments by the runtime team.
> >
> > On Thu, 2017-10-19 at 15:00 +0000, Kharbas, Kishor wrote:
> > > Hi Sangheon,
> > >
> > > Thanks for the review and comments.
> > > I created two webrevs:
> > > webrev.07 : Is the rebase of webrev.06 on jdk10    
> > > (http://cr.openjdk
> > > .java.net/~kkharbas/8171181/webrev.07/)
> > > webrev.08 : Is incremental webrev over 07.              
> > > (http://cr.op
> > > enjdk.java.net/~kkharbas/8171181/webrev.08/)
> > >
> >
> > Some first comments on the webrev follow:
> >
> >  - could you please provide both a full and incremental webrev for 
> > your changes? The former help the reviewers late to the party, the 
> > incremental ones the people already working with you.
> >
>
> [Kharbas, Kishor] Yes. I will keep this in mind.
>
> >  - the patch has not been rebased to jdk10/hs as Sangheon
> > suggested.
> > Probably you rebased to jdk10/jdk10?
> >
>
> [Kharbas, Kishor] My bad, it was rebased to jdk10/jdk10. New webrev's
> will be based on jdk10/hs.
>
> > os_posix.cpp:
> >
> >  - os::create_file_for_heap: the malloc allocates one byte too
> > little, 
> > creating an automatic write outside the buffer at the strcat()
> > method.
> >
>
> [Kharbas, Kishor] Done.
>
> >  - os::create_file_for_heap does not use the size parameter. Please
> > remove.
> >
>
> [Kharbas, Kishor] Done.
>
> >  - in several places: improve the output of the error messages to
> > have 
> > meaning for the user. Not just "malloc failed"; there were already 
> > some suggestions in the referenced thread at  
> > http://mail.openjdk.java.net/p ipermail/hotspot-runtime-dev/2017-
> > March/022733.html .
> >
> > Same for the asserts, and particularly for them - they will
> > implicitly 
> > terminate the VM, so e.g. a "fchmod error" is definitely too
> > little. 
> > At least error no/output of os::strerror() etc. would be required
> > for quick diagnosis.
> >
> > I read in some of the emails that you intend to let the caller
> > print a 
> > good error message. I fear that the caller might not have the 
> > necessary information any more to do that in a useful way.
> >
>
> [Kharbas, Kishor] Done. Moved macro assert_with_errror() to top of
> file so I can use in my code.
>
> >  - in that same thread there has also been the question about the
> > need 
> > for blocking the signals for the process that has gone unanswered.
> >
>
> [Kharbas, Kishor] Let me get back on this. The implementation at
> pmem.io which I use as reference for file operations does blocks
> signals. I will check if there is more to it than just a 'good
> practice'.
>
> >  - in that same thread there has also been the question about why
> > the 
> > code sets permissions to 0600 manually; this is because of glibc
> > 2.0.6 compatibility.
> > Glibc 2.06 has been released 1997-12-29. I looked a bit through
> > glibc 
> > requirements for the Oracle JDK, and while I could not find exact 
> > requirements, some posts indicate the need for GLIBC_2.4 at
> > minimum 
> > (for
> > jdk7) which is from 2003. I recommend removing this, it seems 
> > unnecessary and completely outdated.
> >
>
> [Kharbas, Kishor] Agree. Removed it.
>
> >  - os::malloc should be paired with os::free in
> > os::create_file_for_heap() (2x)
> >
>
> [Kharbas, Kishor] Done.
>  
> >  - I am not sure whether the methods that deal with mapping memory
> > to 
> > a file should have "dax" in their name. The code seems to be
> > pretty 
> > generically map memory into a file.
> >
>
> [Kharbas, Kishor] I used 'dax' in the name because to get direct
> mapping to DAX device you need to use 'MAP_SHARED'. That makes this
> map function different than generic map function. But it’s a minor
> point. I can change the name if it's still desired./mkstem/
>
> >  - I am not sure if the current approach to FS errors after mkstemp
> > is 
> > very nice. Please at least print a meaningful warning message to
> > the log.
> >
>
> [Kharbas, Kishor] Added a warning() message when mkstemp could not
> create a file.
>
> >  - please also specify the meaning of the return value for
> > os::create_file_for_heap() in the documentation.
> >
>
> [Kharbas, Kishor] Added more information in the comment before
> function's declaration.
>
> >  - there is a missing "p" in the name of
> > os::reserve_mmaped_memory().
> >
>
> [Kharbas, Kishor] Done.
>
> >  - os::util_posix_fallocate(): s/continous/continuous
> >
>
> [Kharbas, Kishor] Done.
>
> >  - os::map_memory_to_dax_file(): according to man pages, 
> > posix_fallocate does not set errno, but the code checking its
> > return 
> > (or
> > util_posix_fallocate()) expects it to do so.
> >
>
> [Kharbas, Kishor] I removed printing of error string. Can the return
> error value of posix_fallocate() we used as argument to
> os::strerror(errno)?
>
> >  - os::attempt_reserve_memory_at(), in the call to
> > pd_attempt_reserve_memory_at() for AIX, in the third parameter,
> > please 
> > use the exact name of the parameter in the comment.
> >
>
> [Kharbas, Kishor] Done.
>
> >  - os::attempt_reserve_memory_at(), the second "if" needs a blank 
> > before the bracket.
> >
>
> [Kharbas, Kishor] Done.
>
> >  - os::attempt_reserve_memory_at(), in the comment,
> > s/mmemory/memory
> >
>
> [Kharbas, Kishor] Done.
>
> >  - in that same method, the #if..#endif block should be aligned
> > like 
> > other similar blocks.
> >
>
> [Kharbas, Kishor] Done.
>
> >  - in that same method, the "else" should be on the same line as
> > the 
> > closing bracket of the if-body.
> >
>
> [Kharbas, Kishor] Done.
>
> > os_windows.cpp:
> >
> > os::create_file_for_heap():
> >  - same bug about length of allocation the _alloca call. Not 
> > completely sure about why use alloca (or not use alloca in
> > os_posix.cpp).
> >
>
> [Kharbas, Kishor] Changed to have uniform implementation.
>
> >  - this version calls os::native_path(). It seems strange to not
> > do 
> > that as well in the others (although it does nothing).
> >
>
> [Kharbas, Kishor] Added in os_posix.cpp to make it uniform.
>
> >  - os::reserve_memory_aligned(): an "else" should be moved into
> > the 
> > same line as the closing bracket.
> >
>
> [Kharbas, Kishor] Done.
>
> > universe.cpp:
> >
> >  - Universe::reserve_heap(): I do not think this functionality has 
> > anything to do with compressed oops, so the log message should not
> > have the coops tag.
> > The log message could/should have more information about the file
> > location.
> > I also recommend using "Java heap" instead of "Heap" here, as the 
> > latter is ambiguous.
> >
>
> [Kharbas, Kishor] Done. Added more info to log message.
>
> > virtualspace.cpp:
> >
> >  - in failed_to_reserve_as_requested: "else" indentation. Also,
> > the 
> > "else { if (..." could be shortened to "} else if (..." to avoid
> > the extra indentation level.
> > (2x)
> >
>
> [Kharbas, Kishor] Done.
>
> >  - ReservedSpace::initialize(): s/upto/up to
> >
>
> [Kharbas, Kishor] Done.
>
> >  - ReservedSpace::initialize(): remove the coops tag here as well.
> > Also, the info message might be more similar to the comment above 
> > where it says that large page support is up to the file system, and
> > the flag ignored.
> >
>
> [Kharbas, Kishor] Done.
> >  - the code contains the snippet
> >
> > if (_fd_for_heap != -1) {
> >   if (!os::unmap_memory...
> > } else {
> >   if (!os::release_memory...
> > }
> >
> > at least twice. Maybe this code snippet could be extracted into a
> > new 
> > method.
> >
>
> [Kharbas, Kishor] Defined a new static function
> unmap_or_release_memory()
>
> >  - (ignore if you want) ReservedHeapSpace::ReservedHeapSpace, at
> > the 
> > end
> > - maybe the condition is easier to read if it looks like:
> >
> > if (_fd_for_heap != -1) {
> >   os::close(_fd_for_heap);
> > }
> >
> > than it is now.
>
> [Kharbas, Kishor] I agree. Done.
> >
> >  - virtualspace.hpp: in the changed constructor signature, please
> > add 
> > a comment about what backing_fs_for_heap actually means (and what 
> > happens if set).
> >
>
> [Kharbas, Kishor] Done.
>
> > arguments.cpp:
> >
> >  - Arguments::finalize_vm_init_args: maybe at the place where the 
> > change adds the warning/info message about NUMA support being
> > specific 
> > to the file system; also add the same warning about UseLargePages
> > that 
> > is located elsewhere.
> >
> > Like "UseXXXX may not be supported in some specific file system 
> > implementations." - or just ignore as Andrew said in the other
> > email thread.
> >
> > Note that I am not sure that the selected place is the correct
> > place 
> > for such warning about incompatible flags (and maybe 
> > UseNUMA/UseLargePages should be forcibly disabled here? But then 
> > again, it does not hurt just having it enabled?).
> >
> > I will let the runtime team comment as a lot of that argument 
> > processing changed in jdk9.
> >
> > Maybe Arguments::check_vm_args_consistency() is better.
> >
> > There is similar code in Arguments::adjust_after_os()...
> >
>
> [Kharbas, Kishor] In progress. I will get back on these comments in
> next reply.
>
> > os.cpp:
> >
> >  - os::reserve_memory: "else" indentation
> >
> >  - os::reserve_memory: I do not follow that comment - it explains
> > the 
> > code as written, not why, and what map_memory_to_dax_file() has to
> > do 
> > with AIX and SHM. It uses an outdated method name, and also
> > s/your/our.
> >
> > Probably the whole comment can be removed.
> >
>
> [Kharbas, Kishor] I changed the comment. In the beginning I had
> called pd_reserve_memory() followed by
> replace_existing_mapping_with_dax_file_mapping(), but later realized
> that AIX can allocate from SHM, in which case it's not straight
> forward to replace the mapping. So I leave this comment to make sure
> in future I (or someone else) does not ponder over the same thing.
>
> > os.hpp:
> >
> >  - indentation of the new #if defined clause
> >
> >  - here I may probably be speaking wrongly on behalf of the
> > runtime 
> > team, but os.hpp, as an abstraction of the OS should probably not
> > have 
> > platform specific ifdefs in it.
> >
>
> [Kharbas, Kishor] Change the indentation. I will address abstraction
> issue in next reply.
>
> > Thanks,
> >   Thomas
> >
> > > In webrev08 I have addressed all the comments, except for ones
> > > below:
> > >
> > > ---------------------------------------
> > > src/os/aix/vm/os_aix.cpp
> > >
> > > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char* 
> > > requested_addr, bool use_SHM) {
> > > - Question. Why os_aix has additional parameter at 
> > > pd_attemp_reserve_memory_at()? Probably only AIX has shmated
> >
> > memory
> > > implementation?
> > >          A: Yes that’s correct.
> > >
> > > --------------------------------------
> > > 137       log_debug(gc, heap, coops)("UseLargePages can't be set 
> > > with HeapDir option.");
> > > - Is it enough to print log message instead of warning message?
> > > i.e.
> > > Don't we need something at arguments.cpp:3656, similar to NUMA
> > > flags?
> > >        A: If don’t disable UseLargePages like UseNUMA because
> > > large 
> > > pages can be used for other allocation such as CodeCache.
> > >
> > > ------------------------------------
> > >
> > > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t 
> > > alignment, bool large, const char* backing_fs_for_heap)
> > > - ReservedSpace has '_backing_fd' but the constructor doesn't
> > > take 
> > > it as a parameter and only ReservedHeapSpace uses it. This seems
> > > not 
> > > ideal, couldn't make it better? I know actual logic is at 
> > > ReservedSpace so it is not convenient to add _backing_fs_for_heap
> > > at 
> > > ReservedHeapSapce.
> > >       A: ReservedHeapSpace depends on few functions in
> > > ReservedSpace 
> > > such as initialize(), release(). So instead of passing it as 
> > > argument, I set it as a propert of ReservedSpace.
> > >
> > > -----------------------------------
> > > 1663
> > > - You removed os::attempt_reserve_memory_at() from os.cpp and
> > > split 
> > > into os_posix.cpp and os_windows.cpp.
> > >   But I think you should remain os::attempt_reserve_memory_at()
> > > at 
> > > os.cpp and implement os specific stuffs at each os_xxx.cpp files
> > > for 
> > > pd_xxx. Of cource move MemTracker function call as well.
> > >         A: I do it this way to reduce the redundant code, If I 
> > > implement in OS specific files in pd_xxx(), the code to replace 
> > > reserved mapping with file mapping
> > > (replace_existing_mapping_with_dax_file_mapping()) will be
> > > redundant.
> > >              Still if you feel I will do the change and see how
> > > it 
> > > looks.
> > >
> > > Regards
> > > Kishor
> > >
> > >
> > > From: sangheon.kim [mailto:[hidden email]]
> > > Sent: Tuesday, September 26, 2017 3:18 PM
> > > To: Kharbas, Kishor <[hidden email]>; 
> > > 'hotspot-gc-dev@openj dk.java.net' <[hidden email]
> > > et>
> > > Subject: Re: RFR(M): 8171181: Supporting heap allocation on 
> > > alternative memory devices
> > >
> > > Hi Kishor,
> > >
> > > On 07/20/2017 06:34 PM, Kharbas, Kishor wrote:
> > > I have a new version of this patch at 
> > > http://cr.openjdk.java.net/~kkh arbas/8171181/webrev.06/
> > >
> > > This version has been tested on Windows, Linux, Solaris and Mac
> > > OS. 
> > > I could not get access to AIX for testing.
> > > I used tmpfs to test the functionality. Cases that were tested
> > > were.
> > > 1.       Allocation of heap using file mapping when –XX:HeapDir= 
> > > option is used.
> > > 2.       Creation of nameless temporary file for Heap allocation 
> > > which prevents access to file using its name.
> > > 3.       Correct deletion and freeing up of space allocated for
> > > file 
> > > under different exit conditions.
> > > 4.       Error handling when path specified is not present, heap 
> > > size is more than size of file system, etc.
> > > Overall seems good.
> > > I tried to list some missing part.
> > >
> > > 1. Please rebase with consolidated repository. (jdk10/hs) 2.
> > > Build 
> > > failure on Solaris.
> > >     (Sorry no build error log file, as I tested a few weeks ago,
> > > it 
> > > is
> > > deleted) 3. Have you tested with various heap reserve path using 
> > > HeapBaseMinAddress flag? i.e. to test code path of
> > > ReservedHeapSpace::try_reserve_heap() and try_reserve_range().
> > > 4. How about adding HeapDir allocation success message? e.g.
> > > gc+heap+coops=info
> > > 5. Adding JTREG test. Probably we would need to verify this 
> > > allocation is succeeded via #4 added allocation success message.
> > > 6. CSR (Compatibility & Specification Review). At some point,
> > > you 
> > > need to file another CR of 'CSR' type to add a new flag of
> > > 'HeapDir'.
> > > 7. It will be much appreciated if you provide incremental webrev.
> > > I 
> > > think 06(this version) vs. 07(rebase version) would be hard to
> > > get.
> > > Probably from 08 version.
> > >
> > > Here's my comments.
> > > -----------------------------
> > > src/os/aix/vm/os_aix.cpp
> > >
> > > 2514 char* os::pd_attempt_reserve_memory_at(size_t bytes, char* 
> > > requested_addr, bool use_SHM) {
> > > - Question. Why os_aix has additional parameter at 
> > > pd_attemp_reserve_memory_at()? Probably only AIX has shmated
> >
> > memory
> > > implementation?
> > >
> > > -----------------------------
> > > src/os/posix/vm/os_posix.cpp
> > >
> > > 148   char *fullname = (char*)::malloc(strlen(dir) + 
> > > sizeof(name_template));
> > > - Use os::malloc()
> > >
> > > 196   int flags;
> > > 197
> > > 198   flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;
> > > - Combining 196 and 198 seems better to me.
> > >
> > > 200     assert((uintptr_t)requested_addr % os::Linux::page_size()
> > > == 
> > > 0, "unaligned address");
> > > - Linux dependency on posix file which makes build error on
> > > Solaris.
> > > Probably os::vm_page_size().
> > >
> > > 207   addr = (char*)::mmap(requested_addr, bytes, PROT_NONE,
> > > 208     flags, -1, 0);
> > > - Missing some spaces? Alignment seems odd to me.
> > >
> > > 226     if (ret == -1)
> > > - Probably you wanted to add handling code? If not, just return
> > > ret.
> > >
> > > 252   if (addr == MAP_FAILED || (base != NULL && addr != base)) {
> > > 253     if (addr != MAP_FAILED) {
> > > 254       if (!os::release_memory(addr, size)) {
> > > 255         warning("Could not release memory on unsuccessful
> > > file 
> > > mapping");
> > > 256       }
> > > 257     }
> > > 258     return NULL;
> > > 259   }
> > > - Splitting MAP_FAILED case and another gives better readability
> > > to 
> > > me. But this is your call.
> > >
> > > 269
> > > - Extra line.
> > >
> > > 284   if (result != NULL && file_desc != -1) {
> > > 285     if
> > > (replace_existing_mapping_with_dax_file_mapping(result,
> > > bytes, file_desc) == NULL) {
> > > 286       vm_exit_during_initialization(err_msg("Error in
> > > mapping 
> > > Java heap at the given filesystem directory"));
> > > 287     }
> > > 288
> > >
> >
> > MemTracker::record_virtual_memory_reserve_and_commit((address)resul
> > t
> > ,
> > > bytes, CALLER_PC);
> > > 289     return result;
> > > 290   }
> > > 291   if (result != NULL) {
> > > 292    
> > > MemTracker::record_virtual_memory_reserve((address)result,
> > > bytes, CALLER_PC);
> > > 293   }
> > > - Combining line 284 and 291 seems better to me.
> > > 284   if (result != NULL) {
> > >         if (file_desc != -1) {
> > >           if
> > > (replace_existing_mapping_with_dax_file_mapping(result,
> > > bytes, file_desc) == NULL) {
> > >             vm_exit_during_initialization(err_msg("Error in
> > > mapping 
> > > Java heap at the given filesystem directory"));
> > >           }
> > >
> > >
> >
> > MemTracker::record_virtual_memory_reserve_and_commit((address)resul
> > t
> > ,
> > > bytes, CALLER_PC);
> > >         } else {
> > >          
> > > MemTracker::record_virtual_memory_reserve((address)result,
> > > bytes, CALLER_PC);
> > >         }
> > >       }
> > >       return result;
> > >
> > > -----------------------------
> > > src/os/windows/vm/os_windows.cpp
> > > 3141 // if 'base' is not NULL, function will return NULL if it 
> > > cannot get 'base'
> > > - Start with uppercase.
> > >
> > > 3142 //
> > > - This line seems redundant.
> > >
> > > 3151       vm_exit_during_initialization(err_msg("Could not
> > > allocate 
> > > sufficient disk space for heap"));
> > > - heap -> Java heap (same as line 3153)
> > >
> > > 3168   assert(base != NULL, "base cannot be NULL");
> > > - 'base' -> 'Base' or 'Base address'.
> > >
> > > 3172
> > > - Redundant line.
> > >
> > > 3230     }
> > > 3231     else {
> > > -> } else {
> > >
> > > 3278   return reserve_memory(bytes, requested_addr, 0);
> > > - Is it correct to use '0' not '-1'? It would be better to
> > > explain 
> > > why we use hard-coded value here.
> > >
> > > -----------------------------
> > > src/share/vm/memory/universe.cpp
> > > - No comments
> > >
> > > -----------------------------
> > > src/share/vm/memory/virtualspace.cpp
> > > - copyright update
> > >
> > > 74                                            const size_t size, 
> > > bool special, bool is_file_mapped= false)
> > > - Need space between 'is_file_mapped' and '='.
> > >
> > > 92           fatal("os::release_memory failed");
> > > - Typo, 'os::unmap_memory failed'.
> > >
> > > 129   // If there is a backing file directory for this
> > > VirtualSpace 
> > > then whether
> > > - This is not VirtualSpace. Probably just 'space'.
> > >
> > > 130   // large pages are allocated is upto the filesystem the
> > > dir 
> > > resides in.
> > > - 'dir'? Probably 'backing file for Java heap'.
> > >
> > > 137       log_debug(gc, heap, coops)("UseLargePages can't be set 
> > > with HeapDir option.");
> > > - Is it enough to print log message instead of warning message?
> > > i.e.
> > > Don't we need something at arguments.cpp:3656, similar to NUMA
> > > flags?
> > >
> > > 191         // unmap_memory will do extra work esp. in Windows
> > > - esp. -> especially
> > >
> > > 282       }
> > > 283       else {
> > > -> } else {
> > >
> > > 346   // If there is a backing file directory for this
> > > VirtualSpace 
> > > then whether
> > > - Again this is not VirtualSpace, so just 'space'.
> > >
> > > 352     if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||
> > > 353       !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {
> > > - Wrong alignment at line 353. Consider to make same as line 380.
> > >
> > > 603 ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t 
> > > alignment, bool large, const char* backing_fs_for_heap)
> > > - ReservedSpace has '_backing_fd' but the constructor doesn't
> > > take 
> > > it as a parameter and only ReservedHeapSpace uses it. This seems
> > > not 
> > > ideal, couldn't make it better? I know actual logic is at 
> > > ReservedSpace so it is not convenient to add _backing_fs_for_heap
> > > at 
> > > ReservedHeapSapce.
> > >
> > > -----------------------------
> > > src/share/vm/memory/virtualspace.hpp
> > > 40   int    _backing_fd;
> > > - I would prefer to have better name to describe.
> > >   e.g. as command-line option name is 'HeapDir', _heap_fd or 
> > > _fd_for_heap(similar to below)?
> > >
> > > 115   ReservedHeapSpace(size_t size, size_t
> > > forced_base_alignment, 
> > > bool large, const char* backingFSforHeap = NULL);
> > > - Snake case. How about 'fs_for_heap' or 'heap_fs'?
> > >
> > > -----------------------------
> > > src/share/vm/runtime/arguments.cpp
> > > 3655     FLAG_SET_CMDLINE(bool, UseNUMA, false);
> > > - (questions) Don't need to add a warning message for 
> > > UseLargePagesSame=true as commented virtualspace.cpp:137?
> > >
> > > -----------------------------
> > > src/share/vm/runtime/globals.hpp
> > > - No comments
> > >
> > > -----------------------------
> > > src/share/vm/runtime/os.cpp
> > >
> > > 1632
> > > - Extra line.
> > >
> > > 1642   }
> > > 1643   else {
> > > -> } else {
> > >
> > > 1663
> > > - You removed os::attempt_reserve_memory_at() from os.cpp and
> > > split 
> > > into os_posix.cpp and os_windows.cpp.
> > >   But I think you should remain os::attempt_reserve_memory_at()
> > > at 
> > > os.cpp and implement os specific stuffs at each os_xxx.cpp files
> > > for 
> > > pd_xxx. Of cource move MemTracker function call as well.
> > >
> > > -----------------------------
> > > src/share/vm/runtime/os.hpp
> > >
> > > 349   // replace existing reserved memory with file mapping
> > > - Start with uppercase letter.
> > >
> > > Thanks,
> > > Sangheon
> > >
> > >
> > >
> > >
> > > - Kishor
> > >
> > > From: Kharbas, Kishor
> > > Sent: Tuesday, July 11, 2017 6:40 PM
> > > To: '[hidden email]
> > > <[hidden email]
> > > t>
> > > Cc: Kharbas, Kishor <[hidden email]>
> > > Subject: RFR(M): 8171181: Supporting heap allocation on
> > > alternative 
> > > memory devices
> > >
> > > Greetings,
> > >
> > > I have an updated patch for JEP 
> > > https://bugs.openjdk.java.net/browse/
> > > JDK-8171181 at 
> > > http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> > > This patch fixes the bugs pointed earlier and other suggestions
> > > to 
> > > make the code less intrusive.
> > >
> > > I have also sent this to ‘hotspot-runtime-dev’ mailing list 
> > > (included below).
> > >
> > > I would appreciate comments and feedback.
> > >
> > > Thanks
> > > Kishor
> > >
> > > From: Kharbas, Kishor
> > > Sent: Monday, July 10, 2017 1:53 PM
> > > To: [hidden email]
> > > Cc: Kharbas, Kishor <[hidden email]>
> > > Subject: RFR(M): 8171181: Supporting heap allocation on
> > > alternative 
> > > memory devices
> > >
> > > Hello all!
> > >
> > > I have an updated patch for 
> > > https://bugs.openjdk.java.net/browse/JDK-
> > > 8171181 at http://cr.openjdk.java.net/~kkharbas/8171181/webrev.05
> > > I have lost the old email chain so had to start a fresh one. The 
> > > archived conversation can be found at - 
> > > http://mail.openjdk.java.net/ 
> > > pipermail/hotspot-runtime-dev/2017-March/022733.html
> > >
> > > 1.       I have worked on all the comments and fixed the bugs. 
> > > Mainly bugs fixed are related to sigprocmask() and changed the 
> > > implementation such that ‘fd’ is not passed all the way down the 
> > > call stack. Thus minimizing function signature changes.
> > >
> > > 2.       Patch supports all OS’es. Consolidated all Posix
> > > compliant 
> > > OS’s implementation in os_posix.cpp.
> > >
> > > 3.       The patch is tested on Windows and Linux. Working on 
> > > testing it on other OS’es.
> > >
> > > Let me know if this version looks clean and correct.
> > >
> > > Thanks
> > > Kishor
> > >
>
>