lkml.org 
[lkml]   [2022]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] kretprobe events missing on 2-core KVM guest
From
On 2022/11/7 21:36, Solar Designer wrote:
> On Wed, Oct 26, 2022 at 12:33:15AM +0900, Masami Hiramatsu wrote:
>> On Tue, 25 Oct 2022 18:01:17 +0800
>> wuqiang <wuqiang.matt@bytedance.com> wrote:
>>
>>> Default value of maxactive is set as num_possible_cpus() for nonpreemptable
>>> systems. For a 2-core system, only 2 kretprobe instances would be allocated
>>> in default, then these 2 instances for execve kretprobe are very likely to
>>> be used up with a pipelined command.
>>>
>>> This patch increases the minimum of maxactive to 10.
>>
>> This looks reasonable to me.
>>
>> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Reasonable yes, but:
>
> Is 10 enough? How exactly do those instances get used up without
> preemption? Perhaps because execve can sleep? If so, perhaps we should
> use the same logic without preemption that we do with preemption? So
> maybe just make this line unconditional? -
>
> rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());

I agree to make it unconditional, though it could cost a bit more memory.

Here's my testcase: a shell script was added to crontab, and the content of
the script is:

#!/bin/sh
do_something_with_magic `tr -dc a-z < /dev/urandom | head -c 10`

cron will trigger a series of program executions (4 times every hour). Then
we noticed events loss after 3-4 hours of testings.

The issue is caused by a burst of series of execve requests. The best number
of instances could be different case by case, and should be the user's duty
to decide, but num_possible_cpus() as a default value is inadequate. For my
testcase, 8 is enough as verified, and 10 is chosen to keep it identical.

The handling of execve syscall can be suspended or voluntarily yield up cpu
due to i/o or memory resources and then a new execve gets its time slice to
start. It could be worse for scenarios of resource throttling or routines
that are heavier than execve (though rare as I think).

> Also, the behavior was documented in Documentation/trace/kprobes.rst, so
> perhaps that file should be updated at the same time with the code.

Right, will update in next version.

>>> Signed-off-by: wuqiang <wuqiang.matt@bytedance.com>
>>> ---
>>> kernel/kprobes.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>> index 3220b0a2fb4a..b781dee3f552 100644
>>> --- a/kernel/kprobes.c
>>> +++ b/kernel/kprobes.c
>>> @@ -2211,7 +2211,7 @@ int register_kretprobe(struct kretprobe *rp)
>>> #ifdef CONFIG_PREEMPTION
>>> rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
>>> #else
>>> - rp->maxactive = num_possible_cpus();
>>> + rp->maxactive = max_t(unsigned int, 10, num_possible_cpus());
>>> #endif
>>> }
>>> #ifdef CONFIG_KRETPROBE_ON_RETHOOK
>>> --
>>> 2.34.1
>>>
>>
>>
>> --
>> Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Thanks,
>
> Alexander

Best regards,
wuqiang

\
 
 \ /
  Last update: 2022-11-08 03:51    [W:0.085 / U:0.824 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site