livepatch: Avoid CPU hogging with cond_resched

PATCH In review David Vernet 5 participants 10 messages Dec 29, 2021 – Jan 11, 2022 Live patching

Share & tools

David Vernet

author

2021-12-29 21:56 UTC #0 lore raw

PATCH 1/1 livepatch: Avoid CPU hogging with cond_resched

When initializing a 'struct klp_object' in klp_init_object_loaded(), and performing relocations in klp_resolve_symbols(), klp_find_object_symbol() is invoked to look up the address of a symbol in an already-loaded module (or vmlinux). This, in turn, calls kallsyms_on_each_symbol() or module_kallsyms_on_each_symbol() to find the address of the symbol that is being patched.

It turns out that symbol lookups often take up the most CPU time when enabling and disabling a patch, and may hog the CPU and cause other tasks on that CPU's runqueue to starve -- even in paths where interrupts are enabled. For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed. This may end up causing TCP retransmits on the host where the KLP patch is being applied, and in general, may cause any interrupts serviced by softirqd to be delayed while the patch is being applied.

So as to ensure that kallsyms_on_each_symbol() does not end up hogging the CPU, this patch adds a call to cond_resched() in kallsyms_on_each_symbol() and module_kallsyms_on_each_symbol(), which are invoked when doing a symbol lookup in vmlinux and a module respectively. Without this patch, if a live-patch is applied on a 36-core Intel host with heavy TCP traffic, a ~10x spike is observed in TCP retransmits while the patch is being applied. Additionally, collecting sched events with perf indicates that ksoftirqd is awakened ~1.3 seconds before it's eventually scheduled. With the patch, no increase in TCP retransmit events is observed, and ksoftirqd is scheduled shortly after it's awakened.

Signed-off-by David Vernet <[email protected]>

---
 kernel/kallsyms.c | 1 +
 kernel/module.c   | 2 ++
 2 files changed, 3 insertions(+)

kernel/kallsyms.c +1 -0

index 0ba87982d017..2a9afe484aec 100644
@@ -223,6 +223,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
 		ret = fn(data, namebuf, NULL, kallsyms_sym_address(i));
 		if (ret != 0)
 			return ret;
+		cond_resched();
 	}
 	return 0;
 }

kernel/module.c +2 -0

index 40ec9a030eec..c96160f7f3f5 100644
@@ -4462,6 +4462,8 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 				 mod, kallsyms_symbol_value(sym));
 			if (ret != 0)
 				goto out;
+
+			cond_resched();
 		}
 	}
 out:

Signature · 1 line

2.30.2

Miroslav Benes

maintainer Acked-by on v1 1/1

2022-01-05 10:17 UTC #1 lore raw

Quoted 28 lines · David Vernet

When initializing a 'struct klp_object' in klp_init_object_loaded(), and performing relocations in klp_resolve_symbols(), klp_find_object_symbol() is invoked to look up the address of a symbol in an already-loaded module (or vmlinux). This, in turn, calls kallsyms_on_each_symbol() or module_kallsyms_on_each_symbol() to find the address of the symbol that is being patched.
It turns out that symbol lookups often take up the most CPU time when enabling and disabling a patch, and may hog the CPU and cause other tasks on that CPU's runqueue to starve -- even in paths where interrupts are enabled. For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed. This may end up causing TCP retransmits on the host where the KLP patch is being applied, and in general, may cause any interrupts serviced by softirqd to be delayed while the patch is being applied.
So as to ensure that kallsyms_on_each_symbol() does not end up hogging the CPU, this patch adds a call to cond_resched() in kallsyms_on_each_symbol() and module_kallsyms_on_each_symbol(), which are invoked when doing a symbol lookup in vmlinux and a module respectively. Without this patch, if a live-patch is applied on a 36-core Intel host with heavy TCP traffic, a ~10x spike is observed in TCP retransmits while the patch is being applied. Additionally, collecting sched events with perf indicates that ksoftirqd is awakened ~1.3 seconds before it's eventually scheduled. With the patch, no increase in TCP retransmit events is observed, and ksoftirqd is scheduled shortly after it's awakened.

Quoted 1 line

Signed-off-by David Vernet <[email protected]>

Acked-by Miroslav Benes <[email protected]>

I had similar ideas Petr mentioned elsewhere, but I, also, have no strong opinion about it. Especially when livepatch is the only user of the said interface.

Song Liu

maintainer Acked-by on v1 1/1

2022-01-07 00:21 UTC #2 lore raw

In reply to David Vernet

Quoted 29 lines · David Vernet

