Message-ID: <2a8c6ceb-9dda-f8b6-1a96-6e75dd3b4eea@rasmusvillemoes.dk>
Date:   Thu, 11 Mar 2021 02:45:25 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jessica Yu <jeyu@...nel.org>, Borislav Petkov <bp@...en8.de>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH v2 1/2] init/initramfs.c: allow asynchronous unpacking

On 11/03/2021 01.17, Rasmus Villemoes wrote:
> On 09/03/2021 23.16, Linus Torvalds wrote:
>> On Tue, Mar 9, 2021 at 1:17 PM Rasmus Villemoes
>> <linux@...musvillemoes.dk> wrote:
>>>
>>> So add an initramfs_async= kernel parameter, allowing the main init
>>> process to proceed to handling device_initcall()s without waiting for
>>> populate_rootfs() to finish.
>>
>> Oh, and a completely unrelated second comment about this: some of the
>> initramfs population code seems to be actively written to be slow.
>>
>> For example, I'm not sure why that write_buffer() function uses an
>> array of indirect function pointer actions. Even ignoring the whole
>> "speculation protections make that really slow" issue that came later,
>> it seems to always have been actively (a) slower and (b) more complex.
>>
>> The normal way to do that is with a simple switch() statement, which
>> makes the compiler able to do a much better job. Not just for the
>> state selector - maybe it picks that function pointer approach, but
>> probably these days just direct comparisons - but simply to do things
>> like inline all those "it's used in one place" cases entirely. In
>> fact, at that point, a lot of the state machine changes may end up
>> turning into pure goto's - compilers are sometimes good at that
>> (because state machines have often been very timing-critical).
> 
> FWIW, I tried doing
> 

Hm, gcc does elide the test of the return value, but jumps back to a
place where it always loads state from its memory location and does the
whole switch(). To get it to jump directly to the code implementing the
various do_* helpers it seems one needs to avoid that global variable
and instead return the next state explicitly. The below boots, but I
still can't see any measurable improvement on ppc.

Rasmus

Subject: [PATCH] init/initramfs.c: change state machine implementation

Instead of having write_buffer() rely on the global variable "state",
have each of the do_* helpers return the next state, or the new token
Stop. Also, instead of an array of function pointers, use a switch
statement.

This means all the do_* helpers end up inlined into write_buffer(),
and all the places which return a compile-time constant next state now
compile to a direct jump to that code.

We still need the global variable state for the initial choice within
write_buffer, and we also need to preserve the last non-Stop state
across calls.
---
 init/initramfs.c | 90 ++++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 1d0fdd05e5e9..ad7e04393acb 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -189,7 +189,8 @@ static __initdata enum state {
 	GotName,
 	CopyFile,
 	GotSymlink,
-	Reset
+	Reset,
+	Stop
 } state, next_state;

 static __initdata char *victim;
@@ -207,17 +208,17 @@ static __initdata char *collected;
 static long remains __initdata;
 static __initdata char *collect;

-static void __init read_into(char *buf, unsigned size, enum state next)
+static int __init read_into(char *buf, unsigned size, enum state next)
 {
 	if (byte_count >= size) {
 		collected = victim;
 		eat(size);
-		state = next;
+		return next;
 	} else {
 		collect = collected = buf;
 		remains = size;
 		next_state = next;
-		state = Collect;
+		return Collect;
 	}
 }

@@ -225,8 +226,7 @@ static __initdata char *header_buf, *symlink_buf,
*name_buf;

 static int __init do_start(void)
 {
-	read_into(header_buf, 110, GotHeader);
-	return 0;
+	return read_into(header_buf, 110, GotHeader);
 }

 static int __init do_collect(void)
@@ -238,50 +238,46 @@ static int __init do_collect(void)
 	eat(n);
 	collect += n;
 	if ((remains -= n) != 0)
