linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] livepatch: Avoid CPU hogging with cond_resched
@ 2021-12-29 21:56 David Vernet
  2022-01-05 10:17 ` Miroslav Benes
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: David Vernet @ 2021-12-29 21:56 UTC (permalink / raw)
  To: live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes,
	joe.lawrence
  Cc: void

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 <void@manifault.com>
---
 kernel/kallsyms.c | 1 +
 kernel/module.c   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 0ba87982d017..2a9afe484aec 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -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;
 }
diff --git a/kernel/module.c b/kernel/module.c
index 40ec9a030eec..c96160f7f3f5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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:
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2021-12-29 21:56 [PATCH] livepatch: Avoid CPU hogging with cond_resched David Vernet
@ 2022-01-05 10:17 ` Miroslav Benes
  2022-01-07  0:21 ` Song Liu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Miroslav Benes @ 2022-01-05 10:17 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, pmladek, jikos, joe.lawrence

On Wed, 29 Dec 2021, David Vernet wrote:

> 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 <void@manifault.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

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.

M

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2021-12-29 21:56 [PATCH] livepatch: Avoid CPU hogging with cond_resched David Vernet
  2022-01-05 10:17 ` Miroslav Benes
@ 2022-01-07  0:21 ` Song Liu
  2022-01-07  8:17   ` Petr Mladek
  2022-01-07 13:03 ` Petr Mladek
  2022-01-07 14:13 ` Joe Lawrence
  3 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2022-01-07  0:21 UTC (permalink / raw)
  To: void
  Cc: live-patching, open list, jpoimboe, pmladek, jikos, mbenes, joe.lawrence

On Wed, Dec 29, 2021 at 1:57 PM David Vernet <void@manifault.com> wrote:
>
> 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 <void@manifault.com>

Acked-by: Song Liu <song@kernel.org>

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.)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2022-01-07  0:21 ` Song Liu
@ 2022-01-07  8:17   ` Petr Mladek
  2022-01-10 14:55     ` David Vernet
  0 siblings, 1 reply; 14+ messages in thread
From: Petr Mladek @ 2022-01-07  8:17 UTC (permalink / raw)
  To: Song Liu
  Cc: void, live-patching, open list, jpoimboe, jikos, mbenes, joe.lawrence

On Thu 2022-01-06 16:21:18, Song Liu wrote:
> On Wed, Dec 29, 2021 at 1:57 PM David Vernet <void@manifault.com> wrote:
> >
> > 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 <void@manifault.com>
> 
> Acked-by: Song Liu <song@kernel.org>
> 
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2021-12-29 21:56 [PATCH] livepatch: Avoid CPU hogging with cond_resched David Vernet
  2022-01-05 10:17 ` Miroslav Benes
  2022-01-07  0:21 ` Song Liu
@ 2022-01-07 13:03 ` Petr Mladek
  2022-01-07 14:13 ` Joe Lawrence
  3 siblings, 0 replies; 14+ messages in thread
From: Petr Mladek @ 2022-01-07 13:03 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence

On Wed 2021-12-29 13:56:47, David Vernet wrote:
> 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 <void@manifault.com>

OK, there was not any strong pushback.

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

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2021-12-29 21:56 [PATCH] livepatch: Avoid CPU hogging with cond_resched David Vernet
                   ` (2 preceding siblings ...)
  2022-01-07 13:03 ` Petr Mladek
@ 2022-01-07 14:13 ` Joe Lawrence
  2022-01-07 16:46   ` Song Liu
  3 siblings, 1 reply; 14+ messages in thread
From: Joe Lawrence @ 2022-01-07 14:13 UTC (permalink / raw)
  To: David Vernet, live-patching, linux-kernel, jpoimboe, pmladek,
	jikos, mbenes

On 12/29/21 4:56 PM, David Vernet wrote:
> 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,
-- 
Joe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2022-01-07 14:13 ` Joe Lawrence
@ 2022-01-07 16:46   ` Song Liu
  2022-01-10 16:16     ` Joe Lawrence
  0 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2022-01-07 16:46 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: David Vernet, live-patching, open list, jpoimboe, pmladek, jikos, mbenes

