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
>> <li...@rasmusvillemoes.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