-		return 1;
-	state = next_state;
-	return 0;
+		return Stop;
+	return next_state;
 }

 static int __init do_header(void)
 {
 	if (memcmp(collected, "070707", 6)==0) {
 		error("incorrect cpio method used: use -H newc option");
-		return 1;
+		return Stop;
 	}
 	if (memcmp(collected, "070701", 6)) {
 		error("no cpio magic");
-		return 1;
+		return Stop;
 	}
 	parse_header(collected);
 	next_header = this_header + N_ALIGN(name_len) + body_len;
 	next_header = (next_header + 3) & ~3;
-	state = SkipIt;
 	if (name_len <= 0 || name_len > PATH_MAX)
-		return 0;
+		return SkipIt;
 	if (S_ISLNK(mode)) {
 		if (body_len > PATH_MAX)
-			return 0;
+			return SkipIt;
 		collect = collected = symlink_buf;
 		remains = N_ALIGN(name_len) + body_len;
 		next_state = GotSymlink;
-		state = Collect;
-		return 0;
+		return Collect;
 	}
 	if (S_ISREG(mode) || !body_len)
-		read_into(name_buf, N_ALIGN(name_len), GotName);
-	return 0;
+		return read_into(name_buf, N_ALIGN(name_len), GotName);
+	return SkipIt;
 }

 static int __init do_skip(void)
 {
 	if (this_header + byte_count < next_header) {
 		eat(byte_count);
-		return 1;
+		return Stop;
 	} else {
 		eat(next_header - this_header);
-		state = next_state;
-		return 0;
+		return next_state;
 	}
 }

@@ -291,7 +287,7 @@ static int __init do_reset(void)
 		eat(1);
 	if (byte_count && (this_header & 3))
 		error("broken padding");
-	return 1;
+	return Stop;
 }

 static void __init clean_path(char *path, umode_t fmode)
@@ -324,11 +320,12 @@ static __initdata loff_t wfile_pos;

 static int __init do_name(void)
 {
-	state = SkipIt;
+	int s = SkipIt;
+
 	next_state = Reset;
 	if (strcmp(collected, "TRAILER!!!") == 0) {
 		free_hash();
-		return 0;
+		return s;
 	}
 	clean_path(collected, mode);
 	if (S_ISREG(mode)) {
@@ -339,14 +336,14 @@ static int __init do_name(void)
 				openflags |= O_TRUNC;
 			wfile = filp_open(collected, openflags, mode);
 			if (IS_ERR(wfile))
-				return 0;
+				return s;
 			wfile_pos = 0;

 			vfs_fchown(wfile, uid, gid);
 			vfs_fchmod(wfile, mode);
 			if (body_len)
 				vfs_truncate(&wfile->f_path, body_len);
-			state = CopyFile;
+			s = CopyFile;
 		}
 	} else if (S_ISDIR(mode)) {
 		init_mkdir(collected, mode);
@@ -362,7 +359,7 @@ static int __init do_name(void)
 			do_utime(collected, mtime);
 		}
 	}
-	return 0;
+	return s;
 }

 static int __init do_copy(void)
@@ -378,14 +375,13 @@ static int __init do_copy(void)

 		fput(wfile);
 		eat(body_len);
-		state = SkipIt;
-		return 0;
+		return SkipIt;
 	} else {
 		if (xwrite(wfile, victim, byte_count, &wfile_pos) != byte_count)
 			error("write error");
 		body_len -= byte_count;
 		eat(byte_count);
-		return 1;
+		return Stop;
 	}
 }

@@ -396,29 +392,33 @@ static int __init do_symlink(void)
 	init_symlink(collected + N_ALIGN(name_len), collected);
 	init_chown(collected, uid, gid, AT_SYMLINK_NOFOLLOW);
 	do_utime(collected, mtime);
-	state = SkipIt;
 	next_state = Reset;
-	return 0;
+	return SkipIt;
 }

-static __initdata int (*actions[])(void) = {
-	[Start]		= do_start,
-	[Collect]	= do_collect,
-	[GotHeader]	= do_header,
-	[SkipIt]	= do_skip,
-	[GotName]	= do_name,
-	[CopyFile]	= do_copy,
-	[GotSymlink]	= do_symlink,
-	[Reset]		= do_reset,
-};
-
 static long __init write_buffer(char *buf, unsigned long len)
 {
+	int s = state;
+	int save;
+
 	byte_count = len;
 	victim = buf;

-	while (!actions[state]())
-		;
+	do {
+		save = s;
+		switch (s) {
+		case Start: s = do_start(); break;
+		case Collect: s = do_collect(); break;
+		case GotHeader: s = do_header(); break;
+		case SkipIt: s = do_skip(); break;
+		case GotName: s = do_name(); break;
+		case CopyFile: s = do_copy(); break;
+		case GotSymlink: s = do_symlink(); break;
+		case Reset: s = do_reset(); break;
+		}
+	} while (s != Stop);
+	state = save;
+
 	return len - byte_count;
 }

-- 
2.29.2

Powered by blists - more mailing lists