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

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

[wuqiang]

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.

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

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

[Masami Hiramatsu]

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>

Thank you!

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

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

[Solar Designer]

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());

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

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

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@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

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

[wuqiang]

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.

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

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

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

The issue is caused by a burst of series of execve requests. The best number
of kretprobe instances could be different case by case, and should be user's
duty to determine, but num_possible_cpus() as the default value is inadequate
especially for systems with small number of cpus.

This patch enables the logic for preemption as default, thus increases the
minimum of maxactive to 10 for nonpreemptable systems.

Signed-off-by: wuqiang <wuqiang.matt@bytedance.com>
---
 Documentation/trace/kprobes.rst |  3 +--
 kernel/kprobes.c                | 10 +++-------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/Documentation/trace/kprobes.rst b/Documentation/trace/kprobes.rst
index 48cf778a2468..fc7ce76eab65 100644
--- a/Documentation/trace/kprobes.rst
+++ b/Documentation/trace/kprobes.rst
@@ -131,8 +131,7 @@ For example, if the function is non-recursive and is called with a
 spinlock held, maxactive = 1 should be enough.  If the function is
 non-recursive and can never relinquish the CPU (e.g., via a semaphore
 or preemption), NR_CPUS should be enough.  If maxactive <= 0, it is
-set to a default value.  If CONFIG_PREEMPT is enabled, the default
-is max(10, 2*NR_CPUS).  Otherwise, the default is NR_CPUS.
+set to a default value: max(10, 2*NR_CPUS).
 
 It's not a disaster if you set maxactive too low; you'll just miss
 some probes.  In the kretprobe struct, the nmissed field is set to
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a8b202f87e2d..1e80bddf2654 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2212,11 +2212,7 @@ int register_kretprobe(struct kretprobe *rp)
 	rp->kp.post_handler = NULL;
 
 	/* Pre-allocate memory for max kretprobe instances */
-	if (rp->maxactive <= 0) {
-#ifdef CONFIG_PREEMPTION
+	if (rp->maxactive <= 0)
 		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
-#else
-		rp->maxactive = num_possible_cpus();
-#endif
-	}
+
 #ifdef CONFIG_KRETPROBE_ON_RETHOOK
--
2.34.1

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

[Solar Designer]

On Thu, Nov 10, 2022 at 04:15:02PM +0800, wuqiang 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.
> 
> Here's the testcase: a shell script was added to crontab, and the content
> of the script is:
> 
>   #!/bin/sh
>   do_something_magic `tr -dc a-z < /dev/urandom | head -c 10`
> 
> cron will trigger a series of program executions (4 times every hour). Then
> events loss would be noticed normally after 3-4 hours of testings.
> 
> The issue is caused by a burst of series of execve requests. The best number
> of kretprobe instances could be different case by case, and should be user's
> duty to determine, but num_possible_cpus() as the default value is inadequate
> especially for systems with small number of cpus.
> 
> This patch enables the logic for preemption as default, thus increases the
> minimum of maxactive to 10 for nonpreemptable systems.
> 
> Signed-off-by: wuqiang <wuqiang.matt@bytedance.com>

Reviewed-by: Solar Designer <solar@openwall.com>

Thank you!

