Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23925 )
Change subject: KUDU-3736 fix SIGSEGV in codegen with libgcc-11.5.0-10+ ...................................................................... Patch Set 2: (4 comments) > (2 comments) > > Thanks for working on this Alexey! > I haven't gone through the full change, I just have a clarification > question: Is this implementation susceptible to the same > scalability bottleneck that the b-tree was introduced to solve? This implementation isn't susceptible to the scalability bottleneck in former implementation of libgcc, no. This code isn't affecting that in any way. Also, since Kudu always runs only a single MCJIT compilation at a time (no concurrency there), that scalability bottleneck is not relevant in any way to Kudu itself. http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc File src/kudu/codegen/jit_frame_manager.cc: http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@95 PS2, Line 95: const size_t num_pages = (num_bytes + page_size - 1) / page_size; > Are section allocation requests to the memory mapper guaranteed to be page- Requests for section memory allocation do not come aligned to memory page boundary. However, that logic takes into account the actually allocated size, so it utilizes all the free memory left in every allocated chunk (you can see that in the implementation of SectionMemoryManager::allocateSection() method in lib/ExecutionEngine/SectionMemoryManager.cpp). With that, and the fact that there are just a few memory allocation requests for every data type section (RO, RW, Code), plus page-aligning the estimate for their sizes and doubling the size in JITFrameManager::reserveAllocationSpace(), there isn't a way to overrun the pre-allocated chunk. Even if it did, there is JITFrameManager::runPostAllocationCheck() that takes care of handling such a condition. http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@115 PS2, Line 115: ec = Memory::protectMappedMemory(result, protection_flags); > Is it guaranteed that protectMappedMemory() always performs icache invalida Yes, it is, and LLVM relies on this. Also, as for applicable platforms where Kudu runs, this is relevant only on aarch64. This code doesn't deviate in this regard from what LLVM does -- as you could see from the comment on this method, it's modeled after LLVM's code in lib/Support/Unix/Memory.inc http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@138 PS2, Line 138: JITFrameManager::~JITFrameManager() { > SectionMemoryManager’s destructor releases all AllocatedMem blocks. Here we This is intentional, and it's necessary since not all the pre-allocated space might be utilized by all AllocatedMem blocks. http://gerrit.cloudera.org:8080/#/c/23925/2/src/kudu/codegen/jit_frame_manager.cc@161 PS2, Line 161: const size_t code_required_size_bytes = : code_align * ((code_size + code_align - 1) / code_align); : const size_t ro_data_required_size_bytes = : ro_data_align * ((ro_data_size + ro_data_align - 1) / ro_data_align); : const size_t rw_data_required_size_bytes = : rw_data_align * ((rw_data_size + rw_data_align - 1) / rw_data_align); > Do we rely on LLVM always passing non?zero alignment values here? Yes, we do. It cannot pass 0 for any of the alignment numbers here, but adding an extra DCHECK() wouldn't hurt. -- To view, visit http://gerrit.cloudera.org:8080/23925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I691d2f442c3148f235847c4c8e56767577804b1a Gerrit-Change-Number: 23925 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Mon, 02 Feb 2026 20:22:12 +0000 Gerrit-HasComments: Yes
