[PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made

[PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made

[Kees Cook]

Hi,

As Rasmus noted[1], there were some deficiencies in how the Make jobserver
vs sphinx parallelism logic was handled. This series attempts to address
all those problems by building a set of wrappers and fixing some of the
internal logic.

Thank you Rasmus for the suggestions (and the "jobhog" example)! :)

-Kees

[1] https://lore.kernel.org/lkml/eb25959a-9ec4-3530-2031-d9d716b40b20@rasmusvillemoes.dk

Kees Cook (3):
  docs, parallelism: Fix failure path and add comment
  docs, parallelism: Do not leak blocking mode to writer
  docs, parallelism: Rearrange how jobserver reservations are made

 Documentation/Makefile                   |  5 +-
 Documentation/sphinx/parallel-wrapper.sh | 25 +++++++++
 scripts/jobserver-count                  | 58 --------------------
 scripts/jobserver-exec                   | 69 ++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 61 deletions(-)
 create mode 100644 Documentation/sphinx/parallel-wrapper.sh
 delete mode 100755 scripts/jobserver-count
 create mode 100644 scripts/jobserver-exec

-- 
2.17.1

[PATCH 1/3] docs, parallelism: Fix failure path and add comment

[Kees Cook]

Rasmus noted that the failure path didn't correctly exit. Fix this and
add another comment about GNU Make's job server environment variable
names over time.

Reported-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Link: https://lore.kernel.org/lkml/eb25959a-9ec4-3530-2031-d9d716b40b20@rasmusvillemoes.dk
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/jobserver-count | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/jobserver-count b/scripts/jobserver-count
index 0b482d6884d2..6e15b38df3d0 100755
--- a/scripts/jobserver-count
+++ b/scripts/jobserver-count
@@ -24,6 +24,8 @@ try:
 	flags = os.environ['MAKEFLAGS']
 
 	# Look for "--jobserver=R,W"
+	# Note that GNU Make has used --jobserver-fds and --jobserver-auth
+	# so this handles all of them.
 	opts = [x for x in flags.split(" ") if x.startswith("--jobserver")]
 
 	# Parse out R,W file descriptor numbers and set them nonblocking.
@@ -53,6 +55,7 @@ os.write(writer, jobs)
 # If the jobserver was (impossibly) full or communication failed, use default.
 if len(jobs) < 1:
 	print(default)
+	sys.exit(0)
 
 # Report available slots (with a bump for our caller's reserveration).
 print(len(jobs) + 1)
-- 
2.17.1

[PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer

[Kees Cook]

Setting non-blocking via a local copy of the jobserver file descriptor
is safer than just assuming the writer on the original fd is prepared
for it to be non-blocking.

Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Link: https://lore.kernel.org/lkml/44c01043-ab24-b4de-6544-e8efd153e27a@rasmusvillemoes.dk
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/jobserver-count | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/scripts/jobserver-count b/scripts/jobserver-count
index 6e15b38df3d0..a68a04ad304f 100755
--- a/scripts/jobserver-count
+++ b/scripts/jobserver-count
@@ -12,12 +12,6 @@ default="1"
 if len(sys.argv) > 1:
 	default=sys.argv[1]
 
-# Set non-blocking for a given file descriptor.
-def nonblock(fd):
-	flags = fcntl.fcntl(fd, fcntl.F_GETFL)
-	fcntl.fcntl(fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
-	return fd
-
 # Extract and prepare jobserver file descriptors from envirnoment.
 try:
 	# Fetch the make environment options.
@@ -31,8 +25,13 @@ try:
 	# Parse out R,W file descriptor numbers and set them nonblocking.
 	fds = opts[0].split("=", 1)[1]
 	reader, writer = [int(x) for x in fds.split(",", 1)]
-	reader = nonblock(reader)
-except (KeyError, IndexError, ValueError, IOError):
+	# Open a private copy of reader to avoid setting nonblocking
+	# on an unexpecting writer.
+	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
+	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
+	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)
+except (KeyError, IndexError, ValueError, IOError, OSError) as e:
+	print(e, file=sys.stderr)
 	# Any missing environment strings or bad fds should result in just
 	# using the default specified parallelism.
 	print(default)
-- 
2.17.1

[PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made

[Kees Cook]

Rasmus correctly observed that the existing jobserver reservation only
worked if no other build targets were specified. The correct approach
is to hold the jobserver slots until sphinx has finished. To fix this,
the following changes are made:

- refactor (and rename) scripts/jobserver-exec to set an environment
  variable for the maximally reserved jobserver slots and exec a
  child, to release the slots on exit.

- create Documentation/scripts/parallel-wrapper.sh which examines both
  $PARALLELISM and the detected "-jauto" logic from Documentation/Makefile
  to decide sphinx's final -j argument.

- chain these together in Documentation/Makefile

Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Link: https://lore.kernel.org/lkml/eb25959a-9ec4-3530-2031-d9d716b40b20@rasmusvillemoes.dk
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/Makefile                      |  5 +-
 Documentation/sphinx/parallel-wrapper.sh    | 25 +++++++
 scripts/{jobserver-count => jobserver-exec} | 73 ++++++++++++---------
 3 files changed, 68 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/sphinx/parallel-wrapper.sh
 rename scripts/{jobserver-count => jobserver-exec} (50%)
 mode change 100755 => 100644

diff --git a/Documentation/Makefile b/Documentation/Makefile
index ce8eb63b523a..30554a2fbdd7 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -33,8 +33,6 @@ ifeq ($(HAVE_SPHINX),0)
 
 else # HAVE_SPHINX
 
-export SPHINX_PARALLEL = $(shell perl -e 'open IN,"sphinx-build --version 2>&1 |"; while (<IN>) { if (m/([\d\.]+)/) { print "auto" if ($$1 >= "1.7") } ;} close IN')
-
 # User-friendly check for pdflatex and latexmk
 HAVE_PDFLATEX := $(shell if which $(PDFLATEX) >/dev/null 2>&1; then echo 1; else echo 0; fi)
 HAVE_LATEXMK := $(shell if which latexmk >/dev/null 2>&1; then echo 1; else echo 0; fi)
@@ -67,8 +65,9 @@ quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
       cmd_sphinx = $(MAKE) BUILDDIR=$(abspath $(BUILDDIR)) $(build)=Documentation/media $2 && \
 	PYTHONDONTWRITEBYTECODE=1 \
 	BUILDDIR=$(abspath $(BUILDDIR)) SPHINX_CONF=$(abspath $(srctree)/$(src)/$5/$(SPHINX_CONF)) \
+	$(PYTHON) $(srctree)/scripts/jobserver-exec \
+	$(SHELL) $(srctree)/Documentation/sphinx/parallel-wrapper.sh \
 	$(SPHINXBUILD) \
-	-j $(shell python $(srctree)/scripts/jobserver-count $(SPHINX_PARALLEL)) \
 	-b $2 \
 	-c $(abspath $(srctree)/$(src)) \
 	-d $(abspath $(BUILDDIR)/.doctrees/$3) \
diff --git a/Documentation/sphinx/parallel-wrapper.sh b/Documentation/sphinx/parallel-wrapper.sh
new file mode 100644
index 000000000000..a416dbfd2025
--- /dev/null
+++ b/Documentation/sphinx/parallel-wrapper.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Figure out if we should follow a specific parallelism from the make
+# environment (as exported by scripts/jobserver-exec), or fall back to
+# the "auto" parallelism when "-jN" is not specified at the top-level
+# "make" invocation.
+
+sphinx="$1"
+shift || true
+
+parallel="${PARALLELISM:-1}"
+if [ ${parallel} -eq 1 ] ; then
+	auto=$(perl -e 'open IN,"'"$sphinx"' --version 2>&1 |";
+			while (<IN>) {
+				if (m/([\d\.]+)/) {
+					print "auto" if ($1 >= "1.7")
+				}
+			}
+			close IN')
+	if [ -n "$auto" ] ; then
+		parallel="$auto"
+	fi
+fi
+exec "$sphinx" "-j$parallel" "$@"
diff --git a/scripts/jobserver-count b/scripts/jobserver-exec
old mode 100755
new mode 100644
similarity index 50%
rename from scripts/jobserver-count
rename to scripts/jobserver-exec
index a68a04ad304f..4593b2a1e36d
--- a/scripts/jobserver-count
+++ b/scripts/jobserver-exec
@@ -2,17 +2,16 @@
 # SPDX-License-Identifier: GPL-2.0+
 #
 # This determines how many parallel tasks "make" is expecting, as it is
-# not exposed via an special variables.
+# not exposed via an special variables, reserves them all, runs a subprocess
+# with PARALLELISM environment variable set, and releases the jobs back again.
+#
 # https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html#POSIX-Jobserver
 from __future__ import print_function
 import os, sys, fcntl, errno
-
-# Default parallelism is "1" unless overridden on the command-line.
-default="1"
-if len(sys.argv) > 1:
-	default=sys.argv[1]
+import subprocess
 
 # Extract and prepare jobserver file descriptors from envirnoment.
+jobs = b""
 try:
 	# Fetch the make environment options.
 	flags = os.environ['MAKEFLAGS']
@@ -30,31 +29,41 @@ try:
 	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
 	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
 	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)
-except (KeyError, IndexError, ValueError, IOError, OSError) as e:
-	print(e, file=sys.stderr)
+
+	# Read out as many jobserver slots as possible.
+	while True:
+		try:
+			slot = os.read(reader, 1)
+			jobs += slot
+		except (OSError, IOError) as e:
+			if e.errno == errno.EWOULDBLOCK:
+				# Stop at the end of the jobserver queue.
+				break
+			# If something went wrong, give back the jobs.
+			if len(jobs):
+				os.write(writer, jobs)
+			raise e
+except (KeyError, IndexError, ValueError, OSError, IOError) as e:
 	# Any missing environment strings or bad fds should result in just
-	# using the default specified parallelism.
-	print(default)
-	sys.exit(0)
+	# not being parallel.
+	pass
 
-# Read out as many jobserver slots as possible.
-jobs = b""
-while True:
-	try:
-		slot = os.read(reader, 1)
-		jobs += slot
-	except (OSError, IOError) as e:
-		if e.errno == errno.EWOULDBLOCK:
-			# Stop when reach the end of the jobserver queue.
-			break
-		raise e
-# Return all the reserved slots.
-os.write(writer, jobs)
-
-# If the jobserver was (impossibly) full or communication failed, use default.
-if len(jobs) < 1:
-	print(default)
-	sys.exit(0)
-
-# Report available slots (with a bump for our caller's reserveration).
-print(len(jobs) + 1)
+claim = len(jobs)
+if claim < 1:
+	# If the jobserver was (impossibly) full or communication failed
+	# in some way do not use parallelism.
+	claim = 0
+
+# Launch command with a bump for our caller's reserveration,
+# since we're just going to sit here blocked on our child.
+claim += 1
+
+os.unsetenv('MAKEFLAGS')
+os.environ['PARALLELISM'] = '%d' % (claim)
+rc = subprocess.call(sys.argv[1:])
+
+# Return all the actually reserved slots.
+if len(jobs):
+	os.write(writer, jobs)
+
+sys.exit(rc)
-- 
2.17.1

Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer

[Rasmus Villemoes]

On 21/11/2019 01.03, Kees Cook wrote:
> Setting non-blocking via a local copy of the jobserver file descriptor
> is safer than just assuming the writer on the original fd is prepared
> for it to be non-blocking.

This is a bit inaccurate. The fd referring to the write side of the pipe
is always blocking - it has to be, due to the protocol requiring you to
write back the tokens you've read, so you can't just drop a token on the
floor. But it's also rather moot, since the pipe will never hold
anywhere near 4096 bytes, let alone a (linux) pipe's default capacity of
64K.

But what we cannot do is change the mode of the open file description to
non-blocking for the read side, in case the parent make (or some sibling
process that has also inherited the same "struct file") expects it to be
blocking.

> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Link: https://lore.kernel.org/lkml/44c01043-ab24-b4de-6544-e8efd153e27a@rasmusvillemoes.dk
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  scripts/jobserver-count | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/jobserver-count b/scripts/jobserver-count
> index 6e15b38df3d0..a68a04ad304f 100755
> --- a/scripts/jobserver-count
> +++ b/scripts/jobserver-count
> @@ -12,12 +12,6 @@ default="1"
>  if len(sys.argv) > 1:
>  	default=sys.argv[1]
>  
> -# Set non-blocking for a given file descriptor.
> -def nonblock(fd):
> -	flags = fcntl.fcntl(fd, fcntl.F_GETFL)
> -	fcntl.fcntl(fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
> -	return fd
> -
>  # Extract and prepare jobserver file descriptors from envirnoment.
>  try:
>  	# Fetch the make environment options.
> @@ -31,8 +25,13 @@ try:
>  	# Parse out R,W file descriptor numbers and set them nonblocking.
>  	fds = opts[0].split("=", 1)[1]
>  	reader, writer = [int(x) for x in fds.split(",", 1)]
> -	reader = nonblock(reader)
> -except (KeyError, IndexError, ValueError, IOError):
> +	# Open a private copy of reader to avoid setting nonblocking
> +	# on an unexpecting writer.

s/writer/reader/

> +	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
> +	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
> +	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)

I think you can just specify O_NONBLOCK in the open() call so you avoid
those two fcntls.

Rasmus

Re: [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made

[Rasmus Villemoes]

On 21/11/2019 01.03, Kees Cook wrote:
> Rasmus correctly observed that the existing jobserver reservation only
> worked if no other build targets were specified. The correct approach
> is to hold the jobserver slots until sphinx has finished. To fix this,
> the following changes are made:
> 
> - refactor (and rename) scripts/jobserver-exec to set an environment
>   variable for the maximally reserved jobserver slots and exec a
>   child, to release the slots on exit.
> 
> - create Documentation/scripts/parallel-wrapper.sh which examines both
>   $PARALLELISM and the detected "-jauto" logic from Documentation/Makefile
>   to decide sphinx's final -j argument.
> 
> - chain these together in Documentation/Makefile
> 
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Link: https://lore.kernel.org/lkml/eb25959a-9ec4-3530-2031-d9d716b40b20@rasmusvillemoes.dk
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/Makefile                      |  5 +-
>  Documentation/sphinx/parallel-wrapper.sh    | 25 +++++++
>  scripts/{jobserver-count => jobserver-exec} | 73 ++++++++++++---------
>  3 files changed, 68 insertions(+), 35 deletions(-)
>  create mode 100644 Documentation/sphinx/parallel-wrapper.sh
>  rename scripts/{jobserver-count => jobserver-exec} (50%)
>  mode change 100755 => 100644
> 
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index ce8eb63b523a..30554a2fbdd7 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -33,8 +33,6 @@ ifeq ($(HAVE_SPHINX),0)
>  
>  else # HAVE_SPHINX
>  
> -export SPHINX_PARALLEL = $(shell perl -e 'open IN,"sphinx-build --version 2>&1 |"; while (<IN>) { if (m/([\d\.]+)/) { print "auto" if ($$1 >= "1.7") } ;} close IN')
> -
>  # User-friendly check for pdflatex and latexmk
>  HAVE_PDFLATEX := $(shell if which $(PDFLATEX) >/dev/null 2>&1; then echo 1; else echo 0; fi)
>  HAVE_LATEXMK := $(shell if which latexmk >/dev/null 2>&1; then echo 1; else echo 0; fi)
> @@ -67,8 +65,9 @@ quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
>        cmd_sphinx = $(MAKE) BUILDDIR=$(abspath $(BUILDDIR)) $(build)=Documentation/media $2 && \
>  	PYTHONDONTWRITEBYTECODE=1 \
>  	BUILDDIR=$(abspath $(BUILDDIR)) SPHINX_CONF=$(abspath $(srctree)/$(src)/$5/$(SPHINX_CONF)) \
> +	$(PYTHON) $(srctree)/scripts/jobserver-exec \
> +	$(SHELL) $(srctree)/Documentation/sphinx/parallel-wrapper.sh \
>  	$(SPHINXBUILD) \
> -	-j $(shell python $(srctree)/scripts/jobserver-count $(SPHINX_PARALLEL)) \
>  	-b $2 \
>  	-c $(abspath $(srctree)/$(src)) \
>  	-d $(abspath $(BUILDDIR)/.doctrees/$3) \
> diff --git a/Documentation/sphinx/parallel-wrapper.sh b/Documentation/sphinx/parallel-wrapper.sh
> new file mode 100644
> index 000000000000..a416dbfd2025
> --- /dev/null
> +++ b/Documentation/sphinx/parallel-wrapper.sh
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Figure out if we should follow a specific parallelism from the make
> +# environment (as exported by scripts/jobserver-exec), or fall back to
> +# the "auto" parallelism when "-jN" is not specified at the top-level
> +# "make" invocation.
> +
> +sphinx="$1"
> +shift || true
> +
> +parallel="${PARALLELISM:-1}"
> +if [ ${parallel} -eq 1 ] ; then
> +	auto=$(perl -e 'open IN,"'"$sphinx"' --version 2>&1 |";
> +			while (<IN>) {
> +				if (m/([\d\.]+)/) {
> +					print "auto" if ($1 >= "1.7")
> +				}
> +			}
> +			close IN')
> +	if [ -n "$auto" ] ; then
> +		parallel="$auto"
> +	fi
> +fi
> +exec "$sphinx" "-j$parallel" "$@"

I don't understand this logic. If the parent failed to claim any tokens
(likely because the top make and its descendants are already running 16
gcc processes), just let sphinx run #cpus jobs in parallel? That doesn't
make sense - it gets us back to the "we've now effectively injected K
tokens to the jobserver that weren't there originally".

From the comment above, what you want is to use "auto" if the top
invocation was simply "make docs". Well, I kind of disagree with falling
back to auto in that case; the user can say "make -j8 docs" and the
wrapper is guaranteed to claim them all. But if you really want, the
jobserver-count script needs to detect and export the "no parallelism
requested at top level" in some way distinct from "PARALLELISM=1",
because that's ambiguous.

> diff --git a/scripts/jobserver-count b/scripts/jobserver-exec
> old mode 100755
> new mode 100644
> similarity index 50%
> rename from scripts/jobserver-count
> rename to scripts/jobserver-exec
> index a68a04ad304f..4593b2a1e36d
> --- a/scripts/jobserver-count
> +++ b/scripts/jobserver-exec
> @@ -2,17 +2,16 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  #
>  # This determines how many parallel tasks "make" is expecting, as it is
> -# not exposed via an special variables.
> +# not exposed via an special variables, reserves them all, runs a subprocess
> +# with PARALLELISM environment variable set, and releases the jobs back again.
> +#
>  # https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html#POSIX-Jobserver
>  from __future__ import print_function
>  import os, sys, fcntl, errno
> -
> -# Default parallelism is "1" unless overridden on the command-line.
> -default="1"
> -if len(sys.argv) > 1:
> -	default=sys.argv[1]
> +import subprocess
>  
>  # Extract and prepare jobserver file descriptors from envirnoment.
> +jobs = b""
>  try:
>  	# Fetch the make environment options.
>  	flags = os.environ['MAKEFLAGS']
> @@ -30,31 +29,41 @@ try:
>  	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
>  	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
>  	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)
> -except (KeyError, IndexError, ValueError, IOError, OSError) as e:
> -	print(e, file=sys.stderr)
> +
> +	# Read out as many jobserver slots as possible.
> +	while True:
> +		try:
> +			slot = os.read(reader, 1)
> +			jobs += slot

I'd just try to slurp in 8 or 16 tokens at a time, there's no reason to
limit to 1 in each loop.

> +		except (OSError, IOError) as e:
> +			if e.errno == errno.EWOULDBLOCK:
> +				# Stop at the end of the jobserver queue.
> +				break
> +			# If something went wrong, give back the jobs.
> +			if len(jobs):
> +				os.write(writer, jobs)
> +			raise e
> +except (KeyError, IndexError, ValueError, OSError, IOError) as e:
>  	# Any missing environment strings or bad fds should result in just
> -	# using the default specified parallelism.
> -	print(default)
> -	sys.exit(0)
> +	# not being parallel.
> +	pass
>  
> -# Read out as many jobserver slots as possible.
> -jobs = b""
> -while True:
> -	try:
> -		slot = os.read(reader, 1)
> -		jobs += slot
> -	except (OSError, IOError) as e:
> -		if e.errno == errno.EWOULDBLOCK:
> -			# Stop when reach the end of the jobserver queue.
> -			break
> -		raise e
> -# Return all the reserved slots.
> -os.write(writer, jobs)
> -
> -# If the jobserver was (impossibly) full or communication failed, use default.
> -if len(jobs) < 1:
> -	print(default)
> -	sys.exit(0)
> -
> -# Report available slots (with a bump for our caller's reserveration).
> -print(len(jobs) + 1)
> +claim = len(jobs)
> +if claim < 1:
> +	# If the jobserver was (impossibly) full or communication failed
> +	# in some way do not use parallelism.
> +	claim = 0

Eh, "claim < 1" is the same as "claim == 0", right? So this doesn't seem
to do much. But what seems to be missing is that after you write back
the tokens in the error case above (os.write(writer, jobs)), jobs is not
set back to the empty string. That needs to be done either there or in
the outer exception handler (where you just have a "pass" currently).

> +# Launch command with a bump for our caller's reserveration,
> +# since we're just going to sit here blocked on our child.
> +claim += 1
> +
> +os.unsetenv('MAKEFLAGS')
> +os.environ['PARALLELISM'] = '%d' % (claim)
> +rc = subprocess.call(sys.argv[1:])
> +
> +# Return all the actually reserved slots.
> +if len(jobs):
> +	os.write(writer, jobs)
> +
> +sys.exit(rc)

What happens if the child dies from a signal? Will this correctly
forward that information?

Similarly (and the harder problem), what happens when our parent wants
to send its child a signal to say "stop what you're doing, return the
tokens, brush your teeth and go to bed". We should forward that signal
to the real job instead of just dying, losing track of both the tokens
we've claimed as well as orphaning the child.

Rasmus

Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer

[Kees Cook]

On Thu, Nov 21, 2019 at 08:41:01AM +0100, Rasmus Villemoes wrote:
> On 21/11/2019 01.03, Kees Cook wrote:
> > Setting non-blocking via a local copy of the jobserver file descriptor
> > is safer than just assuming the writer on the original fd is prepared
> > for it to be non-blocking.
> 
> This is a bit inaccurate. The fd referring to the write side of the pipe
> is always blocking - it has to be, due to the protocol requiring you to
> write back the tokens you've read, so you can't just drop a token on the
> floor. But it's also rather moot, since the pipe will never hold
> anywhere near 4096 bytes, let alone a (linux) pipe's default capacity of
> 64K.
> 
> But what we cannot do is change the mode of the open file description to
> non-blocking for the read side, in case the parent make (or some sibling
> process that has also inherited the same "struct file") expects it to be
> blocking.

Ah! This explains my confusion over what you were trying to tell me
before. I thought you meant the other end of the pipe, which seemed
crazy. You mean the other jobserver readers (i.e. "make" itself) who
have the same shared _reader_ fd. This is exactly what you said, but I
was too dense. :)

I'll fix this up!

> 
> > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Link: https://lore.kernel.org/lkml/44c01043-ab24-b4de-6544-e8efd153e27a@rasmusvillemoes.dk
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  scripts/jobserver-count | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/scripts/jobserver-count b/scripts/jobserver-count
> > index 6e15b38df3d0..a68a04ad304f 100755
> > --- a/scripts/jobserver-count
> > +++ b/scripts/jobserver-count
> > @@ -12,12 +12,6 @@ default="1"
> >  if len(sys.argv) > 1:
> >  	default=sys.argv[1]
> >  
> > -# Set non-blocking for a given file descriptor.
> > -def nonblock(fd):
> > -	flags = fcntl.fcntl(fd, fcntl.F_GETFL)
> > -	fcntl.fcntl(fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
> > -	return fd
> > -
> >  # Extract and prepare jobserver file descriptors from envirnoment.
> >  try:
> >  	# Fetch the make environment options.
> > @@ -31,8 +25,13 @@ try:
> >  	# Parse out R,W file descriptor numbers and set them nonblocking.
> >  	fds = opts[0].split("=", 1)[1]
> >  	reader, writer = [int(x) for x in fds.split(",", 1)]
> > -	reader = nonblock(reader)
> > -except (KeyError, IndexError, ValueError, IOError):
> > +	# Open a private copy of reader to avoid setting nonblocking
> > +	# on an unexpecting writer.
> 
> s/writer/reader/
> 
> > +	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
> > +	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
> > +	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)
> 
> I think you can just specify O_NONBLOCK in the open() call so you avoid
> those two fcntls.

Hah. Yes indeed.

-- 
Kees Cook

Re: [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made

[Kees Cook]

On Thu, Nov 21, 2019 at 09:09:37AM +0100, Rasmus Villemoes wrote:
> On 21/11/2019 01.03, Kees Cook wrote:
> > diff --git a/Documentation/sphinx/parallel-wrapper.sh b/Documentation/sphinx/parallel-wrapper.sh
> > new file mode 100644
> > index 000000000000..a416dbfd2025
> > --- /dev/null
> > +++ b/Documentation/sphinx/parallel-wrapper.sh
> > @@ -0,0 +1,25 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# Figure out if we should follow a specific parallelism from the make
> > +# environment (as exported by scripts/jobserver-exec), or fall back to
> > +# the "auto" parallelism when "-jN" is not specified at the top-level
> > +# "make" invocation.
> > +
> > +sphinx="$1"
> > +shift || true
> > +
> > +parallel="${PARALLELISM:-1}"
> > +if [ ${parallel} -eq 1 ] ; then
> > +	auto=$(perl -e 'open IN,"'"$sphinx"' --version 2>&1 |";
> > +			while (<IN>) {
> > +				if (m/([\d\.]+)/) {
> > +					print "auto" if ($1 >= "1.7")
> > +				}
> > +			}
> > +			close IN')
> > +	if [ -n "$auto" ] ; then
> > +		parallel="$auto"
> > +	fi
> > +fi
> > +exec "$sphinx" "-j$parallel" "$@"
> 
> I don't understand this logic. If the parent failed to claim any tokens
> (likely because the top make and its descendants are already running 16
> gcc processes), just let sphinx run #cpus jobs in parallel? That doesn't
> make sense - it gets us back to the "we've now effectively injected K
> tokens to the jobserver that weren't there originally".

I was going to say "but jobserver-exec can't be running unless there are
available slots", but I see the case is "if there are 16 slots and
jobserver-exec gets _1_, it should not fall back to 'auto'".

> From the comment above, what you want is to use "auto" if the top
> invocation was simply "make docs". Well, I kind of disagree with falling
> back to auto in that case; the user can say "make -j8 docs" and the
> wrapper is guaranteed to claim them all. But if you really want, the
> jobserver-count script needs to detect and export the "no parallelism
> requested at top level" in some way distinct from "PARALLELISM=1",
> because that's ambiguous.

Right -- failure needs to be be distinct from "only 1 available".

> > +	# Read out as many jobserver slots as possible.
> > +	while True:
> > +		try:
> > +			slot = os.read(reader, 1)
> > +			jobs += slot
> 
> I'd just try to slurp in 8 or 16 tokens at a time, there's no reason to
> limit to 1 in each loop.

Good point. I will change that.

> > +rc = subprocess.call(sys.argv[1:])
> > +
> > +# Return all the actually reserved slots.
> > +if len(jobs):
> > +	os.write(writer, jobs)
> > +
> > +sys.exit(rc)
> 
> What happens if the child dies from a signal? Will this correctly
> forward that information?

As far as I understand, yes, signal codes are passed through via the exit
code (i.e. see WIFSIGNALED, etc).

> Similarly (and the harder problem), what happens when our parent wants
> to send its child a signal to say "stop what you're doing, return the
> tokens, brush your teeth and go to bed". We should forward that signal
> to the real job instead of just dying, losing track of both the tokens
> we've claimed as well as orphaning the child.

Hm, hm. I guess I could pass INT and TERM to the child. That seems like
the most sensible best-effort here. It seems "make" isn't only looking
at the slots to determine process management.

-- 
Kees Cook

Re: [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made

[Kees Cook]

On Thu, Nov 21, 2019 at 11:39:03AM -0800, Kees Cook wrote:
> On Thu, Nov 21, 2019 at 09:09:37AM +0100, Rasmus Villemoes wrote:
> > Similarly (and the harder problem), what happens when our parent wants
> > to send its child a signal to say "stop what you're doing, return the
> > tokens, brush your teeth and go to bed". We should forward that signal
> > to the real job instead of just dying, losing track of both the tokens
> > we've claimed as well as orphaning the child.
> 
> Hm, hm. I guess I could pass INT and TERM to the child. That seems like
> the most sensible best-effort here. It seems "make" isn't only looking
> at the slots to determine process management.

Actually, this doesn't seem to work at all. Interruption already behaves
correctly. I'm going to ignore this for now...

-- 
Kees Cook