Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9676 )
Change subject: KUDU-1447. Automatically disable THP on process heap ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/master/master_main.cc File src/kudu/master/master_main.cc: http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/master/master_main.cc@72 PS2, Line 72: process_memory::MaybeDisableTHP(); If you put this in ServerBase or KuduServer, you won't have to duplicate it, and we'll get more coverage for it in internal mini cluster tests too. http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.h File src/kudu/util/process_memory.h: http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.h@47 PS2, Line 47: // If configured to do so, disable the use of transparent huge pages on all future Should probably doc that although this exists when not TCMALLOC_ENABLED, it's a no-op in that case. http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.h@49 PS2, Line 49: void MaybeDisableTHP(); This is safe to call at any point in the process lifetime, even if it has already started threads and allocated memory, right? http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc File src/kudu/util/process_memory.cc: http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@85 PS2, Line 85: DEFINE_string(disable_hugepages, "auto", Tag as advanced? http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@87 PS2, Line 87: below Below what? Or should this have been "RHEL 7 and below"? http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@198 PS2, Line 198: int save_errno = errno; Why is this needed? Doesn't a non-null 'addr' imply errno was reset to 0? http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@246 PS2, Line 246: VLOG(1) << "Based on running kernel, will " I think we should LOG(INFO) if we're going to disable THP. It's important enough that we'd like to know (when troubleshooting) if it was done. http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@282 PS2, Line 282: Nit: extra space here http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@417 PS2, Line 417: extern "C" void* end; Unused? -- To view, visit http://gerrit.cloudera.org:8080/9676 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e356466f0473546d52763123e7948f2e8756ceb Gerrit-Change-Number: 9676 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Mar 2018 00:03:41 +0000 Gerrit-HasComments: Yes
