Page MenuHomePhabricator

[JITLink][Orc] Add MemoryMapper interface with POSIX shared memory implementation
AbandonedPublicDraft

Authored by argentite on Jun 6 2022, 12:53 PM.

Details

Reviewers
None
Summary

MemoryMapper interface provides 3 functions:
+ reserve() reserves address space in executor process and working

memory in the controller process

+ finalize() ensures working memory is transferred to executor process

with appropriate memory protections

+ release() deallocates memory on both side

SharedMemoryMapper implementation uses POSIX shared memory APIs to map
shared memory pages in both processes at reserve(). finalize()
only sets the memory protections and release() unmaps the memory.
Executor side is implemented with ExecutorSharedMemoryManager.

Diff Detail

Unit TestsFailed

TimeTest
60,130 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,120 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp

Event Timeline

argentite created this revision.Jun 6 2022, 12:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 12:53 PM
lhames added a subscriber: lhames.Jun 6 2022, 5:53 PM

I think it's worth adding an InProcessMapper too. That can be defined in ORC alongside the Mapper interface, and would give us a good way to test the API design separate from the shared memory details.

llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
30

I'd leave out the implicit cast here -- only people writing new mappers should need to worry about this, and it's fine (and safer) to make them spell out .RemoteAddr explicitly.

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryManager.h
29

I lazily re-used the MemoryManager suffix for SimpleExecutorMemoryManager when I wrote it, but I think that was a mistake: it makes it harder to understand the difference between this "memory manager" and the JITLinkMemoryManager.

Maybe ExecutorSharedMemoryMapperService would be a better name for this. What do you think?

33

This operation should be renamed to reserve. We want to be clear that there will no longer be a 1-1 mapping between the alloc calls on our JITLinkMemoryManager and calls to ExecutorSharedMemoryManager::reserve.

We can start off that way (depending on whether you want to land this first patch earlier or later), but the eventual aim is that our new JITLinkMemoryManager's alloc method should look like this:

void allocate(const jitlink::JITLinkDylib *JD, jitlink::LinkGraph &G,
  OnAllocatedFunction OnAllocated) override {
  if (!haveSpaceForGraph(G))
    if (auto Err = reserveMoreSpace()) // RPC to ExecutorSharedMemoryManager::reserve.
      return Err;
  // Space is guaranteed to be available once we get here.
  return allocFromReservedSpace(JD, G, std::move(OnAllocated));
}
llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryManager.cpp
35

I think we want to #ifdef out this entire class if LLVM_ON_UNIX is not set.

For now I'd literally wrap all the methods, and have a second set of implementations with llvm_unreachable for Windows. If the file gets unwieldy we can move to conditional includes like in https://github.com/llvm/llvm-project/blob/eb68cbb405512b448a2f3336e102982c144ad4e9/llvm/lib/Support/Memory.cpp#L21.

73

This looks like it was derived from SimpleExecutorMemoryManager -- I think we can actually shoot for something simpler in this new scheme where we separate the slab from the individual allocations within the slab.

Within ExecutorSharedMemoryManager you'll want to track two things: the slabs (which we'll just treat as pure memory ranges to be freed later), and allocations (finalized ranges within a slab with associated deallocation actions).

In this new scheme the finalize method just needs to apply protections, run finalization actions, and (on success) record the deallocation actions. We don't need to deallocate anything on failure, so we no longer need the complex bailout logic.

I think that this can simplify to:

Error ExecutorSharedMemoryManager::finalize(
  tpctypes::SharedMemoryFinalizeRequest &FR) {

  ExecutorAddr Base(~0ULL);
  for (auto &Seg : FR.Segments)
    Base = std::min(Base, Seg.Addr);

  // Future TODO -- scope exit to restore permissions (and ideally disassociate
  //   physical pages) on any subsequent errors.
  // --- Apply segment memory protections here ---

  auto DeallocActions = runFinalizeActions(FR.Actions);
  if (!DeallocActions)
    return DeallocActions.takeError();

  // Record allocation:
  Allocation[Base] = std::move(*DeallocActions);

  return Error::success();
}
argentite updated this revision to Diff 434808.Jun 7 2022, 7:14 AM
argentite marked 2 inline comments as done.

Remove implicit ExecutorAddr cast from Mapping
Renamed allocate to reserve and deallocate to release

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryManager.h
29

How about just ExecutorSharedMemoryService? Besides mapping pages, it also needs to keep track of allocation and their actions as well.

lhames added inline comments.Jun 7 2022, 10:43 AM
llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryManager.h
29

Hrm. I thought ExecutorSharedMemoryMapperService to make clear that it's the executor-side implementation of the ShardedMemoryMapper class.

Maybe we need a better name than Mapper?

I think go with whichever name suits you best for now and we can revisit the question later.

sunho added a subscriber: sunho.Jun 8 2022, 5:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Aug 8, 3:22 AM