linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] docs: Use make invocation's -j argument for parallelism
@ 2019-09-24 23:29 Kees Cook
  2019-10-01 12:39 ` Jonathan Corbet
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Kees Cook @ 2019-09-24 23:29 UTC (permalink / raw)
  To: Jonathan Corbet; +Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel

While sphinx 1.7 and later supports "-jauto" for parallelism, this
effectively ignores the "-j" flag used in the "make" invocation, which
may cause confusion for build systems. Instead, extract the available
parallelism from "make"'s job server (since it is not exposed in any
special variables) and use that for the "sphinx-build" run. Now things
work correctly for builds where -j is specified at the top-level:

	make -j16 htmldocs

If -j is not specified, continue to fallback to "-jauto" if available.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v3: python2, specific exceptions, correct SPDX, blocking writer
v2: retain "-jauto" default behavior with top-level -j is missing.              
---
 Documentation/Makefile  |  3 ++-
 scripts/jobserver-count | 58 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100755 scripts/jobserver-count

diff --git a/Documentation/Makefile b/Documentation/Makefile
index e145e4db508b..c6e564656a5b 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -33,7 +33,7 @@ ifeq ($(HAVE_SPHINX),0)
 
 else # HAVE_SPHINX
 
-export SPHINXOPTS = $(shell perl -e 'open IN,"sphinx-build --version 2>&1 |"; while (<IN>) { if (m/([\d\.]+)/) { print "-jauto" if ($$1 >= "1.7") } ;} close IN')
+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)
@@ -68,6 +68,7 @@ quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
 	PYTHONDONTWRITEBYTECODE=1 \
 	BUILDDIR=$(abspath $(BUILDDIR)) SPHINX_CONF=$(abspath $(srctree)/$(src)/$5/$(SPHINX_CONF)) \
 	$(SPHINXBUILD) \
+	-j $(shell python $(srctree)/scripts/jobserver-count $(SPHINX_PARALLEL)) \
 	-b $2 \
 	-c $(abspath $(srctree)/$(src)) \
 	-d $(abspath $(BUILDDIR)/.doctrees/$3) \
diff --git a/scripts/jobserver-count b/scripts/jobserver-count
new file mode 100755
index 000000000000..0b482d6884d2
--- /dev/null
+++ b/scripts/jobserver-count
@@ -0,0 +1,58 @@
+#!/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]
+
+# 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.
+	flags = os.environ['MAKEFLAGS']
+
+	# Look for "--jobserver=R,W"
+	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)]
+	reader = nonblock(reader)
+except (KeyError, IndexError, ValueError, IOError):
+	# 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)
+
+# Report available slots (with a bump for our caller's reserveration).
+print(len(jobs) + 1)
-- 
2.17.1


-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-09-24 23:29 [PATCH v3] docs: Use make invocation's -j argument for parallelism Kees Cook
@ 2019-10-01 12:39 ` Jonathan Corbet
  2019-10-04  8:04 ` Christian Borntraeger
  2019-10-04  9:15 ` Rasmus Villemoes
  2 siblings, 0 replies; 8+ messages in thread
From: Jonathan Corbet @ 2019-10-01 12:39 UTC (permalink / raw)
  To: Kees Cook; +Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel

On Tue, 24 Sep 2019 16:29:58 -0700
Kees Cook <keescook@chromium.org> wrote:

> While sphinx 1.7 and later supports "-jauto" for parallelism, this
> effectively ignores the "-j" flag used in the "make" invocation, which
> may cause confusion for build systems. Instead, extract the available
> parallelism from "make"'s job server (since it is not exposed in any
> special variables) and use that for the "sphinx-build" run. Now things
> work correctly for builds where -j is specified at the top-level:
> 
> 	make -j16 htmldocs
> 
> If -j is not specified, continue to fallback to "-jauto" if available.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

I finally messed with this a bit; it seems to do exactly what's written on
the box.

It seems to me that The Real Solution™ here is to send a patch to the
Sphinx folks adding a "-jgmake" (or some such) option.  It also seems to
me that none of us is likely to get around to that in the near future.  So
I just applied this, thanks for dealing with all my picky comments...

jon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-09-24 23:29 [PATCH v3] docs: Use make invocation's -j argument for parallelism Kees Cook
  2019-10-01 12:39 ` Jonathan Corbet
@ 2019-10-04  8:04 ` Christian Borntraeger
  2019-10-04 10:58   ` Christian Borntraeger
  2019-10-04  9:15 ` Rasmus Villemoes
  2 siblings, 1 reply; 8+ messages in thread
