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

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

[Kees Cook]

v2:
    - correct comments and commit logs (rasmus)
    - handle non-parallel mode more cleanly (rasmus)
    - reserve slots 8 at a time (rasmus)
v1: https://lore.kernel.org/lkml/20191121000304.48829-1-keescook@chromium.org

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 other readers
  docs, parallelism: Rearrange how jobserver reservations are made

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

-- 
2.17.1

[PATCH v2 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 v2 2/3] docs, parallelism: Do not leak blocking mode to other readers

[Kees Cook]

Setting non-blocking via a local copy of the jobserver file descriptor
is safer than just assuming other reader processes with the same fd open
are 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 | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/scripts/jobserver-count b/scripts/jobserver-count
index 6e15b38df3d0..7807bfa7dafa 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,12 @@ 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 process with the same reader fd.
+	reader = os.open("/proc/self/fd/%d" % (reader),
+			 os.O_RDONLY | 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 v2 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 | 33 ++++++++++++
 scripts/jobserver-count                  | 59 ---------------------
 scripts/jobserver-exec                   | 66 ++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 62 deletions(-)
 create mode 100644 Documentation/sphinx/parallel-wrapper.sh
 delete mode 100755 scripts/jobserver-count
 create mode 100755 scripts/jobserver-exec

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..7daf5133bdd3
--- /dev/null
+++ b/Documentation/sphinx/parallel-wrapper.sh
@@ -0,0 +1,33 @@
+#!/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"
+if [ -z "$parallel" ] ; then
+	# If no parallelism is specified at the top-level make, then
+	# fall back to the expected "-jauto" mode that the "htmldocs"
+	# target has had.
+	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
+# Only if some parallelism has been determined do we add the -jN option.
+if [ -n "$parallel" ] ; then
+	parallel="-j$parallel"
+fi
+
+exec "$sphinx" "$parallel" "$@"
diff --git a/scripts/jobserver-count b/scripts/jobserver-count
deleted file mode 100755
index 7807bfa7dafa..000000000000
--- a/scripts/jobserver-count
+++ /dev/null
@@ -1,59 +0,0 @@
-#!/usr/bin/env python
-# SPDX-License-Identifier: GPL-2.0+
-#
-# This determines how many parallel tasks "make" is expecting, as it is
-# not exposed via an special variables.
-# 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]
-
-# Extract and prepare jobserver file descriptors from envirnoment.
-try:
-	# Fetch the make environment options.
-	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.
-	fds = opts[0].split("=", 1)[1]
-	reader, writer = [int(x) for x in fds.split(",", 1)]
-	# Open a private copy of reader to avoid setting nonblocking
-	# on an unexpecting process with the same reader fd.
-	reader = os.open("/proc/self/fd/%d" % (reader),
-			 os.O_RDONLY | 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)
-	sys.exit(0)
-
-# 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)
diff --git a/scripts/jobserver-exec b/scripts/jobserver-exec
new file mode 100755
index 000000000000..0fdb31a790a8
--- /dev/null
+++ b/scripts/jobserver-exec
@@ -0,0 +1,66 @@
+#!/usr/bin/env python
+# SPDX-License-Identifier: GPL-2.0+
+#
+# This determines how many parallel tasks "make" is expecting, as it is
+# 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, errno
+import subprocess
+
+# Extract and prepare jobserver file descriptors from envirnoment.
+claim = 0
+jobs = b""
+try:
+	# Fetch the make environment options.
+	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.
+	fds = opts[0].split("=", 1)[1]
+	reader, writer = [int(x) for x in fds.split(",", 1)]
+	# Open a private copy of reader to avoid setting nonblocking
+	# on an unexpecting process with the same reader fd.
+	reader = os.open("/proc/self/fd/%d" % (reader),
+			 os.O_RDONLY | os.O_NONBLOCK)
+
+	# Read out as many jobserver slots as possible.
+	while True:
+		try:
+			slot = os.read(reader, 8)
+			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
+	# Add a bump for our caller's reserveration, since we're just going
+	# to sit here blocked on our child.
+	claim = len(jobs) + 1
+except (KeyError, IndexError, ValueError, OSError, IOError) as e:
+	# Any missing environment strings or bad fds should result in just
+	# not being parallel.
+	pass
+
+# We can only claim parallelism if there was a jobserver (i.e. a top-level
+# "-jN" argument) and there were no other failures. Otherwise leave out the
+# environment variable and let the child figure out what is best.
+if claim > 0:
+	os.environ['PARALLELISM'] = '%d' % (claim)
+
+rc = subprocess.call(sys.argv[1:])
+
+# Return all the reserved slots.
+if len(jobs):
+	os.write(writer, jobs)
+
+sys.exit(rc)
-- 
2.17.1

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

[Jonathan Corbet]

On Thu, 21 Nov 2019 12:59:26 -0800
Kees Cook <keescook@chromium.org> wrote:

> v2:
>     - correct comments and commit logs (rasmus)
>     - handle non-parallel mode more cleanly (rasmus)
>     - reserve slots 8 at a time (rasmus)
> v1: https://lore.kernel.org/lkml/20191121000304.48829-1-keescook@chromium.org
> 
> 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)! :)

OK, I have applied this set for 5.5.

I do worry that this all looks a little complex and fragile.  I wonder if
there's a way that we could set up some sort of dependency chain that
would just tell make not to run the docs builds in parallel with anything
else?  That is more-or-less the effect of what we're doing anyway.

Meanwhile, though, this seems to work, so let's go with it :)

Thanks,

jon