Todd Lipcon 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: (14 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 Good point. My original implementation only affected future allocations and not existing ones, so I wanted to call it as early as possible, but I went and did the /proc/maps thing so now it is fine to run a little later. 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 Done 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 a yep. http://gerrit.cloudera.org:8080/#/c/9676/1/src/kudu/util/process_memory.cc File src/kudu/util/process_memory.cc: http://gerrit.cloudera.org:8080/#/c/9676/1/src/kudu/util/process_memory.cc@96 PS1, Line 96: using std::pair; //NOLINT > warning: using decl 'pair' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/9676/1/src/kudu/util/process_memory.cc@98 PS1, Line 98: using std::vector; //NOLINT > warning: using decl 'vector' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/9676/1/src/kudu/util/process_memory.cc@244 PS1, Line 244: if (FLAGS_disable_hugepages == "auto") { > consider using boost::iequals here and below if you want to be a bit more l Done. Also added a FlagValidator because adar likes them a lot. http://gerrit.cloudera.org:8080/#/c/9676/1/src/kudu/util/process_memory.cc@284 PS1, Line 284: << reinterpret_cast<void*>(min_addr) << "-" > Consider using 'addr_range' directly instead of reconstructing it with cast Worth noting that addr_range has a format without '0x', so using 'addr_range' directly gives you: '7fe418822000-7fe418826000'. So if we want '0x' in there to make it more clearly hex addresses, we'd need to use min_max_addr_str.first and min_max_addr_str.second and add the '0x' manually. Still think it's worth it? Separately, I'll add an example line from 'maps' above this parsing code. http://gerrit.cloudera.org:8080/#/c/9676/1/src/kudu/util/process_memory.cc@417 PS1, Line 417: extern "C" void* end; > add a comment? Not clear what this is for. oh, that was me futzing with trying to find the heap without parsing /proc/self/maps. Forgot to delete. 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? Done 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"? Done 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? Good point. 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 e Done (here for the auto case). Do you think we should do it even when explicitly configured? I'm thinking that would be redundant since we already log any non-default flag values at startup. http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@282 PS2, Line 282: > Nit: extra space here Done http://gerrit.cloudera.org:8080/#/c/9676/2/src/kudu/util/process_memory.cc@417 PS2, Line 417: extern "C" void* end; > Unused? Done -- 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: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 16 Mar 2018 00:14:50 +0000 Gerrit-HasComments: Yes
