From f6baefaf9cdd1e9850a032c2b9622ea8fbcc0772 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D0=BD=D0=B0=D0=B1?= <nabijaczleweli@nabijaczleweli.xyz>
Date: Tue, 21 Nov 2023 14:44:53 +0100
Subject: [PATCH] Remove extraneous and broken self-imposed 20ms sleep
 (nominally-50/s-rate limit) for each EFI variable read
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <nabijaczleweli@nabijaczleweli.xyz>
---
 src/efivarfs.c | 12 ------------
 src/vars.c     | 11 -----------
 2 files changed, 23 deletions(-)

diff --git a/src/efivarfs.c b/src/efivarfs.c
index d4cee453..6483004d 100644
--- a/src/efivarfs.c
+++ b/src/efivarfs.c
@@ -246,16 +246,6 @@ efivarfs_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
 	int fd = -1;
 	char *path = NULL;
 	int rc;
-	int ratelimit;
-
-	/*
-	 * The kernel rate limiter hits us if we go faster than 100 efi
-	 * variable reads per second as non-root.  So if we're not root, just
-	 * delay this long after each read.  The user is not going to notice.
-	 *
-	 * 1s / 100 = 10000us.
-	 */
-	ratelimit = geteuid() == 0 ? 0 : 10000;
 
 	rc = make_efivarfs_path(&path, guid, name);
 	if (rc < 0) {
@@ -269,14 +259,12 @@ efivarfs_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
 		goto err;
 	}
 
-	usleep(ratelimit);
 	rc = read(fd, &ret_attributes, sizeof (ret_attributes));
 	if (rc < 0) {
 		efi_error("read failed");
 		goto err;
 	}
 
-	usleep(ratelimit);
 	rc = read_file(fd, &ret_data, &size);
 	if (rc < 0) {
 		efi_error("read_file failed");
diff --git a/src/vars.c b/src/vars.c
index ea37f85f..1df6a662 100644
--- a/src/vars.c
+++ b/src/vars.c
@@ -290,16 +290,6 @@ vars_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
 	char *path = NULL;
 	int rc;
 	int fd = -1;
-	int ratelimit;
-
-	/*
-	 * The kernel rate limiter hits us if we go faster than 100 efi
-	 * variable reads per second as non-root.  So if we're not root, just
-	 * delay this long after each read.  The user is not going to notice.
-	 *
-	 * 1s / 100 = 10000us.
-	 */
-	ratelimit = geteuid() == 0 ? 0 : 10000;
 
 	rc = asprintf(&path, "%s%s-" GUID_FORMAT "/raw_var", get_vars_path(),
 		      name, GUID_FORMAT_ARGS(&guid));
@@ -314,7 +304,6 @@ vars_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
 		goto err;
 	}
 
-	usleep(ratelimit);
 	rc = read_file(fd, &buf, &bufsize);
 	if (rc < 0) {
 		efi_error("read_file(%s) failed", path);