From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id B97715A026F for ; Fri, 30 Jun 2023 19:16:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688145390; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mk5cHGq1qiGojGrN6SB1RThukVHQCWALxhHsZ+Rot10=; b=YptYcoOiYiO0+sIPA1Z1JCKS/uHx2Qgpt5P6LAhk4R8j2NAj47pLAUjFGzZw1u8UHI/Asx lrHfaD23/9uWYl+5iY5fPFdu2mnw+NbYjYD5sYupAgfzS3v/rPUQEmrFW+sUGKqrYMcc2h fWvZxX52QPfdxBKxQZr8wtziPqwxnro= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-626-WiKDibN3Mb2i6k532q7i4g-1; Fri, 30 Jun 2023 13:16:24 -0400 X-MC-Unique: WiKDibN3Mb2i6k532q7i4g-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id EF6FE185A78B; Fri, 30 Jun 2023 17:16:23 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.6]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 260BBC00049; Fri, 30 Jun 2023 17:16:21 +0000 (UTC) Date: Fri, 30 Jun 2023 19:16:19 +0200 From: Stefano Brivio To: KuhnChris Subject: Re: [PATCH] MAKE: Fix parallel builds; .o files; .gitignore; new makedocs Message-ID: <20230630191544.34acb996@elisabeth> In-Reply-To: <20230628140727.15750-1-kuhnchris+git@kuhnchris.eu> References: <20230628140727.15750-1-kuhnchris+git@kuhnchris.eu> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 6TXB5ZOQATQNP6BIUZVAQXJ64FPJL7H7 X-Message-ID-Hash: 6TXB5ZOQATQNP6BIUZVAQXJ64FPJL7H7 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, KuhnChris X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 wrote: > From: KuhnChris > > --- > .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=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 = ; > +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