* [PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made
@ 2019-11-21 0:03 Kees Cook
2019-11-21 0:03 ` [PATCH 1/3] docs, parallelism: Fix failure path and add comment Kees Cook
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Kees Cook @ 2019-11-21 0:03 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Kees Cook, Rasmus Villemoes, linux-doc, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] docs, parallelism: Fix failure path and add comment 2019-11-21 0:03 [PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook @ 2019-11-21 0:03 ` Kees Cook 2019-11-21 0:03 ` [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer Kees Cook 2019-11-21 0:03 ` [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook 2 siblings, 0 replies; 9+ messages in thread From: Kees Cook @ 2019-11-21 0:03 UTC (permalink / raw) To: Jonathan Corbet; +Cc: Kees Cook, Rasmus Villemoes, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer 2019-11-21 0:03 [PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook 2019-11-21 0:03 ` [PATCH 1/3] docs, parallelism: Fix failure path and add comment Kees Cook @ 2019-11-21 0:03 ` Kees Cook 2019-11-21 7:41 ` Rasmus Villemoes 2019-11-21 0:03 ` [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook 2 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2019-11-21 0:03 UTC (permalink / raw) To: Jonathan Corbet; +Cc: Kees Cook, Rasmus Villemoes, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer 2019-11-21 0:03 ` [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer Kees Cook @ 2019-11-21 7:41 ` Rasmus Villemoes 2019-11-21 19:09 ` Kees Cook 0 siblings, 1 reply; 9+ messages in thread From: Rasmus Villemoes @ 2019-11-21 7:41 UTC (permalink / raw) To: Kees Cook, Jonathan Corbet; +Cc: linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer 2019-11-21 7:41 ` Rasmus Villemoes @ 2019-11-21 19:09 ` Kees Cook 0 siblings, 0 replies; 9+ messages in thread From: Kees Cook @ 2019-11-21 19:09 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Jonathan Corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made 2019-11-21 0:03 [PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook 2019-11-21 0:03 ` [PATCH 1/3] docs, parallelism: Fix failure path and add comment Kees Cook 2019-11-21 0:03 ` [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer Kees Cook @ 2019-11-21 0:03 ` Kees Cook 2019-11-21 8:09 ` Rasmus Villemoes 2 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2019-11-21 0:03 UTC (permalink / raw) To: Jonathan Corbet; +Cc: Kees Cook, Rasmus Villemoes, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made 2019-11-21 0:03 ` [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook @ 2019-11-21 8:09 ` Rasmus Villemoes 2019-11-21 19:39 ` Kees Cook 0 siblings, 1 reply; 9+ messages in thread From: Rasmus Villemoes @ 2019-11-21 8:09 UTC (permalink / raw) To: Kees Cook, Jonathan Corbet; +Cc: linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made 2019-11-21 8:09 ` Rasmus Villemoes @ 2019-11-21 19:39 ` Kees Cook 2019-11-21 19:52 ` Kees Cook 0 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2019-11-21 19:39 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Jonathan Corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made 2019-11-21 19:39 ` Kees Cook @ 2019-11-21 19:52 ` Kees Cook 0 siblings, 0 replies; 9+ messages in thread From: Kees Cook @ 2019-11-21 19:52 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Jonathan Corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-21 19:52 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-21 0:03 [PATCH 0/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook 2019-11-21 0:03 ` [PATCH 1/3] docs, parallelism: Fix failure path and add comment Kees Cook 2019-11-21 0:03 ` [PATCH 2/3] docs, parallelism: Do not leak blocking mode to writer Kees Cook 2019-11-21 7:41 ` Rasmus Villemoes 2019-11-21 19:09 ` Kees Cook 2019-11-21 0:03 ` [PATCH 3/3] docs, parallelism: Rearrange how jobserver reservations are made Kees Cook 2019-11-21 8:09 ` Rasmus Villemoes 2019-11-21 19:39 ` Kees Cook 2019-11-21 19:52 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).