From: Christian Borntraeger @ 2019-10-04  8:04 UTC (permalink / raw)
  To: Kees Cook, Jonathan Corbet
  Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel, Heiko Carstens,
	Vasily Gorbik, Linux-Next Mailing List, Stephen Rothwell


On 25.09.19 01:29, Kees Cook wrote:
> While sphinx 1.7 and later supports "-jauto" for parallelism, this
> effectively ignores the "-j" flag used in the "make" invocation, which
> may cause confusion for build systems. Instead, extract the available
> parallelism from "make"'s job server (since it is not exposed in any
> special variables) and use that for the "sphinx-build" run. Now things
> work correctly for builds where -j is specified at the top-level:
> 
> 	make -j16 htmldocs
> 
> If -j is not specified, continue to fallback to "-jauto" if available.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v3: python2, specific exceptions, correct SPDX, blocking writer
> v2: retain "-jauto" default behavior with top-level -j is missing.              
[...]
> diff --git a/scripts/jobserver-count b/scripts/jobserver-count
> new file mode 100755
> index 000000000000..0b482d6884d2
> --- /dev/null
> +++ b/scripts/jobserver-count
> @@ -0,0 +1,58 @@
> +#!/usr/bin/env python


This breaks our daily linux-next build for an fedora 30 rpm on s390x:

+ /usr/lib/rpm/redhat/brp-mangle-shebangs
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/profile2linkerlist.pl from /usr/bin/env perl to #!/usr/bin/perl
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/headerdep.pl from /usr/bin/env perl to #!/usr/bin/perl
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/buildtar from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/builddeb from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/mkspec from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/mkdebian from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/checksyscalls.sh from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/gen_ksymdeps.sh from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/makelst from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/checkversion.pl from /usr/bin/env perl to #!/usr/bin/perl
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/gcc-plugin.sh from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/gfp-translate from /bin/bash to #!/usr/bin/bash
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/tags.sh from /bin/bash to #!/usr/bin/bash
*** ERROR: ambiguous python shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/jobserver-count: #!/usr/bin/env python. Change it to python3 (or python2) explicitly.
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/adjust_autoksyms.sh from /bin/sh to #!/usr/bin/sh
mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/kernel-doc from /usr/bin/env perl to #!/usr/bin/perl
[...]



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-09-24 23:29 [PATCH v3] docs: Use make invocation's -j argument for parallelism Kees Cook
  2019-10-01 12:39 ` Jonathan Corbet
  2019-10-04  8:04 ` Christian Borntraeger
@ 2019-10-04  9:15 ` Rasmus Villemoes
  2019-10-04 16:08   ` Kees Cook
  2 siblings, 1 reply; 8+ messages in thread
From: Rasmus Villemoes @ 2019-10-04  9:15 UTC (permalink / raw)
  To: Kees Cook, Jonathan Corbet; +Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel

On 25/09/2019 01.29, Kees Cook wrote:
>  
>  # User-friendly check for pdflatex and latexmk
>  HAVE_PDFLATEX := $(shell if which $(PDFLATEX) >/dev/null 2>&1; then echo 1; else echo 0; fi)
> @@ -68,6 +68,7 @@ quiet_cmd_sphinx = SPHINX  $@ --> file://$(abspath $(BUILDDIR)/$3/$4)
>  	PYTHONDONTWRITEBYTECODE=1 \
>  	BUILDDIR=$(abspath $(BUILDDIR)) SPHINX_CONF=$(abspath $(srctree)/$(src)/$5/$(SPHINX_CONF)) \
>  	$(SPHINXBUILD) \
> +	-j $(shell python $(srctree)/scripts/jobserver-count $(SPHINX_PARALLEL)) \
>  	-b $2 \
>  	-c $(abspath $(srctree)/$(src)) \
>  	-d $(abspath $(BUILDDIR)/.doctrees/$3) \
> diff --git a/scripts/jobserver-count b/scripts/jobserver-count
> new file mode 100755
> index 000000000000..0b482d6884d2
> --- /dev/null
> +++ b/scripts/jobserver-count
> @@ -0,0 +1,58 @@
> +#!/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]
> +
> +# 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.
> +	flags = os.environ['MAKEFLAGS']
> +
> +	# Look for "--jobserver=R,W"
> +	opts = [x for x in flags.split(" ") if x.startswith("--jobserver")]

