Support for GNU Make jobserver #94

capezotte · 2022-10-28T17:05:12Z

Quick and dirty implementation of #71, following the GNU Make 4.3 manual (Section 13.1.

TODO:

-l and -j are ignored when jobserver options are active, though CMake team's proposed patch for Ninja instead ignores jobserver options whenever -j is given in the command line. Should we follow them?
Aborts whenever given jobserver FDs are closed/invalid. Should it be downgraded to warning?
Make sure it works with older GNU Make (pre-4.2, with different option names).
The fifo: protocol mentioned on Support make jobserver protocol #71.
Tokens should be returned to GNU Make when the program terminates. This is probably the biggest issue with this PR, but I'd like feedback on the "core" of the changes before embarking on rewriting fatal and installing signal handlers.

michaelforney · 2022-10-31T01:51:19Z

This looks really good, thanks for working on it.

Some still-relevant distros ship with older GNU Make (pre-4.2, with different option names).

Can you elaborate? What happens if samu gets run with an old GNU make? Does it break, or just not work?

Tokens aren't returned to GNU Make when the program terminates.

Can we install an atexit function to clean them up?

capezotte · 2022-10-31T12:58:33Z

Can you elaborate? What happens if samu gets run with an old GNU make? Does it break, or just not work?

Old GNU make uses --jobserver-fds= instead of --jobserver-auth= in the environment. The protocol isn't documented in the old version of manual, so I was unsure.

After some testing and a cursory reading of the source code, it seems that, other than that, it's the same.

Can we install an atexit function to clean them up?

Good idea, just implemented it and tokens are returned after fatal and after a normal exit.

build.c

thesamesam · 2023-05-07T09:39:15Z

@michaelforney are you able to take a look? thank you!

michaelforney · 2023-06-18T07:11:57Z

@thesamesam There are several outstanding comments that still need to be resolved.

trws · 2023-11-28T21:23:49Z

What remains to be done here? Sorry for the drive-by comment, but I've been looking for a ninja alternative with this feature to use with our package manager, which allows doing a build of an entire tree of packages under a single jobserver. If I could help out a bit, I'd be happy to.

One note, this line: https://github.com/michaelforney/samurai/pull/94/files#diff-99b168f07bbb2d0e68bfe3cd8378543a6becd5989922c5283e499d1f69879b42R324
mentions children "stealing" tokens. If possible, it would be best to allow children to take tokens. This implementation allows them to with the gnu make 4.3 style interface, but not the old one. Reason being that things like cargo, spack, make, ld.gold and others can use that for their own parallelism instead of doing their own fanout and over-loading the system.

On a related note, would you like to have jobserver server support here as well?

michaelforney · 2024-02-23T21:02:44Z

What remains to be done here? Sorry for the drive-by comment, but I've been looking for a ninja alternative with this feature to use with our package manager, which allows doing a build of an entire tree of packages under a single jobserver. If I could help out a bit, I'd be happy to.

See the unresolved comments. In particular, the one about polling. There should be a single poll in the event loop.

I pushed my current working tree to the jobserver branch. I don't remember the state, but if someone else wants to help out with this, that'd be a good place to start. Edit: on second thought, maybe not. It seems that my local changes don't even compile.

michaelforney

Well, turns out that the comments I thought I made were never submitted. My apologies.

build.c

michaelforney · 2022-10-31T01:47:05Z

samu.c

+	arg = strtok(env, " ");
+	/* first word might contain dry run */
+	if (arg && strchr(arg, 'n'))
+		buildopts.dryrun = true;


Can you explain this? As far as I can tell, the first word is supposed to be the single-character options, right? I see that by default it is just w. However, if I run make --no-print-directory, it is empty with just a blank space, so I think we might accidentally skip over the jobserver flags.

Another question: how is samu running in the first place if make is in dry run mode?

That does seem to be what the first word is for, though the protocol documentation only goes as far as saying "check the first word for the letter n". Thanks for catching the oversight.

As for the second question, GNU Make (version 4.4.1 my system) invokes jobserver clients even in dry run mode.

