[V3,06/22] bpf/trace: Remove redundant preempt_disable from trace_call_bpf()
diff mbox series

Message ID 20200224145643.059995527@linutronix.de
State Accepted
Delegated to: BPF Maintainers
Headers show
Series
  • bpf: Make BPF and PREEMPT_RT co-exist
Related show

Commit Message

Thomas Gleixner Feb. 24, 2020, 2:01 p.m. UTC
Similar to __bpf_trace_run this is redundant because __bpf_trace_run() is
invoked from a trace point via __DO_TRACE() which already disables
preemption _before_ invoking any of the functions which are attached to a
trace point.

Remove it and add a cant_sleep() check.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V3: New patch. Replaces the previous one which converted this to migrate_disable() 
---
 kernel/trace/bpf_trace.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Alexei Starovoitov Feb. 24, 2020, 7:40 p.m. UTC | #1
On Mon, Feb 24, 2020 at 03:01:37PM +0100, Thomas Gleixner wrote:
> Similar to __bpf_trace_run this is redundant because __bpf_trace_run() is
> invoked from a trace point via __DO_TRACE() which already disables
> preemption _before_ invoking any of the functions which are attached to a
> trace point.
> 
> Remove it and add a cant_sleep() check.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V3: New patch. Replaces the previous one which converted this to migrate_disable() 
> ---
>  kernel/trace/bpf_trace.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -83,7 +83,7 @@ unsigned int trace_call_bpf(struct trace
>  	if (in_nmi()) /* not supported yet */
>  		return 1;
>  
> -	preempt_disable();
> +	cant_sleep();
>  
>  	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
>  		/*
> @@ -115,7 +115,6 @@ unsigned int trace_call_bpf(struct trace
>  
>   out:
>  	__this_cpu_dec(bpf_prog_active);
> -	preempt_enable();

My testing uncovered that above was too aggressive:
[   41.533438] BUG: assuming atomic context at kernel/trace/bpf_trace.c:86
[   41.534265] in_atomic(): 0, irqs_disabled(): 0, pid: 2348, name: test_progs
[   41.536907] Call Trace:
[   41.537167]  dump_stack+0x75/0xa0
[   41.537546]  __cant_sleep.cold.105+0x8b/0xa3
[   41.538018]  ? exit_to_usermode_loop+0x77/0x140
[   41.538493]  trace_call_bpf+0x4e/0x2e0
[   41.538908]  __uprobe_perf_func.isra.15+0x38f/0x690
[   41.539399]  ? probes_profile_seq_show+0x220/0x220
[   41.539962]  ? __mutex_lock_slowpath+0x10/0x10
[   41.540412]  uprobe_dispatcher+0x5de/0x8f0
[   41.540875]  ? uretprobe_dispatcher+0x7c0/0x7c0
[   41.541404]  ? down_read_killable+0x200/0x200
[   41.541852]  ? __kasan_kmalloc.constprop.6+0xc1/0xd0
[   41.542356]  uprobe_notify_resume+0xacf/0x1d60

The following fixes it:

commit 7b7b71ff43cc0b15567b60c38a951c8a2cbc97f0 (HEAD -> bpf-next)
Author: Alexei Starovoitov <ast@kernel.org>
Date:   Mon Feb 24 11:27:15 2020 -0800

    bpf: disable migration for bpf progs attached to uprobe

    trace_call_bpf() no longer disables preemption on its own.
    All callers of this function has to do it explicitly.

    Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 18d16f3ef980..7581f5eb6091 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1333,8 +1333,15 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
        int size, esize;
        int rctx;

-       if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))
-               return;
+       if (bpf_prog_array_valid(call)) {
+               u32 ret;
+
+               migrate_disable();
+               ret = trace_call_bpf(call, regs);
+               migrate_enable();
+               if (!ret)
+                       return;
+       }

But looking at your patch cant_sleep() seems unnecessary strong.
Should it be cant_migrate() instead?
And two calls to __this_cpu*() replaced with this_cpu*() ?
If you can ack it I can fix it up in place and apply the whole thing.
That was the only issue I found.
Thomas Gleixner Feb. 24, 2020, 8:42 p.m. UTC | #2
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> On Mon, Feb 24, 2020 at 03:01:37PM +0100, Thomas Gleixner wrote:
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -83,7 +83,7 @@ unsigned int trace_call_bpf(struct trace
>>  	if (in_nmi()) /* not supported yet */
>>  		return 1;
>>  
>> -	preempt_disable();
>> +	cant_sleep();
>>  
>>  	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
>>  		/*
>> @@ -115,7 +115,6 @@ unsigned int trace_call_bpf(struct trace
>>  
>>   out:
>>  	__this_cpu_dec(bpf_prog_active);
>> -	preempt_enable();
>
> My testing uncovered that above was too aggressive:
> [   41.533438] BUG: assuming atomic context at kernel/trace/bpf_trace.c:86
> [   41.534265] in_atomic(): 0, irqs_disabled(): 0, pid: 2348, name: test_progs
> [   41.536907] Call Trace:
> [   41.537167]  dump_stack+0x75/0xa0
> [   41.537546]  __cant_sleep.cold.105+0x8b/0xa3
> [   41.538018]  ? exit_to_usermode_loop+0x77/0x140
> [   41.538493]  trace_call_bpf+0x4e/0x2e0
> [   41.538908]  __uprobe_perf_func.isra.15+0x38f/0x690
> [   41.539399]  ? probes_profile_seq_show+0x220/0x220
> [   41.539962]  ? __mutex_lock_slowpath+0x10/0x10
> [   41.540412]  uprobe_dispatcher+0x5de/0x8f0
> [   41.540875]  ? uretprobe_dispatcher+0x7c0/0x7c0
> [   41.541404]  ? down_read_killable+0x200/0x200
> [   41.541852]  ? __kasan_kmalloc.constprop.6+0xc1/0xd0
> [   41.542356]  uprobe_notify_resume+0xacf/0x1d60

Duh. I missed that particular callchain.

> The following fixes it:
>
> commit 7b7b71ff43cc0b15567b60c38a951c8a2cbc97f0 (HEAD -> bpf-next)
> Author: Alexei Starovoitov <ast@kernel.org>
> Date:   Mon Feb 24 11:27:15 2020 -0800
>
>     bpf: disable migration for bpf progs attached to uprobe
>
>     trace_call_bpf() no longer disables preemption on its own.
>     All callers of this function has to do it explicitly.
>
>     Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 18d16f3ef980..7581f5eb6091 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1333,8 +1333,15 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
>         int size, esize;
>         int rctx;
>
> -       if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))
> -               return;
> +       if (bpf_prog_array_valid(call)) {
> +               u32 ret;
> +
> +               migrate_disable();
> +               ret = trace_call_bpf(call, regs);
> +               migrate_enable();
> +               if (!ret)
> +                       return;
> +       }
>
> But looking at your patch cant_sleep() seems unnecessary strong.
> Should it be cant_migrate() instead?

Yes, if we go with the migrate_disable(). OTOH, having a
preempt_disable() in that uprobe callsite should work as well, then we
can keep the cant_sleep() check which covers all other callsites
properly. No strong opinion though.

> And two calls to __this_cpu*() replaced with this_cpu*() ?

See above.

> If you can ack it I can fix it up in place and apply the whole thing.

Ack.

Thanks,

     tglx
Alexei Starovoitov Feb. 25, 2020, 12:33 a.m. UTC | #3
On Mon, Feb 24, 2020 at 09:42:52PM +0100, Thomas Gleixner wrote:
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> > On Mon, Feb 24, 2020 at 03:01:37PM +0100, Thomas Gleixner wrote:
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -83,7 +83,7 @@ unsigned int trace_call_bpf(struct trace
> >>  	if (in_nmi()) /* not supported yet */
> >>  		return 1;
> >>  
> >> -	preempt_disable();
> >> +	cant_sleep();
> >>  
> >>  	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> >>  		/*
> >> @@ -115,7 +115,6 @@ unsigned int trace_call_bpf(struct trace
> >>  
> >>   out:
> >>  	__this_cpu_dec(bpf_prog_active);
> >> -	preempt_enable();
> >
> > My testing uncovered that above was too aggressive:
> > [   41.533438] BUG: assuming atomic context at kernel/trace/bpf_trace.c:86
> > [   41.534265] in_atomic(): 0, irqs_disabled(): 0, pid: 2348, name: test_progs
> > [   41.536907] Call Trace:
> > [   41.537167]  dump_stack+0x75/0xa0
> > [   41.537546]  __cant_sleep.cold.105+0x8b/0xa3
> > [   41.538018]  ? exit_to_usermode_loop+0x77/0x140
> > [   41.538493]  trace_call_bpf+0x4e/0x2e0
> > [   41.538908]  __uprobe_perf_func.isra.15+0x38f/0x690
> > [   41.539399]  ? probes_profile_seq_show+0x220/0x220
> > [   41.539962]  ? __mutex_lock_slowpath+0x10/0x10
> > [   41.540412]  uprobe_dispatcher+0x5de/0x8f0
> > [   41.540875]  ? uretprobe_dispatcher+0x7c0/0x7c0
> > [   41.541404]  ? down_read_killable+0x200/0x200
> > [   41.541852]  ? __kasan_kmalloc.constprop.6+0xc1/0xd0
> > [   41.542356]  uprobe_notify_resume+0xacf/0x1d60
> 
> Duh. I missed that particular callchain.
> 
> > The following fixes it:
> >
> > commit 7b7b71ff43cc0b15567b60c38a951c8a2cbc97f0 (HEAD -> bpf-next)
> > Author: Alexei Starovoitov <ast@kernel.org>
> > Date:   Mon Feb 24 11:27:15 2020 -0800
> >
> >     bpf: disable migration for bpf progs attached to uprobe
> >
> >     trace_call_bpf() no longer disables preemption on its own.
> >     All callers of this function has to do it explicitly.
> >
> >     Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> >
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index 18d16f3ef980..7581f5eb6091 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -1333,8 +1333,15 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
> >         int size, esize;
> >         int rctx;
> >
> > -       if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))
> > -               return;
> > +       if (bpf_prog_array_valid(call)) {
> > +               u32 ret;
> > +
> > +               migrate_disable();
> > +               ret = trace_call_bpf(call, regs);
> > +               migrate_enable();
> > +               if (!ret)
> > +                       return;
> > +       }
> >
> > But looking at your patch cant_sleep() seems unnecessary strong.
> > Should it be cant_migrate() instead?
> 
> Yes, if we go with the migrate_disable(). OTOH, having a
> preempt_disable() in that uprobe callsite should work as well, then we
> can keep the cant_sleep() check which covers all other callsites
> properly. No strong opinion though.

ok. I went with preempt_disable() for uprobes. It's simpler.
And pushed the whole set to bpf-next.
In few days we'll send it to Dave for net-next and on the way
to Linus's next release. imo it's a big milestone.
Thank you for the hard work to make it happen.
Thomas Gleixner Feb. 25, 2020, 12:36 p.m. UTC | #4
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> On Mon, Feb 24, 2020 at 09:42:52PM +0100, Thomas Gleixner wrote:
>> > But looking at your patch cant_sleep() seems unnecessary strong.
>> > Should it be cant_migrate() instead?
>> 
>> Yes, if we go with the migrate_disable(). OTOH, having a
>> preempt_disable() in that uprobe callsite should work as well, then we
>> can keep the cant_sleep() check which covers all other callsites
>> properly. No strong opinion though.
>
> ok. I went with preempt_disable() for uprobes. It's simpler.
> And pushed the whole set to bpf-next.
> In few days we'll send it to Dave for net-next and on the way
> to Linus's next release. imo it's a big milestone.
> Thank you for the hard work to make it happen.

Thank you for guidance and review!

      tglx

Patch
diff mbox series

--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -83,7 +83,7 @@  unsigned int trace_call_bpf(struct trace
 	if (in_nmi()) /* not supported yet */
 		return 1;
 
-	preempt_disable();
+	cant_sleep();
 
 	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
 		/*
@@ -115,7 +115,6 @@  unsigned int trace_call_bpf(struct trace
 
  out:
 	__this_cpu_dec(bpf_prog_active);
-	preempt_enable();
 
 	return ret;
 }