When initializing a 'struct klp_object' in klp_init_object_loaded(), and performing relocations in klp_resolve_symbols(), klp_find_object_symbol() is invoked to look up the address of a symbol in an already-loaded module (or vmlinux). This, in turn, calls kallsyms_on_each_symbol() or module_kallsyms_on_each_symbol() to find the address of the symbol that is being patched.
It turns out that symbol lookups often take up the most CPU time when enabling and disabling a patch, and may hog the CPU and cause other tasks on that CPU's runqueue to starve -- even in paths where interrupts are enabled. For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed. This may end up causing TCP retransmits on the host where the KLP patch is being applied, and in general, may cause any interrupts serviced by softirqd to be delayed while the patch is being applied.
So as to ensure that kallsyms_on_each_symbol() does not end up hogging the CPU, this patch adds a call to cond_resched() in kallsyms_on_each_symbol() and module_kallsyms_on_each_symbol(), which are invoked when doing a symbol lookup in vmlinux and a module respectively. Without this patch, if a live-patch is applied on a 36-core Intel host with heavy TCP traffic, a ~10x spike is observed in TCP retransmits while the patch is being applied. Additionally, collecting sched events with perf indicates that ksoftirqd is awakened ~1.3 seconds before it's eventually scheduled. With the patch, no increase in TCP retransmit events is observed, and ksoftirqd is scheduled shortly after it's awakened.

Quoted 1 line

Signed-off-by David Vernet <[email protected]>

Acked-by Song Liu <[email protected]>

