Re: [PATCH] kretprobe events missing on 2-core KVM guest

[wuqiang <wuqiang.matt@bytedance.com>]

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