OK, this handles the fact that Make changed from --jobserver-fds to
--jobserver-auth at some point. Probably the comment could be more accurate.

> +	# 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):
> +	# 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

<strikethrough>Only very new Make (e.g. not make 4.1 shipped with Ubuntu
18.04) sets the rfd as O_NONBLOCK (and only when it detected
HAVE_PSELECT at configure time, but that can probably be assumed). So
won't the above just hang forever if run under such a make?</strikethrough>

Ah, reading more carefully you set O_NONBLOCK explicitly. Well, older
Makes are going to be very unhappy about that (remember that it's a
property of the file description and not file descriptor). They don't
expect EAGAIN when fetching a token, so fail hard. Probably not when
htmldocs is the only target, because in that case the toplevel Make just
reads back the exact number of tokens it put in as a sanity check, but
if it builds other targets/spawns other submakes, I think this breaks.

Yeah, it's a mess, and the Make documentation should be much more
explicit about how one is supposed to interact with the job server and
the file descriptors. Some of the pain would vanish if it just used a
named pipe and had each client open its own fds to that so they could
each choose O_NONBLOCK or not.

> +# Return all the reserved slots.
> +os.write(writer, jobs)

Well, that probably works ok for the isolated case of a toplevel "make
-j12 htmldocs", but if you're building other targets ("make -j12
htmldocs vmlinux") this will effectively inject however many tokens the
above loop grabbed (which might not be all if the top-level make has
started things related to the vmlinux target), so for the duration of
the docs build, there will be more processes running than asked for.

> +# If the jobserver was (impossibly) full or communication failed, use default.
> +if len(jobs) < 1:
> +	print(default)
> +
> +# Report available slots (with a bump for our caller's reserveration).
> +print(len(jobs) + 1)

There's a missing exit() or else: here; if len(jobs) < 1 you print both
default (probably "1") and 0+1 aka "1".

Rasmus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-10-04  8:04 ` Christian Borntraeger
@ 2019-10-04 10:58   ` Christian Borntraeger
  0 siblings, 0 replies; 8+ messages in thread
From: Christian Borntraeger @ 2019-10-04 10:58 UTC (permalink / raw)
  To: Kees Cook, Jonathan Corbet
  Cc: Mauro Carvalho Chehab, linux-doc, linux-kernel, Heiko Carstens,
	Vasily Gorbik, Linux-Next Mailing List, Stephen Rothwell



On 04.10.19 10:04, Christian Borntraeger wrote:
> 
> On 25.09.19 01:29, Kees Cook wrote:
>> While sphinx 1.7 and later supports "-jauto" for parallelism, this
>> effectively ignores the "-j" flag used in the "make" invocation, which
>> may cause confusion for build systems. Instead, extract the available
>> parallelism from "make"'s job server (since it is not exposed in any
>> special variables) and use that for the "sphinx-build" run. Now things
>> work correctly for builds where -j is specified at the top-level:
>>
>> 	make -j16 htmldocs
>>
>> If -j is not specified, continue to fallback to "-jauto" if available.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> v3: python2, specific exceptions, correct SPDX, blocking writer
>> v2: retain "-jauto" default behavior with top-level -j is missing.              
> [...]
>> diff --git a/scripts/jobserver-count b/scripts/jobserver-count
>> new file mode 100755
>> index 000000000000..0b482d6884d2
>> --- /dev/null
>> +++ b/scripts/jobserver-count
>> @@ -0,0 +1,58 @@
>> +#!/usr/bin/env python
> 
> 
> This breaks our daily linux-next build for an fedora 30 rpm on s390x:
> 
> + /usr/lib/rpm/redhat/brp-mangle-shebangs
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/profile2linkerlist.pl from /usr/bin/env perl to #!/usr/bin/perl
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/headerdep.pl from /usr/bin/env perl to #!/usr/bin/perl
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/buildtar from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/builddeb from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/mkspec from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/package/mkdebian from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/checksyscalls.sh from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/gen_ksymdeps.sh from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/makelst from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/checkversion.pl from /usr/bin/env perl to #!/usr/bin/perl
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/gcc-plugin.sh from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/gfp-translate from /bin/bash to #!/usr/bin/bash
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/tags.sh from /bin/bash to #!/usr/bin/bash
> *** ERROR: ambiguous python shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/jobserver-count: #!/usr/bin/env python. Change it to python3 (or python2) explicitly.
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/adjust_autoksyms.sh from /bin/sh to #!/usr/bin/sh
> mangling shebang in /usr/src/kernels/5.4.0-20191004.rc1.git155.311ef88adfa3.301.fc30.s390x+next/scripts/kernel-doc from /usr/bin/env perl to #!/usr/bin/perl
> [...]