> ---
>  Documentation/trace/kprobes.rst |  3 +--
>  kernel/kprobes.c                | 10 +++-------
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/trace/kprobes.rst b/Documentation/trace/kprobes.rst
> index 48cf778a2468..fc7ce76eab65 100644
> --- a/Documentation/trace/kprobes.rst
> +++ b/Documentation/trace/kprobes.rst
> @@ -131,8 +131,7 @@ For example, if the function is non-recursive and is called with a
>  spinlock held, maxactive = 1 should be enough.  If the function is
>  non-recursive and can never relinquish the CPU (e.g., via a semaphore
>  or preemption), NR_CPUS should be enough.  If maxactive <= 0, it is
> -set to a default value.  If CONFIG_PREEMPT is enabled, the default
> -is max(10, 2*NR_CPUS).  Otherwise, the default is NR_CPUS.
> +set to a default value: max(10, 2*NR_CPUS).
>  
>  It's not a disaster if you set maxactive too low; you'll just miss
>  some probes.  In the kretprobe struct, the nmissed field is set to
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index a8b202f87e2d..1e80bddf2654 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2212,11 +2212,7 @@ int register_kretprobe(struct kretprobe *rp)
>  	rp->kp.post_handler = NULL;
>  
>  	/* Pre-allocate memory for max kretprobe instances */
> -	if (rp->maxactive <= 0) {
> -#ifdef CONFIG_PREEMPTION
> +	if (rp->maxactive <= 0)
>  		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
> -#else
> -		rp->maxactive = num_possible_cpus();
> -#endif
> -	}
> +
>  #ifdef CONFIG_KRETPROBE_ON_RETHOOK
> --
> 2.34.1

Alexander

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

[Masami Hiramatsu]

On Thu, 10 Nov 2022 15:52:23 +0100
Solar Designer <solar@openwall.com> wrote:

> On Thu, Nov 10, 2022 at 04:15:02PM +0800, wuqiang 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.
> > 
> > Here's the testcase: a shell script was added to crontab, and the content
> > of the script is:
> > 
> >   #!/bin/sh
> >   do_something_magic `tr -dc a-z < /dev/urandom | head -c 10`
> > 
> > cron will trigger a series of program executions (4 times every hour). Then
> > events loss would be noticed normally after 3-4 hours of testings.
> > 
> > The issue is caused by a burst of series of execve requests. The best number
> > of kretprobe instances could be different case by case, and should be user's
> > duty to determine, but num_possible_cpus() as the default value is inadequate
> > especially for systems with small number of cpus.
> > 
> > This patch enables the logic for preemption as default, thus increases the
> > minimum of maxactive to 10 for nonpreemptable systems.

Hm, OK. I think the enough value depends on where you put the probes on.
But maybe just making it NR_CPU -> 2 * NR_CPUS is reasonable.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> > 
> > Signed-off-by: wuqiang <wuqiang.matt@bytedance.com>
> 
> Reviewed-by: Solar Designer <solar@openwall.com>
> 
> Thank you!
> 
> > ---
> >  Documentation/trace/kprobes.rst |  3 +--
> >  kernel/kprobes.c                | 10 +++-------
> >  2 files changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/Documentation/trace/kprobes.rst b/Documentation/trace/kprobes.rst
> > index 48cf778a2468..fc7ce76eab65 100644
> > --- a/Documentation/trace/kprobes.rst
> > +++ b/Documentation/trace/kprobes.rst
> > @@ -131,8 +131,7 @@ For example, if the function is non-recursive and is called with a
> >  spinlock held, maxactive = 1 should be enough.  If the function is
> >  non-recursive and can never relinquish the CPU (e.g., via a semaphore
> >  or preemption), NR_CPUS should be enough.  If maxactive <= 0, it is
> > -set to a default value.  If CONFIG_PREEMPT is enabled, the default
> > -is max(10, 2*NR_CPUS).  Otherwise, the default is NR_CPUS.
> > +set to a default value: max(10, 2*NR_CPUS).
> >  
> >  It's not a disaster if you set maxactive too low; you'll just miss
> >  some probes.  In the kretprobe struct, the nmissed field is set to
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index a8b202f87e2d..1e80bddf2654 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2212,11 +2212,7 @@ int register_kretprobe(struct kretprobe *rp)
> >  	rp->kp.post_handler = NULL;
> >  
> >  	/* Pre-allocate memory for max kretprobe instances */
> > -	if (rp->maxactive <= 0) {
> > -#ifdef CONFIG_PREEMPTION
> > +	if (rp->maxactive <= 0)
> >  		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
> > -#else
> > -		rp->maxactive = num_possible_cpus();
> > -#endif
> > -	}
> > +
> >  #ifdef CONFIG_KRETPROBE_ON_RETHOOK
> > --
> > 2.34.1
> 
> Alexander


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>