Commits on Nov 21, 2023

1. Remove extraneous and broken self-imposed 20ms sleep (nominally-50/s-… …

   …rate limit) for each EFI variable read
   
   Nominally this rate limit is defined to avoid... getting rate-limited?
   
   But it already severely limits the rate to unusable
   ‒ on two of my real systems this makes efibootmgr take 168ms/194ms,
     which accounts for 95%/82% of the run-time
     (and this is under strace, so it's 100% of the run-time) ‒
   for klapki 0.2, this accounts for 36% and a large (140ms!)
   start-up delay, and for klapki 0.3 it's well over 50%.
   Well before you'd ever run afoul of the real limit.
   
   Discounting "20ms" as "The user is not going to notice." is baffling.
   efibootmgr is infuriatingly slow. 20ms is ping-to-america level.
   
   Worse yet, the entire kernel rate-limiter amounts to fs/efivarfs/file.c
   -- >8 --
     static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
                     size_t count, loff_t *ppos)
     {
             struct efivar_entry *var = file->private_data;
             unsigned long datasize = 0;
             u32 attributes;
             void *data;
             ssize_t size = 0;
             int err;
   
             while (!__ratelimit(&file->f_cred->user->ratelimit))
                     msleep(50);
   -- >8 --
   this is the alloc_uid() ratelimit with 1s interval + 100 burst.
   
   This means that we can (best-case) read 50 variables
   (read(...), read(0)) instantly, then do so again the next second.
   
   Best-case because the current implementation is broken anyway:
   it sleeps for 10ms after the attribute read (sure),
   and then for 10ms after the /two/ reads to read the rest of the
   variable (bad).
   
   This limits libefivar to 33⅓ variables per second.
   
   Most systems have roughly this many variables. Most programs only
   care about a very thin subset of them, and scarcely come close to
   reading enough to run afoul of the kernel limit. But even if they
   did, this limit is /significantly harsher/ than the kernel limit ‒
   it doesn't increase it (how could it? the limit's already there!),
   but severely increases latency for /every single read/, instead of
   just those over the rate.
   It's strictly worse than just not doing it.
   
   This was confirmed experimentally with strace -TTTT /bin/wc * * * * *
   (note the many every-variable expansions so it's noticeable):
   there is both visually a very obvious "big burst, little slowdown"
   oscillation, but also (non-efivarfs reads filtered out)
     $ awk '/^read/ {print $NF}' ss | tr -d '<>' | sort -n | cut -c -6 | uniq -c | sort -n
           1 0.8998
           1 0.9015
           1 0.9581
           1 0.9585
           1 0.9586
           5 0.0013
           9 0.0005
           9 0.0012
          46 0.0011
          70 0.0010
          84 0.0008
          85 0.0009
         102 0.0006
         115 0.0007
   or indeed
    $ awk '/^read/ {print $NF}' ss | tr -d '<>' | sort -n | cut -c -5 | uniq -c | sort -n
          1 0.899
          1 0.901
          3 0.958
        130 0.001
        395 0.000
   (130+395)/2=262½ variables read in under a millisecond,
   and 4½ got limited.
   
   But, much more importantly, the first screenful was free:
   99% of programs that don't read every variable
   over and over and over, but fit well within the 33
   (klapki's 7 and efibootmgr's 8,
    this is with the firmware's base boot entries + two additional ones;
    there isn't a non-hypothetical system in existence with 25 more boot entries).
   
   Fixes: https://bugs.debian.org/1056344
   Signed-off-by: Ahelenia Ziemiańska <[email protected]>

   

   nabijaczleweli committed Nov 21, 2023
   Configuration menu

   • View commit details
   • Copy full SHA for f6baefa
   • Browse repository at this point

   Copy the full SHA

   f6baefa View commit details

   Browse the repository at this point in the history