On Fri, Jan 7, 2022 at 6:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 12/29/21 4:56 PM, David Vernet wrote:
> > 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2022-01-07  8:17   ` Petr Mladek
@ 2022-01-10 14:55     ` David Vernet
  0 siblings, 0 replies; 14+ messages in thread
From: David Vernet @ 2022-01-10 14:55 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Song Liu, live-patching, open list, jpoimboe, jikos, mbenes,
	joe.lawrence

Petr Mladek <pmladek@suse.com> wrote on Fri [2022-Jan-07 09:17:52 +0100]:
> On Thu 2022-01-06 16:21:18, Song Liu wrote:
> > 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.

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2022-01-07 16:46   ` Song Liu
@ 2022-01-10 16:16     ` Joe Lawrence
  2022-01-11  1:49       ` Song Liu
  0 siblings, 1 reply; 14+ messages in thread
From: Joe Lawrence @ 2022-01-10 16:16 UTC (permalink / raw)
  To: Song Liu
  Cc: David Vernet, live-patching, open list, jpoimboe, pmladek, jikos, mbenes

On 1/7/22 11:46 AM, Song Liu wrote:
> On Fri, Jan 7, 2022 at 6:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>
>> On 12/29/21 4:56 PM, David Vernet wrote:
>>> 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.
> 

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.

-- 
Joe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2022-01-10 16:16     ` Joe Lawrence
@ 2022-01-11  1:49       ` Song Liu
  0 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2022-01-11  1:49 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: David Vernet, live-patching, open list, jpoimboe, pmladek, jikos, mbenes

On Mon, Jan 10, 2022 at 8:17 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 1/7/22 11:46 AM, Song Liu wrote:
> > On Fri, Jan 7, 2022 at 6:13 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >>
> >> On 12/29/21 4:56 PM, David Vernet wrote:
> >>> 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.
> >
>
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2022-01-03 16:04 ` Petr Mladek
@ 2022-01-10 14:38   ` David Vernet
  0 siblings, 0 replies; 14+ messages in thread
From: David Vernet @ 2022-01-10 14:38 UTC (permalink / raw)
  To: Petr Mladek
  Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes,
	joe.lawrence, linux-modules, mcgrof, jeyu, bpf, ast, daniel,
	andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh,
	netdev, memxor, clm

Apologies all for the delayed response -- I was still on holiday last week.

Petr Mladek <pmladek@suse.com> wrote on Mon [2022-Jan-03 17:04:31 +0100]:
> > > 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
>     ^^^^^^^^^^^^^^^^^^^^^^^^^
> This suggests that a single kallsyms_on_each_symbol() is not a big
> problem. cond_resched() might be called non-necessarily often there.
> I wonder if it would be enough to add cond_resched() into the two
> loops calling klp_find_object_symbol().

In the initial version of the patch I was intending to send out, I actually
had the cond_resched() in klp_find_object_symbol(). Having it there did
appear to fix the ksoftirqd starvation issue, but I elected to put it in
klp_find_object_symbol() after Chris (cc'd) suggested it because
cond_resched() is so lightweight, and it didn't affect the runtime for
livepatching in my experiments.

> That said, kallsyms_on_each_symbol() is a slow path and there might
> be many symbols. So, it might be the right place.

Yes, my thinking was that because it didn't seem to affect throughput, and
because it would could potentially cause the same ssue to occur if it were
ever called elsewhere, that this was the correct place for it.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2021-12-30  4:16 David Vernet
  2021-12-31 23:05 ` Kumar Kartikeya Dwivedi
@ 2022-01-03 16:04 ` Petr Mladek
  2022-01-10 14:38   ` David Vernet
  1 sibling, 1 reply; 14+ messages in thread
