* Re: [PATCH] MAKE: Fix parallel builds; .o files; .gitignore; new makedocs
2023-06-28 14:07 [PATCH] MAKE: Fix parallel builds; .o files; .gitignore; new makedocs KuhnChris
@ 2023-06-30 17:16 ` Stefano Brivio
2023-07-02 10:04 ` David Gibson
0 siblings, 1 reply; 3+ messages in thread
From: Stefano Brivio @ 2023-06-30 17:16 UTC (permalink / raw)
To: KuhnChris; +Cc: passt-dev, KuhnChris
Thanks for the patch, and sorry for the delay!
First off, as I was mentioning on IRC: it would help to have this as
two/three patches -- not just because it's easier to review, but also
because the Makefile changes have, I guess, a relatively high risk of
introducing some regressions, so keeping it simple for 'git bisect'
would also be a plus here.
If you want to split this in your working tree, you can do something
like:
git reset HEAD~
meaning: reset (the git state, not the file contents) to the commit
before head (~ is a shortcut for "just before"). Then 'git add' your
pieces back -- and if you have two changes to the Makefile you want to
keep separated, go with 'git add -i', which lets you add just parts.
If that's not your HEAD anymore, 'git rebase -i' to something before
this commit, and do the same git reset/git add trick, then 'git rebase
--continue'.
Another general comment (about the title): this doesn't actually fix
parallel builds... it makes builds more parallel.
Now, to the changes:
On Wed, 28 Jun 2023 16:07:28 +0200
KuhnChris <kuhnchris+git@kuhnchris.eu> wrote:
> From: KuhnChris <kuhnchris@kuhnchris.eu>
>
> ---
> .gitignore | 1 +
> Makefile | 51 +++++++++++++++++++++++++--------------------------
> makedocs.pl | 19 +++++++++++++++++++
> 3 files changed, 45 insertions(+), 26 deletions(-)
> create mode 100644 makedocs.pl
>
> diff --git a/.gitignore b/.gitignore
> index d3d0e2c..caeafa5 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,4 +1,5 @@
> *~
> +*.o
> /passt
> /passt.avx2
> /pasta
> diff --git a/Makefile b/Makefile
> index a5256f5..4c99771 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -100,29 +100,41 @@ else
> BIN := passt pasta qrap
> endif
>
> -all: $(BIN) $(MANPAGES) docs
> +.NOTPARALLEL: seccomp.h
> +all: seccomp.h $(BIN) $(MANPAGES) docs
> +
> +PASST_OBJS = $(PASST_SRCS:.c=.o)
> +PASST_AVX2_OBJS = $(PASST_SRCS:.c=.avx2.o)
> +OBJS = $(SRCS:.c=.o)
> +QRAP_OBJS = $(QRAP_SRCS:.c=.o)
> +AVXFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
This all looks correct to me, but for some reason, on a machine
with 12 (free-ish) CPU threads, 'make -j1' takes the same time as
'make -j10', and seems to lead to the same amount of parallel gcc
processes... no idea why, yet. I would have expected actual parallelism
resulting from this.
By the way, build time (after a make clean) is the same as without this
change, with -j1 or -j10.
I know you're introducing this to avoid a full rebuild if only some
source files changed, which is an advantage in any case, but I'm
missing something...
>
> static: FLAGS += -static -DGLIBC_NO_STATIC_NSS
> -static: clean all
> +static: clean seccomp.h all
>
> seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
> @ EXTRA_SYSCALLS="$(EXTRA_SYSCALLS)" ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
>
> -passt: $(PASST_SRCS) $(HEADERS)
> - $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
> +.c.o:
> + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -c $< -o $@
> +
> +$(PASST_AVX2_OBJS): seccomp.h
> + $(CC) $(AVXFLAGS) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -c $(@:.avx2.o=.c) -o $@
> +
> +passt: $(PASST_OBJS) seccomp.h
> + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_OBJS) -o passt $(LDFLAGS)
>
> -passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
> -passt.avx2: $(PASST_SRCS) $(HEADERS)
> - $(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(CPPFLAGS) \
> - $(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
> +passt.avx2: $(PASST_AVX2_OBJS) $(HEADERS) seccomp.h
> + $(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(CPPFLAGS) $(AVXFLAGS) \
> + $(PASST_AVX2_OBJS) -o passt.avx2 $(LDFLAGS)
>
> -passt.avx2: passt
> +passt.avx2: passt seccomp.h
>
> pasta.avx2 pasta.1 pasta: pasta%: passt%
> ln -sf $< $@
>
> -qrap: $(QRAP_SRCS) passt.h
> - $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(QRAP_SRCS) -o qrap $(LDFLAGS)
> +qrap: $(QRAP_OBJS) passt.h seccomp.h
> + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(QRAP_OBJS) -o qrap $(LDFLAGS)
>
> valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \
> getpid gettid kill clock_gettime mmap \
> @@ -170,21 +182,8 @@ pkgs: static
> # other way around: the web version should be obtained by adding HTML and
> # JavaScript portions to a plain Markdown, instead. However, cgit needs to use
> # a file in the git tree. Find a better way around this.
> -docs: README.md
> - @( \
> - skip=0; \
> - while read l; do \
> - case $$l in \
> - "## Demo") exit 0 ;; \
> - "<!"*) ;; \
> - "</"*) skip=1 ;; \
> - "<"*) skip=2 ;; \
> - esac; \
> - \
> - [ $$skip -eq 0 ] && echo "$$l"; \
> - [ $$skip -eq 1 ] && skip=0; \
> - done < README.md; \
> - ) > README.plain.md
> +docs:
The output is much more complete now (and the Makefile cleaner).
> + [ ! -e doc/ ]; mkdir docs; perl makedocs.pl > docs/README.plain.md
Shouldn't we (naturally) use Makefile dependencies for this? Also note
that you're moving the README.plain.md file to docs/, so you would also
need to change the install target:
cp -d README.plain.md $(DESTDIR)$(docdir)/README.md
...but, anyway, I think it would make more sense to keep the output in
the root directory (simply because README.md is already there, so one
can more easily spot the "plain" version).
>
> # Checkers currently disabled for clang-tidy:
> # - llvmlibc-restrict-system-libc-headers
> diff --git a/makedocs.pl b/makedocs.pl
> new file mode 100644
This should probably have 0755 permissions, it's an interpreted
executable after all.
> index 0000000..295e1d9
> --- /dev/null
> +++ b/makedocs.pl
This makes it sound like it does more than just stripping away the HTML
stuff. Perhaps make_plain_readme.pl, or plain_readme.pl,
strip_html.pl... something like that?
> @@ -0,0 +1,19 @@
Missing license header:
# SPDX-License-Identifier: GPL-2.0-or-later
and:
#!/usr/bin/perl
> +use strict;
> +
> +my $str = '';
> +my $regex = qr/(<[^\/].*?>.*?<\/.*?>\n?)|(<\/div>)|(<\/p>)/msp;
> +my $subst = '';
> +
> +
> +local $/=undef;
> +open FILE, '<', 'README.md' or die "Can't open file $!";
> +my $file_content = <FILE>;
> +close FILE;
> +#print "Source: $file_content\n";
> +my $result = $file_content =~ s/$regex//rg;
> +
> +my $regex2 = qr/\n[ \n]{3,}\n/msp;
> +$result = $result =~ s/$regex2/\n\n/rg;
> +
> +#print "The result of the substitution is: $result\n";
> +print "$result\n";
This looks good to me... I'm not sure if you intended to leave in those
comments, but I'm fine either way.
--
Stefano
^ permalink raw reply [flat|nested] 3+ messages in thread