PS: Do we observe livepatch takes a longer time to load after this change? (I believe longer time shouldn't be a problem at all. Just curious.)

Petr Mladek

maintainer

2022-01-07 08:17 UTC #3 lore raw

Quoted 30 lines · Song Liu

Quoted 29 lines · David Vernet
When initializing a 'struct klp_object' in klp_init_object_loaded(), and performing relocations in klp_resolve_symbols(), klp_find_object_symbol() is invoked to look up the address of a symbol in an already-loaded module (or vmlinux). This, in turn, calls kallsyms_on_each_symbol() or module_kallsyms_on_each_symbol() to find the address of the symbol that is being patched.
It turns out that symbol lookups often take up the most CPU time when enabling and disabling a patch, and may hog the CPU and cause other tasks on that CPU's runqueue to starve -- even in paths where interrupts are enabled. For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed. This may end up causing TCP retransmits on the host where the KLP patch is being applied, and in general, may cause any interrupts serviced by softirqd to be delayed while the patch is being applied.
So as to ensure that kallsyms_on_each_symbol() does not end up hogging the CPU, this patch adds a call to cond_resched() in kallsyms_on_each_symbol() and module_kallsyms_on_each_symbol(), which are invoked when doing a symbol lookup in vmlinux and a module respectively. Without this patch, if a live-patch is applied on a 36-core Intel host with heavy TCP traffic, a ~10x spike is observed in TCP retransmits while the patch is being applied. Additionally, collecting sched events with perf indicates that ksoftirqd is awakened ~1.3 seconds before it's eventually scheduled. With the patch, no increase in TCP retransmit events is observed, and ksoftirqd is scheduled shortly after it's awakened.

Quoted 6 lines

Quoted 1 line
Signed-off-by David Vernet <[email protected]>
Acked-by Song Liu <[email protected]>
PS: Do we observe livepatch takes a longer time to load after this change? (I believe longer time shouldn't be a problem at all. Just curious.)

It should depend on the load of the system and the number of patched symbols. The module is typically loaded with a normal priority process.

The commit message talks about 1.3 seconds delay of ksoftirq. In principle, the change caused that this 1.3 sec of a single CPU time was interleaved with other scheduled tasks on the same CPU. I would expect that it prolonged the load just by a couple of seconds in the described use case.

Note that the change has effect only with voluntary scheduling. Well, it is typically used on servers where the livepatching makes sense.

Best Regards,
Petr

David Vernet

author

2022-01-10 14:56 UTC #4 lore raw

Petr Mladek <[email protected]> wrote on Fri [2022-Jan-07 09:17:52 +0100]:

Quoted 8 lines · Song Liu

Quoted 2 lines
PS: Do we observe livepatch takes a longer time to load after this change? (I believe longer time shouldn't be a problem at all. Just curious.)
It should depend on the load of the system and the number of patched symbols. The module is typically loaded with a normal priority process.

Quoted 5 lines

The commit message talks about 1.3 seconds delay of ksoftirq. In principle, the change caused that this 1.3 sec of a single CPU time was interleaved with other scheduled tasks on the same CPU. I would expect that it prolonged the load just by a couple of seconds in the described use case.

Just to add a data point here for the record, I didn't observe any increase in latency when applying a livepatch with this change. It took roughly ~2 +/- ~.5 seconds both with and without the change. Obviously that number doesn't mean much without knowing the specifics of the patch and what workloads were running on the host, but in general the invocations to cond_resched() patch did not seem to materially affect the overhead of livepatching.

The only time I would expect it to cause longer livepatches is when there are a lot of tasks that need the CPU, which is dependent on system load as Petr said. I'd argue that in this case, the patch would be working as intended as hogging the CPU for 1 or more seconds seems like a recipe for problems.

Petr Mladek

maintainer

2022-01-07 13:04 UTC #5 lore raw

In reply to David Vernet

Quoted 28 lines · David Vernet

When initializing a 'struct klp_object' in klp_init_object_loaded(), and performing relocations in klp_resolve_symbols(), klp_find_object_symbol() is invoked to look up the address of a symbol in an already-loaded module (or vmlinux). This, in turn, calls kallsyms_on_each_symbol() or module_kallsyms_on_each_symbol() to find the address of the symbol that is being patched.
It turns out that symbol lookups often take up the most CPU time when enabling and disabling a patch, and may hog the CPU and cause other tasks on that CPU's runqueue to starve -- even in paths where interrupts are enabled. For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed. This may end up causing TCP retransmits on the host where the KLP patch is being applied, and in general, may cause any interrupts serviced by softirqd to be delayed while the patch is being applied.
So as to ensure that kallsyms_on_each_symbol() does not end up hogging the CPU, this patch adds a call to cond_resched() in kallsyms_on_each_symbol() and module_kallsyms_on_each_symbol(), which are invoked when doing a symbol lookup in vmlinux and a module respectively. Without this patch, if a live-patch is applied on a 36-core Intel host with heavy TCP traffic, a ~10x spike is observed in TCP retransmits while the patch is being applied. Additionally, collecting sched events with perf indicates that ksoftirqd is awakened ~1.3 seconds before it's eventually scheduled. With the patch, no increase in TCP retransmit events is observed, and ksoftirqd is scheduled shortly after it's awakened.

Quoted 1 line

Signed-off-by David Vernet <[email protected]>

OK, there was not any strong pushback.

I have committed the patch into livepatch.git, branch for-5.17/kallsyms.

Best Regards,
Petr

Joe Lawrence

maintainer

2022-01-07 14:13 UTC #6 lore raw

In reply to David Vernet

Quoted 3 lines · David Vernet

For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed.

Just curious, approximately how many objects/functions does it take to hit this condition? While the livepatching kselftests are more about API and kernel correctness, this sounds like an interesting test case for some of the other (out of tree) test suites.

Thanks,

Signature · 1 line

Joe

Song Liu

maintainer

2022-01-07 16:46 UTC #7 lore raw

Quoted 10 lines · Joe Lawrence

Quoted 3 lines · David Vernet
For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed.
Just curious, approximately how many objects/functions does it take to hit this condition? While the livepatching kselftests are more about API and kernel correctness, this sounds like an interesting test case for some of the other (out of tree) test suites.

Not many patched functions. We only do small fixes at the moment. In the recent example, we hit the issue with ~10 patched functions. Another version with 2 to 3
patched function seems fine.

Yes, I think this is an important test case.

Song

Joe Lawrence

maintainer

2022-01-10 16:17 UTC #8 lore raw

Quoted 12 lines · Song Liu

Quoted 10 lines · Joe Lawrence
Quoted 3 lines · David Vernet
For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed.
Just curious, approximately how many objects/functions does it take to hit this condition? While the livepatching kselftests are more about API and kernel correctness, this sounds like an interesting test case for some of the other (out of tree) test suites.

Quoted 7 lines

Not many patched functions. We only do small fixes at the moment. In the recent example, we hit the issue with ~10 patched functions. Another version with 2 to 3
patched function seems fine.
Yes, I think this is an important test case.

Thanks, Song. If you can share any test setup details, I'll pass those along to our internal QE group. And once merged, we'll be adding this one to the list of backports for our distro.

Signature · 1 line

Joe

Song Liu

maintainer

2022-01-11 01:49 UTC #9 lore raw

Quoted 19 lines · Joe Lawrence

Quoted 17 lines · Song Liu
Quoted 10 lines · Joe Lawrence
Quoted 3 lines · David Vernet
For example, under certain workloads, enabling a KLP patch with many objects or functions may cause ksoftirqd to be starved, and thus for interrupts to be backlogged and delayed.
Just curious, approximately how many objects/functions does it take to hit this condition? While the livepatching kselftests are more about API and kernel correctness, this sounds like an interesting test case for some of the other (out of tree) test suites.
Not many patched functions. We only do small fixes at the moment. In the recent example, we hit the issue with ~10 patched functions. Another version with 2 to 3
patched function seems fine.

Quoted 6 lines

Quoted 2 lines
Yes, I think this is an important test case.
Thanks, Song. If you can share any test setup details, I'll pass those along to our internal QE group. And once merged, we'll be adding this one to the list of backports for our distro.

We get this on shadow production traffic of our web service, which we cannot recreate out of our data centers.
I tried to use iperf to generate traffic (among a few hosts), but it doesn't work.
At the moment, I am not sure what's the easiest way to repro this.

Thanks,
Song