Skip to content

Commit 3e2a014

Browse files
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
1 parent 90e88b2 commit 3e2a014

File tree

2 files changed

+0
-23
lines changed

2 files changed

+0
-23
lines changed

src/efivarfs.c

-12
Original file line numberDiff line numberDiff line change
@@ -246,16 +246,6 @@ efivarfs_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
246246
int fd = -1;
247247
char *path = NULL;
248248
int rc;
249-
int ratelimit;
250-
251-
/*
252-
* The kernel rate limiter hits us if we go faster than 100 efi
253-
* variable reads per second as non-root. So if we're not root, just
254-
* delay this long after each read. The user is not going to notice.
255-
*
256-
* 1s / 100 = 10000us.
257-
*/
258-
ratelimit = geteuid() == 0 ? 0 : 10000;
259249

260250
rc = make_efivarfs_path(&path, guid, name);
261251
if (rc < 0) {
@@ -269,14 +259,12 @@ efivarfs_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
269259
goto err;
270260
}
271261

272-
usleep(ratelimit);
273262
rc = read(fd, &ret_attributes, sizeof (ret_attributes));
274263
if (rc < 0) {
275264
efi_error("read failed");
276265
goto err;
277266
}
278267

279-
usleep(ratelimit);
280268
rc = read_file(fd, &ret_data, &size);
281269
if (rc < 0) {
282270
efi_error("read_file failed");

src/vars.c

-11
Original file line numberDiff line numberDiff line change
@@ -290,16 +290,6 @@ vars_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
290290
char *path = NULL;
291291
int rc;
292292
int fd = -1;
293-
int ratelimit;
294-
295-
/*
296-
* The kernel rate limiter hits us if we go faster than 100 efi
297-
* variable reads per second as non-root. So if we're not root, just
298-
* delay this long after each read. The user is not going to notice.
299-
*
300-
* 1s / 100 = 10000us.
301-
*/
302-
ratelimit = geteuid() == 0 ? 0 : 10000;
303293

304294
rc = asprintf(&path, "%s%s-" GUID_FORMAT "/raw_var", get_vars_path(),
305295
name, GUID_FORMAT_ARGS(&guid));
@@ -314,7 +304,6 @@ vars_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
314304
goto err;
315305
}
316306

317-
usleep(ratelimit);
318307
rc = read_file(fd, &buf, &bufsize);
319308
if (rc < 0) {
320309
efi_error("read_file(%s) failed", path);

0 commit comments

Comments
 (0)