From: Petr Mladek @ 2022-01-03 16:04 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes,
	joe.lawrence, linux-modules, mcgrof, jeyu, bpf, ast, daniel,
	andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh,
	netdev, memxor

On Wed 2021-12-29 20:16:50, David Vernet wrote:
> Adding modules + BPF list and maintainers to this thread.
> 
> David Vernet <void@manifault.com> wrote on Wed [2021-Dec-29 13:56:47 -0800]:
> > 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
    ^^^^^^^^^^^^^^^^^^^^^^^^^
This suggests that a single kallsyms_on_each_symbol() is not a big
problem. cond_resched() might be called non-necessarily often there.
I wonder if it would be enough to add cond_resched() into the two
loops calling klp_find_object_symbol().

That said, kallsyms_on_each_symbol() is a slow path and there might
be many symbols. So, it might be the right place.

I am just thinking loudly. I do not have strong opinion. I am fine
with any cond_resched() location that solves the problem. Feel free
to use:

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr


> > 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 <void@manifault.com>
> > ---
> >  kernel/kallsyms.c | 1 +
> >  kernel/module.c   | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 0ba87982d017..2a9afe484aec 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -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;
> >  }
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 40ec9a030eec..c96160f7f3f5 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -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:
> > -- 
> > 2.30.2
> > 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
  2021-12-30  4:16 David Vernet
@ 2021-12-31 23:05 ` Kumar Kartikeya Dwivedi
  2022-01-03 16:04 ` Petr Mladek
  1 sibling, 0 replies; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-12-31 23:05 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes,
	joe.lawrence, linux-modules, mcgrof, jeyu, bpf, ast, daniel,
	andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh,
	netdev

On Thu, Dec 30, 2021 at 09:46:50AM IST, David Vernet wrote:
> Adding modules + BPF list and maintainers to this thread.
>

Thanks for the Cc, but I'm dropping my patch for the next revision, so there
should be no conflicts with this one.

> [...]

--
Kartikeya

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] livepatch: Avoid CPU hogging with cond_resched
@ 2021-12-30  4:16 David Vernet
  2021-12-31 23:05 ` Kumar Kartikeya Dwivedi
  2022-01-03 16:04 ` Petr Mladek
  0 siblings, 2 replies; 14+ messages in thread
From: David Vernet @ 2021-12-30  4:16 UTC (permalink / raw)
  To: live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes,
	joe.lawrence
  Cc: linux-modules, mcgrof, jeyu, bpf, ast, daniel, andrii, kafai,
	songliubraving, yhs, john.fastabend, kpsingh, netdev, memxor

Adding modules + BPF list and maintainers to this thread.

David Vernet <void@manifault.com> wrote on Wed [2021-Dec-29 13:56:47 -0800]:
> 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 <void@manifault.com>
> ---
>  kernel/kallsyms.c | 1 +
>  kernel/module.c   | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 0ba87982d017..2a9afe484aec 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -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;
>  }
> diff --git a/kernel/module.c b/kernel/module.c
> index 40ec9a030eec..c96160f7f3f5 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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:
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-01-11  1:49 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-29 21:56 [PATCH] livepatch: Avoid CPU hogging with cond_resched David Vernet
2022-01-05 10:17 ` Miroslav Benes
2022-01-07  0:21 ` Song Liu
2022-01-07  8:17   ` Petr Mladek
2022-01-10 14:55     ` David Vernet
2022-01-07 13:03 ` Petr Mladek
2022-01-07 14:13 ` Joe Lawrence
2022-01-07 16:46   ` Song Liu
2022-01-10 16:16     ` Joe Lawrence
2022-01-11  1:49       ` Song Liu
2021-12-30  4:16 David Vernet
2021-12-31 23:05 ` Kumar Kartikeya Dwivedi
2022-01-03 16:04 ` Petr Mladek
2022-01-10 14:38   ` David Vernet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).