Ok, adding something like 

+pathfix.py -pni "%{__python3} %{py3_shbang_opts}" scripts/jobserver-count

to the spec file fixed the problem. 

Question is, if we want to make the python version more specific or not.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-10-04  9:15 ` Rasmus Villemoes
@ 2019-10-04 16:08   ` Kees Cook
  2019-10-06 19:33     ` Rasmus Villemoes
  0 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2019-10-04 16:08 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Jonathan Corbet, Mauro Carvalho Chehab, linux-doc, linux-kernel

On Fri, Oct 04, 2019 at 11:15:46AM +0200, Rasmus Villemoes wrote:
> On 25/09/2019 01.29, Kees Cook wrote:
> > +# Extract and prepare jobserver file descriptors from envirnoment.
> > +try:
> > +	# Fetch the make environment options.
> > +	flags = os.environ['MAKEFLAGS']
> > +
> > +	# Look for "--jobserver=R,W"
> > +	opts = [x for x in flags.split(" ") if x.startswith("--jobserver")]
> 
> OK, this handles the fact that Make changed from --jobserver-fds to
> --jobserver-auth at some point. Probably the comment could be more accurate.

I can update that, sure.

> > +# 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
> 
> Ah, reading more carefully you set O_NONBLOCK explicitly. Well, older
> Makes are going to be very unhappy about that (remember that it's a
> property of the file description and not file descriptor). They don't
> expect EAGAIN when fetching a token, so fail hard. Probably not when
> htmldocs is the only target, because in that case the toplevel Make just
> reads back the exact number of tokens it put in as a sanity check, but
> if it builds other targets/spawns other submakes, I think this breaks.

Do you mean the processes sharing the file will suddenly gain
O_NONBLOCK? I didn't think that was true, but I can test. If it is true,
we could easily just restore the state before exit.

> > +# Return all the reserved slots.
> > +os.write(writer, jobs)
> 
> Well, that probably works ok for the isolated case of a toplevel "make
> -j12 htmldocs", but if you're building other targets ("make -j12
> htmldocs vmlinux") this will effectively inject however many tokens the
> above loop grabbed (which might not be all if the top-level make has
> started things related to the vmlinux target), so for the duration of
> the docs build, there will be more processes running than asked for.

That is true, yes, though I still think it's an improvement over the
existing case of sphinx-build getting run with -jauto which lands us in
the same (or worse) position.

The best solution would be to teach sphinx-build about the Make
jobserver, though I expect that would be weird. Another idea would be to
hold the reservation until sphinx-build finishes and THEN return the
slots? That would likely need to change from a utility to a sphinx-build
wrapper...

> > +# If the jobserver was (impossibly) full or communication failed, use default.
> > +if len(jobs) < 1:
> > +	print(default)
> > +
> > +# Report available slots (with a bump for our caller's reserveration).
> > +print(len(jobs) + 1)
> 
> There's a missing exit() or else: here; if len(jobs) < 1 you print both
> default (probably "1") and 0+1 aka "1".

Whoops! Yes, that needs fixing too. I'll send a patch...

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-10-04 16:08   ` Kees Cook
@ 2019-10-06 19:33     ` Rasmus Villemoes
  2019-10-15 20:55       ` Rasmus Villemoes
  0 siblings, 1 reply; 8+ messages in thread
From: Rasmus Villemoes @ 2019-10-06 19:33 UTC (permalink / raw)
  To: Kees Cook; +Cc: Jonathan Corbet, Mauro Carvalho Chehab, linux-doc, linux-kernel

On 04/10/2019 18.08, Kees Cook wrote:
> On Fri, Oct 04, 2019 at 11:15:46AM +0200, Rasmus Villemoes wrote:
>> On 25/09/2019 01.29, Kees Cook wrote:
>>> +# Extract and prepare jobserver file descriptors from envirnoment.
>>
>> Ah, reading more carefully you set O_NONBLOCK explicitly. Well, older
>> Makes are going to be very unhappy about that (remember that it's a
>> property of the file description and not file descriptor). They don't
>> expect EAGAIN when fetching a token, so fail hard. Probably not when
>> htmldocs is the only target, because in that case the toplevel Make just
>> reads back the exact number of tokens it put in as a sanity check, but
>> if it builds other targets/spawns other submakes, I think this breaks.
> 
> Do you mean the processes sharing the file will suddenly gain
> O_NONBLOCK? I didn't think that was true, 

