Conversation
process creation. This change has a significant impact on experiment performance with many (> 1000) processes to manage. Initialization runtime is reduced by a large factor. `prctl(PR_SET_PDEATHSIG, SIGKILL)` and `getppid()` have been moved to the shim in `_shim_load()`. n.b. I believe this change does not affect ptrace mode. The `prctl` call may be duplicated, but shouldn't hurt for now.
Codecov Report
@@ Coverage Diff @@
## dev #1059 +/- ##
==========================================
- Coverage 55.07% 55.05% -0.02%
==========================================
Files 127 127
Lines 19231 19246 +15
Branches 4592 4595 +3
==========================================
+ Hits 10591 10596 +5
- Misses 5871 5878 +7
- Partials 2769 2772 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
robgjansen
left a comment
There was a problem hiding this comment.
A couple of comments would be helpful :)
| error("parent (shadow) exited"); | ||
| return -1; | ||
| } | ||
| int rc = execvpe(file, argv, envp); |
There was a problem hiding this comment.
I think we want a comment somewhere around here about how the code is designed so that the child process does not execute any code after the vfork and before calling exec, to guard against future programmers changing this code from accidentally causing undefined behavior.
src/shim/shim.c
Outdated
| error("SHADOW_PID does not contain an unsigned: %s", shadow_pid_str); | ||
| } else { | ||
|
|
||
| assert((pid_t)tmp_pid == tmp_pid); |
There was a problem hiding this comment.
I think it's to validate it wasn't truncated in the conversion to pid_t? +1 that it could use a comment
It'd be nice if we could parse into a pid_t directly, but I suppose that's tricky since we can't know the format specifier to provide to sscanf (without assuming something about the definition of pid_t).
There was a problem hiding this comment.
OTOH we could just not do this downcast, and compare directly. C will widen the pid if it's narrower.
unsigned long long shadow_pid = 0;
if (sscanf(shadow_pid_str, "%llu", &shadow_pid) != 1) {
...
// Implicitly widen return value of getppid
if (getppid() != shadow_pid) {
Btw this control flow would probably be a bit easier to follow by using early exits to avoid excessive nesting.
if (/* something failed */) {
/* log and exit */
}
if (/* something else failed */) {
/* log and exit */
}
| char* const envp[]) { | ||
| pid_t shadow_pid = getpid(); | ||
| pid_t pid = fork(); | ||
| pid_t pid = vfork(); |
There was a problem hiding this comment.
Maybe add a short comment about why we are using vfork since moving away from it isn't a decision that should be taken lightly.
src/shim/shim.c
Outdated
| error("SHADOW_PID does not contain an unsigned: %s", shadow_pid_str); | ||
| } else { | ||
|
|
||
| assert((pid_t)tmp_pid == tmp_pid); |
There was a problem hiding this comment.
I think it's to validate it wasn't truncated in the conversion to pid_t? +1 that it could use a comment
It'd be nice if we could parse into a pid_t directly, but I suppose that's tricky since we can't know the format specifier to provide to sscanf (without assuming something about the definition of pid_t).
1. Added comments warning about code between vfork + exec. 2. Noted that vfork has better performance in comments 3. Cleaned up the control flow of the parent-PID check in the shim.
Changes thread_preload.c to use
vfork()instead offork()duringprocess creation. This change has a significant impact on experiment
performance with many (> 1000) processes to manage.
Initialization runtime is reduced by a large factor.
prctl(PR_SET_PDEATHSIG, SIGKILL)andgetppid()have been moved tothe shim in
_shim_load().n.b. I believe this change does not affect ptrace mode. The
prctlcallmay be duplicated, but shouldn't hurt for now.