Re: [PATCH v2 2/6] Makefile: Run the subdirectory recursion in parallel

Adhemerval Zanella <[email protected]> writes:

> The top-level makefile was marked .NOTPARALLEL and ran the
> per-subdirectory sub-makes strictly one at a time in the topological
> order computed by scripts/gen-sorted.awk.  Only the compilations inside
> a single subdirectory could run in parallel, so on wide machines every
> subdirectory's compile tail and link steps left most cores idle, once
> per subdirectory per pass.
>
> Drop .NOTPARALLEL and encode the ordering the serial recursion relied
> on as explicit dependencies between the per-subdirectory targets:
>
>   * The subdirectories that generate shared files in $(common-objpfx)
>     consumed by the rest of the build without explicit dependencies run
>     serially, in their sorted order, before the rest fan out: csu
>     provides the tree-wide gen-as-const headers, and on Hurd the mach
>     and hurd directories generate the MiG RPC headers (every other
>     subdirectory otherwise runs a nested make in hurd/ to create them,
>     racing under parallel recursion; see sysdeps/mach/hurd/Makefile).
>     The first of them also materializes the other shared generated files
>     (abi-versions.h, sysd-syscalls, before-compile headers).
>
>   * The edges requested by the Depend files (now emitted by
>     gen-sorted.awk as subdir-deps-*) are preserved.  Edges pointing to
>     elf are dropped, as the sorted list already overrides them by
>     forcing elf last.
>
>   * The tests and xtests classes only run the per-directory test
>     programs, which are mutually independent once the others pass has
>     built the tree.  They therefore carry only the others pass barrier
>     below and none of the csu-first or Depend edges (+ordered_parallel_-
>     subdir_targets excludes them); otherwise "make subdir/tests" would
>     also run the tests of every subdirectory reachable through those
>     edges, rather than just the requested one.
>
>   * elf stays last: its rtld link consumes $(common-objpfx)libc_pic.a,
>     which aggregates every other subdirectory's objects, and its
>     rtld-Rules recursion compiles into the other subdirectories' object
>     directories.
>
>   * Pass barriers replace the implicit pass ordering: others after lib
>     (a subdirectory others sub-make would otherwise race to link
>     libc.so itself), tests/xtests after others, and the testroot
>     install behind others.
>
>   * The threading (nptl, or htl on Hurd) and realtime (rt) tests are
>     timing-sensitive and were previously shielded from system load by
>     the global .NOTPARALLEL.  With the recursion now parallel, a full
>     test run ('make check'/'tests', run-built-tests=yes) orders them
>     after the rest of the test run and one group at a time -- the
>     threading subdirectory, then rt -- and each serializes its own run
>     via a .NOTPARALLEL in its Makefile.  A targeted 'make subdir/tests'
>     is not ordered.
>
>     The serialization (the per-subdirectory .NOTPARALLEL and the ordering
>     above) constrains only the test run, not the build of the test
>     programs; but building and running a subdirectory's tests are fused
>     in its sub-make, so under run-built-tests=yes the serialized
>     subdirectories would also build their test programs serially.  To
>     avoid that, the top-level 'make check' (in Makerules) now runs two
>     passes: it builds every test program with run-built-tests=no, where
>     the recursion is fully parallel and none of the serialization
>     applies, and then runs the tests with run-built-tests=yes.  'make
>     tests' and a subdirectory's own 'check' stay single pass.
>
>   * The subdirectory-built files that the top-level libc.so and
>     linkobj/libc_pic.a rules list as prerequisites (elf/ld.so,
>     interp.os, sofini.os, sunrpc/librpc_compat_pic.a, and on Hurd
>     mach/libmachuser_pic.a and hurd/libhurduser_pic.a, from which the
>     lib*user-link.so inputs of libc.so are built) get order-only edges
>     on the corresponding sub-make with an explicit empty recipe.  A
>     prerequisite-only rule would trigger an implicit rule search and
>     this level would compile them itself in the wrong context.
>
>   * The install, clean, abi, and stubs target classes keep the
>     previous total order via a serial dependency chain.
>
>   * The elf DSO sorting test recipes, run when make remakes the
>     included generated makefiles at parse time, create the elf object
>     directory before writing into it; the serial recursion no longer
>     guarantees another rule created it first.
>
>   * catgets builds locale-specific message catalogs (and tst-catgets
>     reads one) by running gencat under de_DE.ISO-8859-1, hr_HR.ISO-8859-2
>     and ja_JP.SJIS, but never declared those locales as prerequisites: it
>     relied on localedata running before it in the serial order.  Under
>     the parallel recursion gencat could run before localedata generated
>     the locale, fall back to C, and fail.  catgets/Makefile now pulls the
>     locales in via gen-locales.mk, like the other subdirectories that use
>     locales in their tests.
>
> Results on a x86_64 machine [1] with default configuration [3]: a
> from-scratch build improves from 78.728s to 61s, and check with
> run-built-tests=no from 374s to 190s.
>
> On a 80-core aarch64 machine [2] with default configuration [3]: a
> from-scratch build improves from 105.251s to 56.703s, and check with
> run-built-tests=no from 886.183s to 298.726s.
>
> Build results are unchanged: all 8919 built objects, archives, and
> shared objects are bit-identical to the serial build across 7 clean
> parallel builds, the installed tree layout is identical, and the
> tests.sum failure sets are identical.  i686-gnu was verified with
> repeated from-scratch builds.
>
> [1] Ryzen 5900x, 12c/24t, gcc 16.1.1, binutils 2.26, and GNU make 4.3
> [2] N1, 80c, gcc 15.1.1, binutils 2.25, GNU make 4.3
> [3] --enable-stack-protector=all --enable-bind-now=yes --enable-profile=yes
>     --enable-fortify-source=2 --enable-hardcoded-path-in-tests
> ---
>  Makefile         | 138 +++++++++++++++++++++++++++++++++++++++++++++--
>  Makerules        |  31 +++++++++--
>  catgets/Makefile |  14 +++--
>  elf/Makefile     |   5 ++
>  htl/Makefile     |   7 +++
>  rt/Makefile      |   8 +++
>  6 files changed, 193 insertions(+), 10 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 6b0e0555189..45dc36942a8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -54,9 +54,6 @@ configure: configure.ac aclocal.m4; $(autoconf-it)
>  endif # $(AUTOCONF) = no
>  
>  
> -# We don't want to run anything here in parallel.
> -.NOTPARALLEL:
> -
>  # These are the targets that are made by making them in each subdirectory.
>  +subdir_targets	:= subdir_lib objects objs others subdir_mostlyclean	\
>  		   subdir_clean subdir_distclean subdir_realclean	\
> @@ -129,6 +126,13 @@ lib-noranlib: subdir_lib
>  ifeq (yes,$(build-shared))
>  # Build the shared object from the PIC object library.
>  lib: $(common-objpfx)libc.so $(common-objpfx)linkobj/libc.so
> +ifdef libc.so-version
> +# Every program linked in the others pass lists the versioned name
> +# (through link-libc-between-gnulib) as a prerequisite, and the rule
> +# creating the symbolic link is visible in every sub-make.  Build it
> +# here once so the concurrent sub-makes do not race to create it.
> +lib: $(common-objpfx)libc.so$(libc.so-version)
> +endif
>  endif # $(build-shared)
>  
>  # Used to build testrun.sh.
> @@ -490,6 +494,134 @@ subdir=$(@D)$(if $($(@D)-srcdir),\
>  endef
>  
>  .PHONY: $(+subdir_targets) $(all-subdirs-targets)
> +
> +# Encode the topological ordering computed by scripts/gen-sorted.awk as
> +# explicit dependencies between the per-subdirectory targets, so that
> +# independent subdirectories build concurrently:

I suggest: ". In summary:"

It makes clear that there's independent comments below about the same thing.

> +#
> +#  * Every subdirectory depends on the first sorted one (csu, or mach on
> +#    Hurd): its sub-make also materializes the shared generated files in
> +#    $(common-objpfx) (abi-versions.h, sysd-syscalls, before-compile
> +#    headers, ...) that concurrent sub-makes would otherwise race to
> +#    create.
> +#
> +#  * The edges requested by the Depend files (emitted by gen-sorted.awk
> +#    as subdir-deps-*) are preserved.
> +#
> +#  * elf stays last, as in the sorted list.  Its rtld build recurses into
> +#    the other subdirectories' object directories via elf/rtld-Rules.
> +#
> +#  * Only target classes without cross-directory file conflicts use this
> +#    sparse ordering; everything else (install, clean, abi, stubs) keeps
> +#    the previous total order via a serial chain.
> +
> ++parallel_subdir_targets := \
> +  subdir_lib \
> +  objects \
> +  objs \
> +  others \
> +  tests \
> +  xtests \
> +  subdir_objs \
> +  # +parallel_subdir_targets
> ++serial_subdir_targets := $(filter-out $(+parallel_subdir_targets),\
> +				       $(+subdir_targets))
> +
> +# The tests and xtests classes run, rather than build, the per-directory
> +# test programs; once the 'others' pass barrier below has built the tree
> +# they are mutually independent and carry no cross-directory ordering.
> +# Keeping them out of the generated-file and Depend edges below is what
> +# lets 'make subdir/tests' run only that subdirectory's tests.
> ++barrier_only_subdir_targets := tests xtests
> ++ordered_parallel_subdir_targets := \
> +  $(filter-out $(+barrier_only_subdir_targets),$(+parallel_subdir_targets))
> +
> +# The subdirectories that generate shared files in $(common-objpfx)
> +# consumed by the rest of the build without explicit dependencies: csu
> +# provides the gen-as-const headers, and on Hurd the mach and hurd
> +# directories generate the MiG RPC headers (every other subdirectory
> +# otherwise runs a nested make in hurd/ to create them, racing under
> +# parallel recursion; see sysdeps/mach/hurd/Makefile).  Run them serially,
> +# in their sorted order (mach, hurd, csu).
> ++subdir-pregen := $(filter mach hurd csu,$(subdirs))
> ++subdir-rest := $(filter-out $(+subdir-pregen),$(subdirs))
> +
> +$(foreach t,$(+ordered_parallel_subdir_targets),$(eval \
> +  $(addsuffix /$(t),$(+subdir-rest)): $(addsuffix /$(t),$(+subdir-pregen))))
> ++subdir-pregen-prev :=
> +$(foreach d,$(+subdir-pregen),$(foreach t,$(+ordered_parallel_subdir_targets),$(eval \
> +  $(d)/$(t): $(addsuffix /$(t),$(+subdir-pregen-prev))))\
> +  $(eval +subdir-pregen-prev := $(d)))
> +# Edges pointing to elf are dropped; the sorted list always forces elf
> +# last, overriding any Depend request, and the elf-last edges below would
> +# otherwise create a cycle.
> +$(foreach t,$(+ordered_parallel_subdir_targets),$(foreach d,$(+subdir-rest),$(eval \
> +  $(d)/$(t): $(addsuffix /$(t),\
> +	      $(filter-out elf,$(filter $(subdirs),$(subdir-deps-$(d))))))))
> +ifneq (,$(filter elf,$(subdirs)))
> +$(foreach t,$(+ordered_parallel_subdir_targets),$(eval \
> +  elf/$(t): $(addsuffix /$(t),$(filter-out elf,$(subdirs)))))
> +endif
> +
> +# Pass barriers: a subdirectory 'others' build links programs against
> +# the libraries, so the 'lib' pass (including the top-level libc.so
> +# link) must have completed.
> +# 'tests' and 'xtests' additionally require the 'others' pass.  The
> +# testroot used by the container tests performs a full installation in
> +# its recipe, which must not run concurrently with the build passes.
> +$(addsuffix /others,$(subdirs)): lib
> +$(addsuffix /tests,$(subdirs)) $(addsuffix /xtests,$(subdirs)): others
> +$(objpfx)testroot.pristine/install.stamp: | others
> +
> +# Timing-sensitive test runs: the threading tests (nptl/htl) and the realtime
> +# tests (rt) are perturbed by the machine load, so run them after the rest of
> +# the test run has finished and one group at a time.  Those subdirectories
> +# also serialize their own tests (.NOTPARALLEL in their Makefiles).
> +#
> +# This only orders a full-suite run ('make check'/'tests'); a targeted
> +# 'make subdir/tests' is left alone.  And it only orders the test run
> +# (run-built-tests=yes); the "build the tests" pass (run-built-tests=no)
> +# is left fully parallel, so every test program still builds concurrently.
> +ifeq ($(run-built-tests),yes)
> +ifneq (,$(filter tests xtests check xcheck,$(MAKECMDGOALS)))
> ++late-test-subdirs := $(filter nptl htl,$(subdirs)) $(filter rt,$(subdirs))
> ++test-run-prev := \
> +  $(addsuffix /tests,$(filter-out $(+late-test-subdirs),$(subdirs)))
> +$(foreach d,$(+late-test-subdirs),\
> +  $(eval $(d)/tests: $(+test-run-prev))\
> +  $(eval +test-run-prev += $(d)/tests))
> +endif
> +endif
> +
> +ifeq (yes,$(build-shared))
> +# The top-level libc.so and linkobj/libc_pic.a rules list these
> +# subdirectory-built files as prerequisites, but no rule at this level
> +# builds them.  The explicit empty recipe (';') is required, a
> +# prerequisite-only rule would send make on an implicitrule search and
> +# have this level compile them itself with the wrong context.
> +$(elf-objpfx)ld.so $(elf-objpfx)sofini.os $(elf-objpfx)interp.os: \
> +  | elf/subdir_lib ;
> +ifneq (,$(filter sunrpc,$(subdirs)))
> +# Makerules explicit adds librpc_compat_pic.a as a dependency of
> +# libc_pic.a.
> +$(common-objpfx)sunrpc/librpc_compat_pic.a: | sunrpc/subdir_lib ;
> +endif
> +# Hurd sysdedp Makeilfe links libc.so against the lib*user-link.so
> +# objects, built by the %-link.so: %_pic.a pattern rule from archives
> +# that only the mach and hurd sub-makes create.
> +ifneq (,$(filter mach,$(subdirs)))
> +$(common-objpfx)mach/libmachuser_pic.a: | mach/subdir_lib ;
> +endif
> +ifneq (,$(filter hurd,$(subdirs)))
> +$(common-objpfx)hurd/libhurduser_pic.a: | hurd/subdir_lib ;
> +endif
> +endif
> +
> +# The remaining target classes keep the old total order.
> ++subdir-chain-prev :=
> +$(foreach d,$(subdirs),$(foreach t,$(+serial_subdir_targets),$(eval \
> +  $(d)/$(t): $(addsuffix /$(t),$(+subdir-chain-prev))))\
> +  $(eval +subdir-chain-prev := $(d)))
>  
>  # Targets to clean things up to various degrees.
>  
> diff --git a/Makerules b/Makerules
> index 180cbee251a..318a916eef0 100644
> --- a/Makerules
> +++ b/Makerules
> @@ -1184,12 +1184,35 @@ ALL_BUILD_CFLAGS = $(BUILD_CFLAGS) $(BUILD_CPPFLAGS) -D_GNU_SOURCE \
>  		   -DIS_IN_build -include $(common-objpfx)config.h
>  
>  # Support the GNU standard name for this target.
> -.PHONY: check
> +# Special target xcheck runs tests which cannot be run unconditionally;
> +# maintainers should use this target.
> +.PHONY: check xcheck
> +
> +# Building and running a subdirectory's tests are fused in its sub-make,
> +# and run-built-tests is fixed for a make instance, so the only way to
> +# build every test program with the recursion fully parallel while the
> +# run still honors the per-subdirectory .NOTPARALLEL (nptl/htl/rt) and the
> +# run-time ordering is to use two passes.  At the top level, 'make check'
> +# therefore builds the test programs (run-built-tests=no, recursion fully
> +# parallel) and then runs them (run-built-tests=yes).  'make tests' and a
> +# subdirectory's own 'check' stay single pass.

Nit: single-pass.

> +check-twopass :=
> +ifndef subdir
> +ifeq (yes,$(run-built-tests))
> +check-twopass := yes
> +endif
> +endif
> +ifeq (yes,$(check-twopass))
> +check:
> +	$(MAKE) run-built-tests=no  tests
> +	$(MAKE) run-built-tests=yes tests
> +xcheck:
> +	$(MAKE) run-built-tests=no  xtests
> +	$(MAKE) run-built-tests=yes xtests
> +else
>  check: tests
> -# Special target to run tests which cannot be run unconditionally.
> -# Maintainers should use this target.
> -.PHONY: xcheck
>  xcheck: xtests
> +endif
>  
>  # Also handle test inputs in sysdeps.
>  vpath %.input $(sysdirs)
> diff --git a/catgets/Makefile b/catgets/Makefile
> index fbc416731ed..630273ac608 100644
> --- a/catgets/Makefile
> +++ b/catgets/Makefile
> @@ -60,6 +60,14 @@ vpath %.c ../locale/programs
>  
>  include ../Rules
>  
> +# The catalog-generation tests below run gencat under specific locales,
> +# and tst-catgets reads the resulting catalog, so the build must wait for
> +# those locales to be generated.  Without this dependency a parallel build
> +# races catgets against localedata and gencat can run before the locale
> +# exists (it then falls back to C and the test fails).
> +LOCALES := de_DE.ISO-8859-1 hr_HR.ISO-8859-2 ja_JP.SJIS
> +include ../gen-locales.mk
> +
>  $(objpfx)gencat: $(gencat-modules:%=$(objpfx)%.o)
>  
>  catgets-CPPFLAGS := -DNLSPATH='"$(localedir)/%L/%N:$(localedir)/%L/LC_MESSAGES/%N:$(localedir)/%l/%N:$(localedir)/%l/LC_MESSAGES/%N:"'
> @@ -95,7 +103,7 @@ tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de \
>  ifeq ($(run-built-tests),yes)
>  # This test just checks whether the program produces any error or not.
>  # The result is not tested.
> -$(objpfx)test1.cat: test1.msg $(objpfx)gencat
> +$(objpfx)test1.cat: test1.msg $(objpfx)gencat $(gen-locales)
>  	$(built-program-cmd-before-env) \
>  	$(run-program-env) LC_ALL=hr_HR.ISO-8859-2 \
>  	$(built-program-cmd-after-env) -H $(objpfx)test1.h $@ $<; \
> @@ -103,7 +111,7 @@ $(objpfx)test1.cat: test1.msg $(objpfx)gencat
>  $(objpfx)test2.cat: test2.msg $(objpfx)gencat
>  	$(built-program-cmd) -H $(objpfx)test2.h $@ $<; \
>  	$(evaluate-test)
> -$(objpfx)de/libc.cat: $(objpfx)de.msg $(objpfx)gencat
> +$(objpfx)de/libc.cat: $(objpfx)de.msg $(objpfx)gencat $(gen-locales)
>  	$(make-target-directory)
>  	$(built-program-cmd-before-env) \
>  	$(run-program-env) LC_ALL=de_DE.ISO-8859-1 \
> @@ -116,7 +124,7 @@ $(objpfx)de.msg: xopen-msg.awk $(..)po/de.po
>  	LC_ALL=C $(AWK) -f $^ $< > $@
>  
>  $(objpfx)test-gencat.out: test-gencat.sh $(objpfx)test-gencat \
> -			  $(objpfx)sample.SJIS.cat
> +			  $(objpfx)sample.SJIS.cat $(gen-locales)
>  	$(SHELL) $< $(common-objpfx) '$(test-program-cmd-before-env)' \
>  		 '$(run-program-env)' '$(test-program-cmd-after-env)'; \
>  	$(evaluate-test)
> diff --git a/elf/Makefile b/elf/Makefile
> index 5ede78c5902..5598bafba76 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1436,6 +1436,7 @@ ifndef avoid-generated
>  # Makefile fragment to be included.
>  define include_dsosort_tests
>  $(objpfx)$(1).generated-makefile: $(1)
> +	$$(make-target-directory)
>  	$(PYTHON) $(..)scripts/dso-ordering-test.py \
>  	--description-file $$< --objpfx $(objpfx) --output-makefile $$@T
>  	mv $$@T $$@
> @@ -1444,6 +1445,7 @@ endef
>  # Likewise, where the .def file itself is generated.
>  define include_dsosort_tests_objpfx
>  $(objpfx)$(1).generated-makefile: $(objpfx)$(1)
> +	$$(make-target-directory)
>  	$(PYTHON) $(..)scripts/dso-ordering-test.py \
>  	--description-file $$< --objpfx $(objpfx) --output-makefile $$@T
>  	mv $$@T $$@
> @@ -1462,12 +1464,15 @@ $(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
>  $(eval $(call include_dsosort_tests,dso-sort-tests-2.def))
>  
>  $(objpfx)dso-sort-tests-all2.def: dso-sort-tests-all.py
> +	$(make-target-directory)
>  	$(PYTHON) $< 2 > $@
>  
>  $(objpfx)dso-sort-tests-all3.def: dso-sort-tests-all.py
> +	$(make-target-directory)
>  	$(PYTHON) $< 3 > $@
>  
>  $(objpfx)dso-sort-tests-all4.def: dso-sort-tests-all.py
> +	$(make-target-directory)
>  	$(PYTHON) $< 4 > $@
>  
>  $(eval $(call include_dsosort_tests_objpfx,dso-sort-tests-all2.def))
> diff --git a/htl/Makefile b/htl/Makefile
> index 5bad0333a5a..0e5a8b8f689 100644
> --- a/htl/Makefile
> +++ b/htl/Makefile
> @@ -257,3 +257,10 @@ $(addprefix $(objpfx),$(tests-static) $(xtests-static)): $(srcdir)/libpthread_sy
>  else
>  $(addprefix $(objpfx),$(tests) $(test-srcs)): $(srcdir)/libpthread_syms.a $(objpfx)libpthread.a
>  endif
> +
> +# The tests here better do not run in parallel.

Say something like: "Like rt, these tests prefer to be run serially."

> +ifeq ($(run-built-tests),yes)
> +ifneq ($(filter %tests,$(MAKECMDGOALS)),)
> +.NOTPARALLEL:
> +endif
> +endif
> diff --git a/rt/Makefile b/rt/Makefile
> index 39a3e5620b3..24e43a87599 100644
> --- a/rt/Makefile
> +++ b/rt/Makefile
> @@ -109,3 +109,11 @@ LDFLAGS-rt.so = -Wl,--enable-new-dtags,-z,nodelete
>  $(objpfx)librt.so: $(shared-thread-library)
>  
>  tst-mqueue7-ARGS = -- $(host-test-program-cmd)
> +
> +# The timer and message-queue tests here are timing-sensitive and better
> +# do not run in parallel.
> +ifeq ($(run-built-tests),yes)
> +ifneq ($(filter %tests,$(MAKECMDGOALS)),)
> +.NOTPARALLEL:
> +endif
> +endif

Reviewed-by: Sam James <[email protected]>

The changes look good. Also, when testing, thus far I only had one small
issue:
https://inbox.sourceware.org/libc-alpha/822344f7cd03b8f199d0e612868221d13d14300d.1783035357.git.sam@gentoo.org/

Thanks.

-----BEGIN PGP SIGNATURE-----

iQEBBAEWCgCpFiEEJaa7iN2bdkxrVUHCc4QJ9SDfkZAFAmpHlZcbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwyXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25z
Lm9wZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXQyNUE2QkI4OEREOUI3NjRDNkI1NTQx
QzI3Mzg0MDlGNTIwREY5MTkwDxxzYW1AZ2VudG9vLm9yZwAKCRBzhAn1IN+RkITB
AP9vBXkKFlgJ40qe7mQ+3fyiVTo9BbcX9s+Y8s1ggEudpAEA8V7/L8DTMMJmb93e
NJ5F/1QtLdQlJk1bhHeQ32/wxAw=
=8SSs
-----END PGP SIGNATURE-----