-
Notifications
You must be signed in to change notification settings - Fork 58
Support for GNU Make jobserver #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
58cf674
b582346
2a9dbf6
32900a7
acbd532
1967630
2c0a5fa
07316b1
17926b6
1d9f35f
383db80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,11 +28,12 @@ struct job { | |
| bool failed; | ||
| }; | ||
|
|
||
| struct buildoptions buildopts = {.maxfail = 1}; | ||
| struct buildoptions buildopts = {.maxfail = 1, .gmakepipe = {-1, -1} }; | ||
| static struct edge *work; | ||
| static size_t nstarted, nfinished, ntotal; | ||
| static bool consoleused; | ||
| static struct timespec starttime; | ||
| static char gmaketokens[512], *gmakelatest = gmaketokens; | ||
|
|
||
| void | ||
| buildreset(void) | ||
|
|
@@ -43,6 +44,13 @@ buildreset(void) | |
| e->flags &= ~FLAG_WORK; | ||
| } | ||
|
|
||
| /* returns whether GNU make jobserver mode is enabled */ | ||
| static inline bool | ||
| isjobserverclient(void) | ||
| { | ||
| return buildopts.gmakepipe[0] >= 0 && buildopts.gmakepipe[1] >= 0; | ||
| } | ||
|
|
||
| /* returns whether n1 is newer than n2, or false if n1 is NULL */ | ||
| static bool | ||
| isnewer(struct node *n1, struct node *n2) | ||
|
|
@@ -319,6 +327,15 @@ jobstart(struct job *j, struct edge *e) | |
| warn("posix_spawn_file_actions_addclose:"); | ||
| goto err3; | ||
| } | ||
| if (isjobserverclient()) { | ||
| /* do not allow children to steal GNU/tokens */ | ||
| for (i = 0; i < 2; ++i) { | ||
| if ((errno = posix_spawn_file_actions_addclose(&actions, buildopts.gmakepipe[i]))) { | ||
| warn("posix_spawn_file_actions_addclose:"); | ||
| goto err3; | ||
| } | ||
| } | ||
| } | ||
| if (e->pool != &consolepool) { | ||
| if ((errno = posix_spawn_file_actions_addopen(&actions, 0, "/dev/null", O_RDONLY, 0))) { | ||
| warn("posix_spawn_file_actions_addopen:"); | ||
|
|
@@ -522,43 +539,91 @@ jobwork(struct job *j) | |
|
|
||
| /* queries the system load average */ | ||
| static double | ||
|
capezotte marked this conversation as resolved.
Outdated
|
||
| queryload(void) | ||
| reachedload(void) | ||
| { | ||
| #ifdef HAVE_GETLOADAVG | ||
| double load; | ||
|
|
||
| if (getloadavg(&load, 1) == -1) { | ||
| warn("getloadavg:"); | ||
| load = 100.0; | ||
| if (buildopts.maxload > 0) { | ||
| if (getloadavg(&load, 1) == -1) { | ||
| warn("getloadavg:"); | ||
| return true; | ||
| } | ||
| return load > buildopts.maxload; | ||
| } else { | ||
| return false; | ||
| } | ||
|
|
||
| return load; | ||
| #else | ||
| return 0; | ||
| return false; | ||
| #endif | ||
| } | ||
|
|
||
| /* returns remaining GNU/tokens */ | ||
| static ssize_t | ||
| returngmake(void) | ||
| { | ||
| ssize_t returned = 0; | ||
| if (isjobserverclient()) | ||
| if ((returned = write(buildopts.gmakepipe[1], gmaketokens, gmakelatest - gmaketokens)) >= 0) | ||
| gmakelatest = gmaketokens; | ||
| return returned; | ||
| } | ||
|
|
||
| static void | ||
| gmakeatexit(void) | ||
| { | ||
| if (returngmake() < 0) | ||
| warn("last write to jobserver:"); | ||
| } | ||
|
|
||
| static void | ||
| termsignal(int signum) | ||
| { | ||
| write(2, "terminating due to signal\n", 27); | ||
| (void)returngmake(); | ||
| _exit(128 + signum); | ||
| } | ||
|
|
||
| void | ||
| build(void) | ||
| { | ||
| struct sigaction sact = { 0 }; | ||
| struct job *jobs = NULL; | ||
| struct pollfd *fds = NULL; | ||
| size_t i, next = 0, jobslen = 0, maxjobs = buildopts.maxjobs, numjobs = 0, numfail = 0; | ||
| ssize_t gmakeread; | ||
| struct edge *e; | ||
|
|
||
| if (ntotal == 0) { | ||
| warn("nothing to do"); | ||
| return; | ||
| } | ||
|
|
||
| fds = xmalloc(sizeof(fds[0])); | ||
| fds[0].fd = buildopts.gmakepipe[0]; | ||
| fds[0].events = 0; | ||
| if (isjobserverclient()) { | ||
| fds[0].events &= POLLIN; | ||
| if (atexit(gmakeatexit) == 0) { | ||
| sact.sa_handler = termsignal; | ||
| sigaction(SIGTERM, &sact, NULL); | ||
| sigaction(SIGINT, &sact, NULL); | ||
| sigaction(SIGPIPE, &sact, NULL); | ||
| } else { | ||
| warn("unable to register an atexit() function"); | ||
| } | ||
| } | ||
|
|
||
| clock_gettime(CLOCK_MONOTONIC, &starttime); | ||
| formatstatus(NULL, 0); | ||
|
|
||
| nstarted = 0; | ||
| for (;;) { | ||
| /* limit number of of jobs based on load */ | ||
| if (buildopts.maxload) | ||
| maxjobs = queryload() > buildopts.maxload ? 1 : buildopts.maxjobs; | ||
| maxjobs = reachedload() ? 1 : buildopts.maxjobs; | ||
| /* limit number of jobs based on gmake tokens */ | ||
| if (isjobserverclient()) | ||
| maxjobs = maxjobs > 1 + (size_t)(gmakelatest - gmaketokens) ? 1 + (size_t)(gmakelatest - gmaketokens) : maxjobs; | ||
| /* start ready edges */ | ||
| while (work && numjobs < maxjobs && numfail < buildopts.maxfail) { | ||
| e = work; | ||
|
|
@@ -578,18 +643,19 @@ build(void) | |
| if (jobslen > buildopts.maxjobs) | ||
| jobslen = buildopts.maxjobs; | ||
| jobs = xreallocarray(jobs, jobslen, sizeof(jobs[0])); | ||
| fds = xreallocarray(fds, jobslen, sizeof(fds[0])); | ||
| fds = xreallocarray(fds, 1 + jobslen, sizeof(fds[0])); | ||
| for (i = next; i < jobslen; ++i) { | ||
| jobs[i].buf.data = NULL; | ||
| jobs[i].buf.len = 0; | ||
| jobs[i].buf.cap = 0; | ||
| jobs[i].next = i + 1; | ||
| fds[i].fd = -1; | ||
| fds[i].events = POLLIN; | ||
| fds[1+i].fd = -1; | ||
| fds[1+i].events = POLLIN; | ||
| } | ||
| } | ||
| fds[next].fd = jobstart(&jobs[next], e); | ||
| if (fds[next].fd < 0) { | ||
|
|
||
| fds[1+next].fd = jobstart(&jobs[next], e); | ||
| if (fds[1+next].fd < 0) { | ||
| warn("job failed to start"); | ||
| ++numfail; | ||
| } else { | ||
|
|
@@ -599,17 +665,30 @@ build(void) | |
| } | ||
| if (numjobs == 0) | ||
| break; | ||
| if (poll(fds, jobslen, 5000) < 0) | ||
| if (poll(fds, 1 + jobslen, 5000) < 0) | ||
| fatal("poll:"); | ||
| for (i = 0; i < jobslen; ++i) { | ||
| if (!fds[i].revents || jobwork(&jobs[i])) | ||
| if (!fds[1+i].revents || jobwork(&jobs[i])) | ||
| continue; | ||
| --numjobs; | ||
| jobs[i].next = next; | ||
| fds[i].fd = -1; | ||
| fds[1+i].fd = -1; | ||
| next = i; | ||
| if (jobs[i].failed) | ||
| ++numfail; | ||
| /* return a token */ | ||
| if (isjobserverclient() && gmakelatest > gmaketokens) { | ||
| if (write(buildopts.gmakepipe[1], --gmakelatest, 1) < 0) | ||
| warn("write to jobserver:"); | ||
| } | ||
| } | ||
| if (fds[0].revents & POLLIN && isjobserverclient() && nstarted < ntotal && !reachedload()) { | ||
| gmakeread = read(buildopts.gmakepipe[0], gmakelatest, buildopts.maxjobs - numjobs); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a race condition here:
The solution used by GNU Make is complex and relies on trapping SIGCHLD and dup-ing the file descriptor. It's complicated and it's why I just went for reading the jobserver file descriptor in a separate thread instead.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not even sure GNU Make's solution is 100% robust without an extra I really have to rethink this implementation.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading that paper about the GNU make implementation, I'm convinced that we must use threads (or some sort of async I/O) to implement this correctly on samurai. The The main issue is that you cannot predict whether a read on the jobserver fd will block or not. It doesn't matter for make since if it blocks, they have nothing to do anyway until a child exits, which can conveniently interrupt the read via Because of this, I'm going to close this PR in favor of #104. Thanks for your work getting started on this. |
||
| if (gmakeread < 0) { | ||
| warn("read from jobserver:"); | ||
| gmakeread = 0; | ||
| } | ||
| gmakelatest += gmakeread; | ||
| } | ||
| } | ||
| for (i = 0; i < jobslen; ++i) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <unistd.h> /* for chdir */ | ||
| #include <fcntl.h> /* for open */ | ||
| #include "arg.h" | ||
| #include "build.h" | ||
| #include "deps.h" | ||
|
|
@@ -90,6 +91,50 @@ jobsflag(const char *flag) | |
| buildopts.maxjobs = num > 0 ? num : -1; | ||
| } | ||
|
|
||
| static void | ||
| jobserverflags(const char *flag) | ||
| { | ||
| int rfd, wfd; | ||
|
|
||
| if (!flag) | ||
| return; | ||
| if (sscanf(flag, "%d,%d", &rfd, &wfd) == 2) { | ||
| /* prepare error message */ | ||
| errno = EBADF; | ||
|
capezotte marked this conversation as resolved.
Outdated
|
||
| } else if (strncmp(flag, "fifo:", 5) == 0) { | ||
| rfd = open(flag + 5, O_RDONLY); | ||
| wfd = open(flag + 5, O_WRONLY); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should just open it once as Also, we should print a message if this happens.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Print a message whenever a FIFO is used? |
||
| } else { | ||
| fatal("invalid jobserver parameter"); | ||
| } | ||
|
|
||
| buildopts.gmakepipe[0] = rfd; | ||
| buildopts.gmakepipe[1] = wfd; | ||
| warn("using GNU Make jobserver"); | ||
| } | ||
|
|
||
| static void | ||
| parsegmakeflags(char *env) { | ||
| char *arg; | ||
|
|
||
| if (!env) | ||
| return; | ||
| env = xmemdup(env, strlen(env) + 1); | ||
|
|
||
| arg = strtok(env, " "); | ||
| while (arg) { | ||
| if (arg[0] && arg[0] != '-' && strchr(arg, 'n')) { | ||
| buildopts.dryrun = true; | ||
| } else if (strncmp(arg, "-j", 2) == 0) { | ||
| jobsflag(arg + 2); | ||
| } else if (strncmp(arg, "--jobserver-auth=", 17) == 0 || strncmp(arg, "--jobserver-fds=", 16) == 0) { | ||
|
capezotte marked this conversation as resolved.
Outdated
|
||
| jobserverflags(strchr(arg, '=')+1); | ||
| } | ||
| arg = strtok(NULL, " "); | ||
| } | ||
| free(env); | ||
| } | ||
|
|
||
| static void | ||
| parseenvargs(char *env) | ||
| { | ||
|
|
@@ -149,6 +194,7 @@ main(int argc, char *argv[]) | |
|
|
||
| argv0 = progname(argv[0], "samu"); | ||
| parseenvargs(getenv("SAMUFLAGS")); | ||
| parsegmakeflags(getenv("MAKEFLAGS")); | ||
| ARGBEGIN { | ||
| case '-': | ||
| arg = EARGF(usage()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @trws asked, do we need to prevent children from interacting with the job server?
If we do, I think it'd be better to set
FD_CLOEXECon the fds than closing them here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why I thought of that in the first place (nowhere in the documentation is this recommended).
After testing for a bit, it seems GNU Make uses the same
--jobserver-authparameters for every client, so it's fine forsamuto share the jobserver with its own clients.