samu.c

michaelforney · 2022-10-31T01:48:42Z

samu.c

+		/* assert valid fds */
+		check[0].fd = read_end;
+		check[1].fd = write_end;
+		if (poll(check, 2, 0) == -1 || check[0].revents & POLLNVAL || check[1].revents & POLLNVAL) {


I don't think we should do a validity check like this. If the fds are invalid that's the caller's problem, and I'd like to minimize the POSIX usage as much as possible.

The documentation recommends doing the check. As it's there mostly to address a PEBKAC (user forgot a + in the Makefile), I do understand if you want to drop it.

build.c

michaelforney · 2024-02-23T21:47:44Z

One question I have, should a jobserver client request a token for every job it starts and release it when its done? Or is it fine if samurai takes as many jobs as it can use, and release them only when it has extra tokens that it can't use?

I don't think it would make a difference, except maybe collective ordering of the jobs run.

eli-schwartz · 2024-02-23T22:06:51Z

If it's going to immediately reuse those tokens then I think it's fine.

michaelforney · 2024-02-23T22:16:32Z

build.c

@@ -610,6 +672,13 @@ build(void)
 			next = i;
 			if (jobs[i].failed)
 				++numfail;
+			/* must return the GNU/tokens once a job is done */
+			if (buildopts.gmakepipe[1] > 0 && gmakelatest != gmaketokens) {
+				if (write(buildopts.gmakepipe[1], --gmakelatest, 1) < 0) {


I'm not sure if it matters, but this might not be the same job token that we acquired to start this job. Maybe we should track the job token in the job struct?

I don't think this is necessary, it's not like make knows how exactly the job slots are used by the client. In fact, despite documentation saying "don’t assume that all tokens are the same character", in my testing they usually are.

samu.c

michaelforney · 2024-04-06T06:32:26Z

samu.c

+		errno = EBADF;
+	} else if (strncmp(flag, "fifo:", 5) == 0) {
+		rfd = open(flag + 5, O_RDONLY);
+		wfd = open(flag + 5, O_WRONLY);


You should just open it once as O_RDWR and set both rfd and wfd to the same value.

Also, we should print a message if this happens.

Print a message whenever a FIFO is used?

samu.c

michaelforney · 2024-04-06T06:46:04Z

build.c

@@ -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 */


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_CLOEXEC on the fds than closing them here.

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-auth parameters for every client, so it's fine for samu to share the jobserver with its own clients.

build.c

fix return type for reachedload avoid needlessly changing errno allow children to take tokens use only one fd with 'fifo:' authentication type fix |=/&= mixup

bonzini · 2024-04-19T13:34:09Z

There should be a single poll in the event loop.

My implementation was completely different and I have now refreshed it. It used a separate thread to read from the GNU Make pipe, and a local pipe between the thread and build() to integrate with the event loop.

bonzini · 2024-04-19T13:48:57Z

The main issue with this implementation is that if you have make -j26 and your build is

      a     b     c                large files that take a huge time
       `----+----'
            d
  ,----.----+--------.
 e     f    g  ....   z            small files that take a small time

then samu will take all 26 tokens and block other users of the jobserver pipe. Instead, tokens must be taken only when jobs are started.

eli-schwartz · 2024-04-19T14:24:01Z

@bonzini I haven't looked at the implementation at all. But, based on the previous review comment:

One question I have, should a jobserver client request a token for every job it starts and release it when its done? Or is it fine if samurai takes as many jobs as it can use, and release them only when it has extra tokens that it can't use?

I had been under the assumption that in the case you describe, samu would relinquish 23 tokens as soon as it detects it is blocked on a/b/c and can't start d.

bonzini · 2024-04-19T17:41:38Z

The only write system call that I see in the jobserver branch is

+                       /* must return the GNU/tokens once a job is done */
+                       if (buildopts.gmakepipe[1] > 0 && gmakelatest != gmaketokens) {
+                               if (write(buildopts.gmakepipe[1], --gmakelatest, 1) < 0) {
+                                       warn("write to jobserver:");
+                               }
+                               --maxjobs;
+                       }

so I don't think it does, though of course I might be wrong.

bonzini · 2024-04-19T17:47:42Z

build.c

+			}
+		}
+		if (fds[0].revents & POLLIN && isjobserverclient() && nstarted < ntotal && !reachedload()) {
+			gmakeread = read(buildopts.gmakepipe[0], gmakelatest, buildopts.maxjobs - numjobs);


There is a race condition here:

poll returns POLLIN

another process steals the token that poll detected

read blocks

one or more children of samurai finish

the whole build cannot reuse the token(s) currently owned by samurai

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.

I'm not even sure GNU Make's solution is 100% robust without an extra (void)alarm(1);, or only starting a new process as an absolute last resort.

I really have to rethink this implementation.

capezotte · 2024-04-20T17:49:50Z

I'm starting to think of an in-between approach of having the job spawn a helper program that waits for a token, feeds it back to samurai through a second pipe (so it's stored in the associated job struct, or the gmakelatest in this PR), and execs the actual job. Only samurai would have read access to this second pipe, so there is no way of POLLIN getting outdated before read.

Once I have more time I might rework this PR into this design, but I'd like for feedback.

bonzini · 2024-05-03T09:47:18Z

That seems similar to what I am doing, just with a process instead of a thread. You could also use fork() to do the wait, instead of using a helper program.

However, there would probably be more complications in returning the tokens on a call to fatal(). samurai does not attempt to kill child processes in that case, instead leaving them to die because of a broken pipe if they write anything to stdout/stderr; but in this case the children would also survive until they get a token. This would be untidy. And also, to ensure that the token is put back if the parent is dead, you would have to catch SIGPIPE (or temporarily ignore it so that you get EPIPE).

As an aside, it' s ok not to return the tokens on SIGINT/SIGTERM. I tried using a Makefile like

test-parent:
	$(MAKE) -f parent-makefile test-child1 test-child2 test-child3

test-child%:

Then invoking make -j3 -Onone and killing the child make in another terminal. Both SIGKILL and SIGTERM have the same behavior and show:

make[1]: *** [Makefile:10: test-child1] Terminated
make[1]: *** [Makefile:10: test-child3] Terminated
make[1]: *** [Makefile:10: test-child2] Terminated
make: *** [Makefile:7: test-parent] Terminated
make: INTERNAL: Exiting with 1 jobserver tokens available; should be 3!

So it's not a huge deal. But if you want to do it, a better way is to enhance samurai so that it traps SIGTERM (#106), killing all the children in response. The basic idea would be as simple as

diff --git a/build.c b/build.c
index 65305d2..877d04c 100644
--- a/build.c
+++ b/build.c
@@ -494,6 +494,9 @@ jobwork(struct job *j)
 	size_t newcap;
 	ssize_t n;
 
+	if (killall)
+		goto kill;
+
 	if (j->buf.cap - j->buf.len < BUFSIZ / 2) {
 		newcap = j->buf.cap + BUFSIZ;
 		newdata = realloc(j->buf.data, newcap);
@@ -560,7 +563,7 @@ build(void)
 		if (buildopts.maxload)
 			maxjobs = queryload() > buildopts.maxload ? 1 : buildopts.maxjobs;
 		/* start ready edges */
-		while (work && numjobs < maxjobs && numfail < buildopts.maxfail) {
+		while (work && !killall && numjobs < maxjobs && numfail < buildopts.maxfail) {
 			e = work;
 			work = work->worknext;
 			if (e->rule != &phonyrule && buildopts.dryrun) {
@@ -616,10 +619,12 @@ build(void)
 		}
 		if (numjobs == 0)
 			break;
-		if (poll(fds, jobslen, 5000) < 0)
+		if (poll(fds, jobslen, 5000) < 0 && errno != EINTR)
 			fatal("poll:");
 		for (i = 0; i < jobslen; ++i) {
-			if (!fds[i].revents || jobwork(&jobs[i]))
+			if (!killall && !fds[i].revents)
+				continue;
+			if (jobwork(&jobs[i]))
 				continue;
 			--numjobs;
 			jobs[i].next = next;

where the killall variable is set by the signal handler. By terminating all jobs cleanly, the tokens would also be returned.

support for gnu make jobserver

58cf674

close jobserver fds before exec, return tokens on normal exit

b582346

install a signal handler for returning tokens

2a9dbf6

capezotte marked this pull request as ready for review October 31, 2022 16:19

nekopsykose reviewed Jan 31, 2023

View reviewed changes

build.c Outdated Show resolved Hide resolved

capezotte added 4 commits June 18, 2023 09:14

Merge branch 'michaelforney:master' into jobserver

32900a7

avoid calling printf in a signal handler

acbd532

support fifo: jobserver authentication

1967630

make parsejobserver more straightforward

2c0a5fa

michaelforney reviewed Feb 23, 2024

View reviewed changes

capezotte added 3 commits March 31, 2024 20:40

fix oversight in option handling + style

07316b1

use -j from makeflags as maxjobs

17926b6

use a single poll

1d9f35f

capezotte force-pushed the jobserver branch from 2cd291f to 1d9f35f Compare April 4, 2024 23:56

michaelforney reviewed Apr 6, 2024

View reviewed changes

misc fixes

383db80

fix return type for reachedload avoid needlessly changing errno allow children to take tokens use only one fd with 'fifo:' authentication type fix |=/&= mixup

capezotte force-pushed the jobserver branch from b3b77b7 to 383db80 Compare April 6, 2024 12:46

bonzini mentioned this pull request Apr 19, 2024

Support for GNU Make jobserver (alternative implementation) #104

Open

bonzini reviewed Apr 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for GNU Make jobserver #94

Support for GNU Make jobserver #94

capezotte commented Oct 28, 2022 •

edited

Loading

michaelforney commented Oct 31, 2022

capezotte commented Oct 31, 2022

thesamesam commented May 7, 2023

michaelforney commented Jun 18, 2023

trws commented Nov 28, 2023

michaelforney commented Feb 23, 2024 •

edited

Loading

michaelforney left a comment

michaelforney Oct 31, 2022

capezotte Mar 31, 2024

michaelforney Oct 31, 2022

capezotte Mar 31, 2024

michaelforney commented Feb 23, 2024

eli-schwartz commented Feb 23, 2024

michaelforney Feb 23, 2024

capezotte Apr 1, 2024 •

edited

Loading

michaelforney Apr 6, 2024

capezotte Apr 6, 2024

michaelforney Apr 6, 2024

capezotte Apr 6, 2024

bonzini commented Apr 19, 2024

bonzini commented Apr 19, 2024 •

edited

Loading

eli-schwartz commented Apr 19, 2024

bonzini commented Apr 19, 2024

bonzini Apr 19, 2024

capezotte Apr 20, 2024

capezotte commented Apr 20, 2024

bonzini commented May 3, 2024 •

edited

Loading

Support for GNU Make jobserver #94

Are you sure you want to change the base?

Support for GNU Make jobserver #94

Conversation

capezotte commented Oct 28, 2022 • edited Loading

michaelforney commented Oct 31, 2022

capezotte commented Oct 31, 2022

thesamesam commented May 7, 2023

michaelforney commented Jun 18, 2023

trws commented Nov 28, 2023

michaelforney commented Feb 23, 2024 • edited Loading

michaelforney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelforney commented Feb 23, 2024

eli-schwartz commented Feb 23, 2024

Choose a reason for hiding this comment

capezotte Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonzini commented Apr 19, 2024

bonzini commented Apr 19, 2024 • edited Loading

eli-schwartz commented Apr 19, 2024

bonzini commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capezotte commented Apr 20, 2024

bonzini commented May 3, 2024 • edited Loading

capezotte commented Oct 28, 2022 •

edited

Loading

michaelforney commented Feb 23, 2024 •

edited

Loading

capezotte Apr 1, 2024 •

edited

Loading

bonzini commented Apr 19, 2024 •

edited

Loading

bonzini commented May 3, 2024 •

edited

Loading