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