[PATCH 0/3] Kbuild: Some fixdep tweaks

[PATCH 0/3] Kbuild: Some fixdep tweaks

[Rasmus Villemoes]

These patches eliminate two (albeit tiny and shortlived) processes
from the cmd_and_fixdep rule, i.e. from every TU being
compiled. Whether the diffstat below is worth it I'll leave to Kbuild
maintainers to decide.

Rasmus Villemoes (3):
  Kbuild: refactor fixdep to use getopt()
  Kbuild: teach fixdep to optionally remove the depfile
  Kbuild: let fixdep do the renaming to .cmd

 scripts/Kbuild.include | 11 ++++------
 scripts/basic/fixdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 15 deletions(-)

-- 
2.16.4

[PATCH 1/3] Kbuild: refactor fixdep to use getopt()

[Rasmus Villemoes]

As preparation for teaching an old dog a few new tricks.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 scripts/basic/fixdep.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 850966f3d602..666041841200 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -381,16 +381,23 @@ int main(int argc, char *argv[])
 	const char *depfile, *target, *cmdline;
 	int insert_extra_deps = 0;
 	void *buf;
+	int opt;
 
-	if (argc == 5 && !strcmp(argv[1], "-e")) {
-		insert_extra_deps = 1;
-		argv++;
-	} else if (argc != 4)
+	while ((opt = getopt(argc, argv, "e")) != -1) {
+		switch (opt) {
+		case 'e': insert_extra_deps = 1; break;
+		default: usage();
+		}
+	}
+	argc -= optind;
+	argv += optind;
+
+	if (argc != 3)
 		usage();
 
-	depfile = argv[1];
-	target = argv[2];
-	cmdline = argv[3];
+	depfile = argv[0];
+	target = argv[1];
+	cmdline = argv[2];
 
 	printf("cmd_%s := %s\n\n", target, cmdline);
 
-- 
2.16.4

[PATCH 2/3] Kbuild: teach fixdep to optionally remove the depfile

[Rasmus Villemoes]

Instead of having to spawn an 'rm' instance to remove the depfile after
processing, let the fixdep program itself do that. For debugging, it's
nice to not do it unconditionally.

Note that the fixdep calls from Makefiles are done under 'set -e', so
this also preserves the behaviour of keeping the depfile if fixdep exits
prematurely.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 scripts/Kbuild.include |  7 +++----
 scripts/basic/fixdep.c | 10 ++++++++--
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index d52db4279aa5..a944510acd9d 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -274,8 +274,8 @@ ifndef CONFIG_TRIM_UNUSED_KSYMS
 
 cmd_and_fixdep =                                                             \
 	$(echo-cmd) $(cmd_$(1));                                             \
-	scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
-	rm -f $(depfile);                                                    \
+	scripts/basic/fixdep -r $(depfile) $@ '$(make-cmd)'                  \
+		> $(dot-target).tmp;                                         \
 	mv -f $(dot-target).tmp $(dot-target).cmd;
 
 else
@@ -298,9 +298,8 @@ ksym_dep_filter =                                                            \
 cmd_and_fixdep =                                                             \
 	$(echo-cmd) $(cmd_$(1));                                             \
 	$(ksym_dep_filter) |                                                 \
-		scripts/basic/fixdep -e $(depfile) $@ '$(make-cmd)'          \
+		scripts/basic/fixdep -e -r $(depfile) $@ '$(make-cmd)'       \
 			> $(dot-target).tmp;	                             \
-	rm -f $(depfile);                                                    \
 	mv -f $(dot-target).tmp $(dot-target).cmd;
 
 endif
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 666041841200..7b59c185858a 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -105,8 +105,9 @@
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: fixdep [-e] <depfile> <target> <cmdline>\n");
+	fprintf(stderr, "Usage: fixdep [-e] [-r] <depfile> <target> <cmdline>\n");
 	fprintf(stderr, " -e  insert extra dependencies given on stdin\n");
+	fprintf(stderr, " -r  remove <depfile> after processing\n");
 	exit(1);
 }
 
