Use shared memory + spinlock & semaphore to perform IPC message passing in preload mode. by rwails · Pull Request #924 · shadow/shadow

rwails · 2020-08-13T21:20:39Z

No description provided.

plugin both implement the following IPC communication pattern: plugin: recv -> send -> recv -> send ... shadow: send -> recv -> send -> recv ... This commit patches a case where shared memory communication would sometimes cause the pattern: shadow: ... send -> send -> recv ... and vice-versa on the plugin. This commit also implements a more sophisticated spin lock/semaphore in the gate CPP module.

codecov · 2020-08-13T21:43:25Z

Codecov Report

Merging #924 into dev will decrease coverage by 0.02%.
The diff coverage is 20.52%.

@@            Coverage Diff             @@
##              dev     #924      +/-   ##
==========================================
- Coverage   55.74%   55.72%   -0.03%     
==========================================
  Files         135      137       +2     
  Lines       20957    21015      +58     
  Branches     4833     4837       +4     
==========================================
+ Hits        11683    11710      +27     
- Misses       6106     6130      +24     
- Partials     3168     3175       +7

Flag	Coverage Δ
#tests	`55.72% <20.52%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/main/host/thread_preload.c	`0.00% <0.00%> (ø)`
src/main/shmem/shmem_allocator.h	`0.00% <ø> (ø)`
src/shim/binary_spinning_sem.cc	`0.00% <0.00%> (ø)`
src/shim/binary_spinning_sem.h	`0.00% <0.00%> (ø)`
src/shim/ipc.cc	`0.00% <0.00%> (ø)`
src/shim/preload_syscalls.c	`21.00% <0.00%> (-0.11%)`	⬇️
src/shim/shim.c	`25.64% <0.00%> (-1.03%)`	⬇️
src/shim/shim_shmem.c	`0.00% <0.00%> (ø)`
src/main/shmem/shmem_test.c	`80.20% <77.77%> (-0.23%)`	⬇️
src/main/shmem/shmem_allocator.c	`70.83% <83.33%> (+0.83%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 523eebc...62fba80. Read the comment docs.

sporksmith · 2020-08-13T21:27:58Z

src/main/host/thread_preload.c

    SysCallHandler* sys;

-    int eventFD;
+    pid_t childPID;


The base Thread struct now has this

src/main/host/thread_preload.c

sporksmith · 2020-08-13T21:37:56Z

src/main/host/thread_preload.c

                };

-                shimevent_sendEvent(thread->eventFD, &ipc_complete_ev);
+                ShimEvent resp = {0};


Nit - move to after next statement, just before first use?

src/main/shmem/buddy.c

sporksmith · 2020-08-13T21:51:26Z

src/shim/CMakeLists.txt

 # -D_GNU_SOURCE enables some additional features in libc, such at RTLD_NEXT.
-target_compile_options(${SHIM_HELPER_LIB} PRIVATE -D_GNU_SOURCE)
+target_compile_options(${SHIM_HELPER_LIB} PRIVATE -D_GNU_SOURCE -fPIC)
+target_link_libraries(${SHIM_HELPER_LIB} PRIVATE shadow-shmem)


I think this should be INTERFACE? I don't think PRIVATE makes sense for a static library target, since its dependencies won't actually be linked into the library; its dependencies need to be included by the binary target(s) that use the library.

From https://cmake.org/cmake/help/v3.0/command/target_link_libraries.html?highlight=target_link_libraries :

The PUBLIC, PRIVATE and INTERFACE keywords can be used to specify both the link dependencies and the link interface in one command. Libraries and targets following PUBLIC are linked to, and are made part of the link interface. Libraries and targets following PRIVATE are linked to, but are not made part of the link interface. Libraries following INTERFACE are appended to the link interface and are not used for linking .

Changed to interface.

sporksmith · 2020-08-13T22:05:24Z

src/shim/gate.cc

+#define SHD_GATE_SPIN_MAX 8096
+
+void gate_init(Gate *gate) {
+  std::memset(gate, 0, sizeof(Gate));


IIRC memset'ing an object to zero is a bit of a gray area for type safety in the standard. It's probably fine in practice, but better to either use struct initialization to default-init the struct in a type-aware way (*gate = {0}), and/or explicitly init all members (gate->x is currently not initialized other than in this memset).

Fixed. n.b. C++ doesn't support the lovely = {0} syntax to zero all struct members.

sporksmith · 2020-08-13T22:15:57Z

src/shim/gate.h

+  pthread_spinlock_t lock;
+};
+
+void gate_init(Gate *gate);


I'm struggling a little bit with the "gate" metaphor. Could perhaps use a brief explanation (and/or a reference if it's a standard term, though I didn't have much luck searching)

Renamed to BinarySpinningSem

… -> binary_spinning_sem

src/main/host/thread_preload.c

sporksmith · 2020-08-19T21:47:39Z

src/main/shmem/buddy.c

 }

-#ifndef NDEBUG
-static void _print(void* pool, size_t pool_nbytes, void* meta) {


Maybe keep around in case you want it again for debugging? You could just #if 0 it out to avoid dead code warnings.

I think let's leave it out. The block is pretty old and probably only makes sense to me. I can dig it out of the history if needed.

sporksmith · 2020-08-19T22:41:31Z

src/shim/ipc.cc

 extern "C" {

 void ipcData_init(IPCData *ipc_data) {
    memset(ipc_data, 0, sizeof(IPCData));


As before, memset is a bit fragile here. Now I remember that this is especially the case for a C++ object, since a memset won't run the constructors of any member objects, if applicable.

As far as I can tell struct init does technically work here, but actually is also fragile due to c++ destuctors: if IPCData or any of its members have destructors, they'd run on the garbage data at ipc_data before calling IPCData's constructor (you haven't defined one, but IIRC there's implicitly one that default-initializes all members).

I think the idiomatic, safe way to do this is to replace IPCData::init with a constructor IPDCata::IPCData, and here use a placement new.

Demo: https://godbolt.org/z/df9Gjc

Alternatively, we could add a static assertion that IPCData (and therefore its members) doesn't have any nontrivial constructor or destructors, in which case the struct-init, or even the memset, should be safe: https://godbolt.org/z/c6nsTa

void ipcData_init(IPCData *ipc_data) { static_assert(std::is_trivial<IPCData>()); static_assert(std::is_trivially_destructible<IPCData>()); *ipc_data = {}; }

Good catch! I think memsetting a class has bitten me before. I had heard of placement new, but had forgotten its use since. Fixed!

sporksmith

Looks good! The coverage test is currently broken, so you can ignore that failure. @stevenengler is about to disable it.

You should be able to fix the bindings-lint check by running cd build; make bindings. (You may need to install bindgen and cbindgen first if you haven't already)

sporksmith · 2020-08-24T17:09:16Z

You should be able to fix the bindings-lint check by running cd build; make bindings. (You may need to install bindgen and cbindgen first if you haven't already)

Actually the binding failure is spurious too; will be fixed by #940

stevenengler · 2020-08-24T17:21:09Z

All of the CI issues should be resolved now.

Ryan Wails and others added 11 commits July 16, 2020 23:28

hacking in spinlock for IPC

2ced406

moving spinlock to cpp

1a8df97

adding sleepy

5ec70db

updates to spin code

dfc2989

removing all instances of fd / socket IPC

b71c4a6

moving gate to be local to IPC, spin -> ipc

471846f

removing sleepy external experiment

9d72dae

Merge branch 'master' into spinlock

fca656c

variable naming in gate

54a7948

using inline assembly to issue pause op

9097a03

rwails requested review from robgjansen and sporksmith August 13, 2020 21:20

sporksmith requested changes Aug 13, 2020

View reviewed changes

Ryan Wails added 3 commits August 19, 2020 14:27

Merge branch 'master' into spinlock

0b1da74

removing hacked in code during sem+spinlock IPC change, renaming gate…

91367d1

… -> binary_spinning_sem

fixing struct LOC placement, removing outdated buddy logging code

f6fa089

rwails requested a review from sporksmith August 19, 2020 21:39

sporksmith requested changes Aug 19, 2020

View reviewed changes

Ryan Wails added 2 commits August 24, 2020 12:23

using placement new for initialization

57c6ecb

Merge branch 'dev' into spinlock

fbced32

sporksmith approved these changes Aug 24, 2020

View reviewed changes

Merge branch 'dev' into spinlock

62fba80

rwails merged commit 6fba546 into shadow:dev Aug 26, 2020

robgjansen mentioned this pull request Sep 3, 2020

Use shared memory and spinlocks for Shadow-to-plugin IPC #894

Closed

4 tasks

Conversation

rwails commented Aug 13, 2020

Uh oh!

codecov bot commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sporksmith left a comment

Choose a reason for hiding this comment

Uh oh!

sporksmith commented Aug 24, 2020

Uh oh!

stevenengler commented Aug 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 13, 2020 •

edited

Loading