It is. Quoting man fcntl

   File status flags
       Each  open  file  description has certain associated status
flags, initialized by open(2) and possibly modified by
       fcntl().  Duplicated file descriptors (made with dup(2),
fcntl(F_DUPFD), fork(2), etc.) refer  to  the  same  open
       file description, and thus share the same file status flags.

...  On Linux, this  command
              can  change  only  the O_APPEND, O_ASYNC, O_DIRECT,
O_NOATIME, and O_NONBLOCK flags.

> we could easily just restore the state before exit.

That doesn't really help - and I'm a bit surprised you'd even suggest
that. I don't know if open(/proc/self/fd/...) would give you a new
struct file.

>>> +# Return all the reserved slots.
>>> +os.write(writer, jobs)
>>
>> Well, that probably works ok for the isolated case of a toplevel "make
>> -j12 htmldocs", but if you're building other targets ("make -j12
>> htmldocs vmlinux") this will effectively inject however many tokens the
>> above loop grabbed (which might not be all if the top-level make has
>> started things related to the vmlinux target), so for the duration of
>> the docs build, there will be more processes running than asked for.
> 
> That is true, yes, though I still think it's an improvement over the
> existing case of sphinx-build getting run with -jauto which lands us in
> the same (or worse) position.

Yes, I agree that that's not ideal either. And probably it's not a big
problem in practice (I don't think a lot of people build the docs, let
alone do it while also building the kernel), but it might be rather
surprising and somewhat hard to "debug" to suddenly have a load twice
what one expected.

> The best solution would be to teach sphinx-build about the Make
> jobserver, though I expect that would be weird. Another idea would be to
> hold the reservation until sphinx-build finishes and THEN return the
> slots? That would likely need to change from a utility to a sphinx-build
> wrapper...

Yes, a more general solution would be some kind of generic wrapper that
would hog however many tokens it could get hold of and run a given
command with a commandline slightly modified to hand over those tokens -
then wait for that process to exit and give back the tokens. That would
work for any command that knows about parallelism but doesn't support
the make jobserver model. (I'd probably implement that by creating a
pipe, fork(), then exec into the real command, while the child simply
blocks in a read on the pipe waiting for EOF and then writes back the
tokens, to simplify the "we have to report exit/killed-by-signal status
to the parent).

Rasmus

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
  2019-10-06 19:33     ` Rasmus Villemoes
@ 2019-10-15 20:55       ` Rasmus Villemoes
  0 siblings, 0 replies; 8+ messages in thread
From: Rasmus Villemoes @ 2019-10-15 20:55 UTC (permalink / raw)
  To: Kees Cook; +Cc: Jonathan Corbet, Mauro Carvalho Chehab, linux-doc, linux-kernel

On 06/10/2019 21.33, Rasmus Villemoes wrote:
> On 04/10/2019 18.08, Kees Cook wrote:

>> The best solution would be to teach sphinx-build about the Make
>> jobserver, though I expect that would be weird. Another idea would be to
>> hold the reservation until sphinx-build finishes and THEN return the
>> slots? That would likely need to change from a utility to a sphinx-build
>> wrapper...
> 
> Yes, a more general solution would be some kind of generic wrapper that
> would hog however many tokens it could get hold of and run a given
> command with a commandline slightly modified to hand over those tokens -
> then wait for that process to exit and give back the tokens. That would
> work for any command that knows about parallelism but doesn't support
> the make jobserver model.

On the off-chance that anybody cares I tried implementing that, because
I've wanted something like that to make "ninja" play nice when invoked
from Make for a long time. Rough sketch at
https://github.com/Villemoes/jobhog .

Rasmus

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-15 20:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-24 23:29 [PATCH v3] docs: Use make invocation's -j argument for parallelism Kees Cook
2019-10-01 12:39 ` Jonathan Corbet
2019-10-04  8:04 ` Christian Borntraeger
2019-10-04 10:58   ` Christian Borntraeger
2019-10-04  9:15 ` Rasmus Villemoes
2019-10-04 16:08   ` Kees Cook
2019-10-06 19:33     ` Rasmus Villemoes
2019-10-15 20:55       ` Rasmus Villemoes

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).