From: Kees Cook <keescook@chromium.org>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Jonathan Corbet <corbet@lwn.net>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] docs: Use make invocation's -j argument for parallelism
Date: Fri, 4 Oct 2019 09:08:32 -0700 [thread overview]
Message-ID: <201910040904.43B61E4@keescook> (raw)
In-Reply-To: <eb25959a-9ec4-3530-2031-d9d716b40b20@rasmusvillemoes.dk>
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
next prev parent reply other threads:[~2019-10-04 16:08 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-10-06 19:33 ` Rasmus Villemoes
2019-10-15 20:55 ` Rasmus Villemoes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201910040904.43B61E4@keescook \
--to=keescook@chromium.org \
--cc=corbet@lwn.net \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=mchehab+samsung@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).