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

[wuqiang]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>
>
> Thanks,
>
> Alexander

Best regards,
wuqiang