Remove extraneous and broken self-imposed 20ms sleep (nominally-50/s-rate limit) for each EFI variable read#258
Open
nabijaczleweli wants to merge 1 commit intorhboot:mainfrom
Open
Remove extraneous and broken self-imposed 20ms sleep (nominally-50/s-rate limit) for each EFI variable read#258nabijaczleweli wants to merge 1 commit intorhboot:mainfrom
nabijaczleweli wants to merge 1 commit intorhboot:mainfrom
Conversation
nabijaczleweli
added a commit
to nabijaczleweli/efivar
that referenced
this pull request
Nov 21, 2023
Read the whole variable at once, then copy out the attributes (for a description of efivarfs rate limiting, see rhboot#258)
…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 <nabijaczleweli@nabijaczleweli.xyz>
nabijaczleweli
added a commit
to nabijaczleweli/efivar
that referenced
this pull request
Nov 21, 2023
Read the whole variable at once, then copy out the attributes (for a description of efivarfs rate limiting, see rhboot#258) Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Contributor
Author
|
DCO check clearly broken, the commit has a DCO signoff |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
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, this system has fewer than 50 variables): there is both visually a very obvious "big burst, little slowdown" oscillation, but also (non-efivarfs reads filtered out)or indeed
(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:
and