Message-ID: <t565x25o3gftbwzekhx6uanmxbkfdeqyydhkulwru5uszbw5wd@d7edoparssgv>
Date: Thu, 18 Sep 2025 19:33:23 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Jonathan Corbet <corbet@....net>
Cc: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, 
	Linux Doc Mailing List <linux-doc@...r.kernel.org>, Akira Yokosawa <akiyks@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 01/24] scripts/jobserver-exec: move the code to a class

On Thu, Sep 18, 2025 at 10:58:08AM -0600, Jonathan Corbet wrote:
> Mauro Carvalho Chehab <mchehab+huawei@...nel.org> writes:
> 
> > +class JobserverExec:
> > +    """
> > +    Claim all slots from make using POSIX Jobserver.
> > +
> > +    The main methods here are:
> > +    - open(): reserves all slots;
> > +    - close(): method returns all used slots back to make;
> > +    - run(): executes a command setting PARALLELISM=<available slots jobs + 1>
> > +    """
> > +
> > +    def __init__(self):
> > +        """Initialize internal vars"""
> > +        self.claim = 0
> > +        self.jobs = b""
> > +        self.reader = None
> > +        self.writer = None
> > +        self.is_open = False
> > +
> > +    def open(self):
> > +        """Reserve all available slots to be claimed later on"""
> > +
> > +        if self.is_open:
> > +            return
> > +
> > +        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.
> > +            # If the MAKEFLAGS variable contains multiple instances of the
> > +            # --jobserver-auth= option, the last one is relevant.
> > +            fds = opts[-1].split("=", 1)[1]
> > +
> > +            # Starting with GNU Make 4.4, named pipes are used for reader
> > +            # and writer.
> > +            # Example argument: --jobserver-auth=fifo:/tmp/GMfifo8134
> > +            _, _, path = fds.partition("fifo:")
> > +
> > +            if path:
> > +                self.reader = os.open(path, os.O_RDONLY | os.O_NONBLOCK)
> > +                self.writer = os.open(path, os.O_WRONLY)
> > +            else:
> > +                self.reader, self.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.
> > +                self.reader = os.open("/proc/self/fd/%d" % (self.reader),
> > +                                      os.O_RDONLY | os.O_NONBLOCK)
> > +
> > +            # Read out as many jobserver slots as possible
> > +            while True:
> > +                try:
> > +                    slot = os.read(self.reader, 8)
> > +                    self.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 self.jobs:
> > +                        os.write(self.writer, self.jobs)
> > +                    raise e
> > +
> > +            # Add a bump for our caller's reserveration, since we're just going
> > +            # to sit here blocked on our child.
> > +            self.claim = len(self.jobs) + 1
> > +
> > +        except (KeyError, IndexError, ValueError, OSError, IOError):
> > +            # Any missing environment strings or bad fds should result in just
> > +            # not being parallel.
> > +            self.claim = None
> 
> Sigh ... this kind of massive try/except block is one of the best ways
> I've found in Python to hide bugs - and Python has a few of those.  I
> realize this is existing code, this is not the place to fix it, but I
> would like to clean that up at some point.

Agreed: this class deserves some cleanup.

While working here, I considered doing more changes, but I refrained
myself. As you pointed, this is not the right series to do large
changes. Also, this is used not only for documentation build but also
inside scripts/Makefile.vmlinux_o. A major change there could affect
vmlinux generation.

Btw, besides Python cleanups, IMHO jobserver class could benefit
on having a maximum value for claim (which could even be 1). I was
also tempted to add a jobserver-aware subprocess.call here.

That's said, doing a risk analysis, if it fails to properly read
jobserver pipes, the except logic will set claim to None, which:

- for documentation: will use "-jauto";
- for vmlinux generation: will probably use a single job when
  building vmlinux. If target dependencies are correct, this
  should not cause build failures.

On my tests, I didn't get any such exception for doc build. I
wrote a small testbench to check if jobserver was handling claim
the right way. Before/after the changes, the behavior remained
the same, and caim was always initialized when running via make.

-- 
Thanks,
Mauro

Powered by blists - more mailing lists