@@ -380,12 +381,14 @@ int main(int argc, char *argv[])
 {
 	const char *depfile, *target, *cmdline;
 	int insert_extra_deps = 0;
+	int remove_depfile = 0;
 	void *buf;
 	int opt;
 
-	while ((opt = getopt(argc, argv, "e")) != -1) {
+	while ((opt = getopt(argc, argv, "er")) != -1) {
 		switch (opt) {
 		case 'e': insert_extra_deps = 1; break;
+		case 'r': remove_depfile = 1; break;
 		default: usage();
 		}
 	}
@@ -405,5 +408,8 @@ int main(int argc, char *argv[])
 	parse_dep_file(buf, target, insert_extra_deps);
 	free(buf);
 
+	if (remove_depfile)
+		unlink(depfile);
+
 	return 0;
 }
-- 
2.16.4

[PATCH 3/3] Kbuild: let fixdep do the renaming to .cmd

[Rasmus Villemoes]

Avoid spawning one more process per TU by having fixdep open the tmpfile
and rename to its final name.

The only change in behaviour is that if fixdep fails, the tmpfile we
leave behind is $(dot-target).cmd.tmp rather than $(dot-target).tmp .

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 scripts/Kbuild.include | 10 ++++------
 scripts/basic/fixdep.c | 31 +++++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index a944510acd9d..ded089436a3f 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -274,9 +274,8 @@ ifndef CONFIG_TRIM_UNUSED_KSYMS
 
 cmd_and_fixdep =                                                             \
 	$(echo-cmd) $(cmd_$(1));                                             \
-	scripts/basic/fixdep -r $(depfile) $@ '$(make-cmd)'                  \
-		> $(dot-target).tmp;                                         \
-	mv -f $(dot-target).tmp $(dot-target).cmd;
+	scripts/basic/fixdep -r -o $(dot-target).cmd                         \
+		$(depfile) $@ '$(make-cmd)';
 
 else
 
@@ -298,9 +297,8 @@ ksym_dep_filter =                                                            \
 cmd_and_fixdep =                                                             \
 	$(echo-cmd) $(cmd_$(1));                                             \
 	$(ksym_dep_filter) |                                                 \
-		scripts/basic/fixdep -e -r $(depfile) $@ '$(make-cmd)'       \
-			> $(dot-target).tmp;	                             \
-	mv -f $(dot-target).tmp $(dot-target).cmd;
+		scripts/basic/fixdep -e -r -o $(dot-target).cmd              \
+			$(depfile) $@ '$(make-cmd)';
 
 endif
 
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 7b59c185858a..78985052a3c7 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -94,6 +94,7 @@
  *  but I don't think the added complexity is worth it)
  */
 
+#define _GNU_SOURCE
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
@@ -105,9 +106,10 @@
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: fixdep [-e] [-r] <depfile> <target> <cmdline>\n");
+	fprintf(stderr, "Usage: fixdep [-e] [-r] [-o <output>] <depfile> <target> <cmdline>\n");
 	fprintf(stderr, " -e  insert extra dependencies given on stdin\n");
 	fprintf(stderr, " -r  remove <depfile> after processing\n");
+	fprintf(stderr, " -o  write to <output> instead of stdout\n");
 	exit(1);
 }
 
@@ -380,15 +382,17 @@ static void parse_dep_file(char *m, const char *target, int insert_extra_deps)
 int main(int argc, char *argv[])
 {
 	const char *depfile, *target, *cmdline;
+	char *outfile = NULL, *tmpfile;
 	int insert_extra_deps = 0;
 	int remove_depfile = 0;
 	void *buf;
 	int opt;
 
-	while ((opt = getopt(argc, argv, "er")) != -1) {
+	while ((opt = getopt(argc, argv, "ero:")) != -1) {
 		switch (opt) {
 		case 'e': insert_extra_deps = 1; break;
 		case 'r': remove_depfile = 1; break;
+		case 'o': outfile = optarg; break;
 		default: usage();
 		}
 	}
@@ -402,12 +406,35 @@ int main(int argc, char *argv[])
 	target = argv[1];
 	cmdline = argv[2];
 
+	if (outfile) {
+		if (asprintf(&tmpfile, "%s.tmp", outfile) < 0) {
+			perror("fixdep:asprintf");
+			exit(1);
+		}
+		if (freopen(tmpfile, "w", stdout) == NULL) {
+			perror("fixdep:freopen");
+			exit(1);
+		}
+	}
+
 	printf("cmd_%s := %s\n\n", target, cmdline);
 
 	buf = read_file(depfile);
 	parse_dep_file(buf, target, insert_extra_deps);
 	free(buf);
 
+	if (fclose(stdout)) {
+		perror("fixdep:fclose");
+		exit(1);
+	}
+	if (outfile) {
+		if (rename(tmpfile, outfile) < 0) {
+			perror("fixdep:rename");
+			exit(1);
+		}
+		free(tmpfile);
+	}
+
 	if (remove_depfile)
 		unlink(depfile);
 
-- 
2.16.4

Re: [PATCH 0/3] Kbuild: Some fixdep tweaks

[Rasmus Villemoes]

On 15 August 2018 at 16:27, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> These patches eliminate two (albeit tiny and shortlived) processes
> from the cmd_and_fixdep rule, i.e. from every TU being
> compiled. Whether the diffstat below is worth it I'll leave to Kbuild
> maintainers to decide.

Ping.

Re: [PATCH 0/3] Kbuild: Some fixdep tweaks

[Masahiro Yamada]

Hi Rasmus,


2018年9月27日(木) 3:58 Rasmus Villemoes <linux@rasmusvillemoes.dk>:
>
> On 15 August 2018 at 16:27, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > These patches eliminate two (albeit tiny and shortlived) processes
> > from the cmd_and_fixdep rule, i.e. from every TU being
> > compiled. Whether the diffstat below is worth it I'll leave to Kbuild
> > maintainers to decide.
>
> Ping.


Sorry for delay.
As far as I tested, the performance improvement was not noticeable level.

This patch set actually sits on the fence.
I tend to choose not-apply when I cannot make up my mind.


If you have something more to convince me, please let me know.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 0/3] Kbuild: Some fixdep tweaks

[Rasmus Villemoes]

On 2018-09-28 04:21, Masahiro Yamada wrote:
> Hi Rasmus,
> 
> This patch set actually sits on the fence.
> I tend to choose not-apply when I cannot make up my mind.
> 
> If you have something more to convince me, please let me know.

No, that's fine. I agree that the overall performance improvement for
whole kernel builds is hard to measure, but that can be said for many
small optimizations. Anyway, as I said, it's up to you, so let's just
drop it.

Thanks,
Rasmus