Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490309 > unrolled thread

Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic context

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Peter Zijlstra <peterz@infradead.org> - 2016-09-23 18:40 +0200
    Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Jiri Olsa <jolsa@redhat.com> - 2016-10-03 15:30 +0200
      Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Peter Zijlstra <peterz@infradead.org> - 2016-10-03 15:50 +0200
        Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic context Michael Ellerman <mpe@ellerman.id.au> - 2016-10-04 06:30 +0200
          Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Peter Zijlstra <peterz@infradead.org> - 2016-10-04 09:10 +0200
            Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Will Deacon <will.deacon@arm.com> - 2016-10-10 15:30 +0200
          Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Jiri Olsa <jolsa@redhat.com> - 2016-10-05 10:10 +0200
            Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Jiri Olsa <jolsa@redhat.com> - 2016-10-05 22:00 +0200
              Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic  context Peter Zijlstra <peterz@infradead.org> - 2016-10-06 09:30 +0200
                [PATCHv2] perf powerpc: Don't call perf_event_disable from atomic  context Jiri Olsa <jolsa@redhat.com> - 2016-10-06 14:40 +0200

#1490309 — Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic context

On Wed, Sep 21, 2016 at 03:55:34PM +0200, Jiri Olsa wrote:
> The trinity syscall fuzzer triggered following WARN on powerpc:
>   WARNING: CPU: 9 PID: 2998 at arch/powerpc/kernel/hw_breakpoint.c:278
>   ...
>   NIP [c00000000093aedc] .hw_breakpoint_handler+0x28c/0x2b0
>   LR [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0
>   Call Trace:
>   [c0000002f7933580] [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0 (unreliable)
>   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
>   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
>   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
>   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
>   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
> 
> Followed by lockdep warning:
>   ===============================
>   [ INFO: suspicious RCU usage. ]
>   4.8.0-rc5+ #7 Tainted: G        W
>   -------------------------------
>   ./include/linux/rcupdate.h:556 Illegal context switch in RCU read-side critical section!
> 
>   other info that might help us debug this:
> 
>   rcu_scheduler_active = 1, debug_locks = 0
>   2 locks held by ls/2998:
>    #0:  (rcu_read_lock){......}, at: [<c0000000000f6a00>] .__atomic_notifier_call_chain+0x0/0x1c0
>    #1:  (rcu_read_lock){......}, at: [<c00000000093ac50>] .hw_breakpoint_handler+0x0/0x2b0
> 
>   stack backtrace:
>   CPU: 9 PID: 2998 Comm: ls Tainted: G        W       4.8.0-rc5+ #7
>   Call Trace:
>   [c0000002f7933150] [c00000000094b1f8] .dump_stack+0xe0/0x14c (unreliable)
>   [c0000002f79331e0] [c00000000013c468] .lockdep_rcu_suspicious+0x138/0x180
>   [c0000002f7933270] [c0000000001005d8] .___might_sleep+0x278/0x2e0
>   [c0000002f7933300] [c000000000935584] .mutex_lock_nested+0x64/0x5a0
>   [c0000002f7933410] [c00000000023084c] .perf_event_ctx_lock_nested+0x16c/0x380
>   [c0000002f7933500] [c000000000230a80] .perf_event_disable+0x20/0x60
>   [c0000002f7933580] [c00000000093aeec] .hw_breakpoint_handler+0x29c/0x2b0
>   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
>   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
>   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
>   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
>   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
> 

Well, that lockdep warning only says you should not be taking sleeping
locks while holding rcu_read_lock(), which is true. It does not say the
context you're doing this is cannot sleep.

I'm not familiar enough with the PPC stuff to tell if the DIE_DABR_MATCH
trap context is atomic or not and this Changelog doesn't tell me.

Anybody?

[toc] | [next] | [standalone]

#1494879

On Fri, Sep 23, 2016 at 06:37:47PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 21, 2016 at 03:55:34PM +0200, Jiri Olsa wrote:
> > The trinity syscall fuzzer triggered following WARN on powerpc:
> >   WARNING: CPU: 9 PID: 2998 at arch/powerpc/kernel/hw_breakpoint.c:278
> >   ...
> >   NIP [c00000000093aedc] .hw_breakpoint_handler+0x28c/0x2b0
> >   LR [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0
> >   Call Trace:
> >   [c0000002f7933580] [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0 (unreliable)
> >   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
> >   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
> >   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
> >   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
> >   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
> > 
> > Followed by lockdep warning:
> >   ===============================
> >   [ INFO: suspicious RCU usage. ]
> >   4.8.0-rc5+ #7 Tainted: G        W
> >   -------------------------------
> >   ./include/linux/rcupdate.h:556 Illegal context switch in RCU read-side critical section!
> > 
> >   other info that might help us debug this:
> > 
> >   rcu_scheduler_active = 1, debug_locks = 0
> >   2 locks held by ls/2998:
> >    #0:  (rcu_read_lock){......}, at: [<c0000000000f6a00>] .__atomic_notifier_call_chain+0x0/0x1c0
> >    #1:  (rcu_read_lock){......}, at: [<c00000000093ac50>] .hw_breakpoint_handler+0x0/0x2b0
> > 
> >   stack backtrace:
> >   CPU: 9 PID: 2998 Comm: ls Tainted: G        W       4.8.0-rc5+ #7
> >   Call Trace:
> >   [c0000002f7933150] [c00000000094b1f8] .dump_stack+0xe0/0x14c (unreliable)
> >   [c0000002f79331e0] [c00000000013c468] .lockdep_rcu_suspicious+0x138/0x180
> >   [c0000002f7933270] [c0000000001005d8] .___might_sleep+0x278/0x2e0
> >   [c0000002f7933300] [c000000000935584] .mutex_lock_nested+0x64/0x5a0
> >   [c0000002f7933410] [c00000000023084c] .perf_event_ctx_lock_nested+0x16c/0x380
> >   [c0000002f7933500] [c000000000230a80] .perf_event_disable+0x20/0x60
> >   [c0000002f7933580] [c00000000093aeec] .hw_breakpoint_handler+0x29c/0x2b0
> >   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
> >   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
> >   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
> >   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
> >   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
> > 
> 
> Well, that lockdep warning only says you should not be taking sleeping
> locks while holding rcu_read_lock(), which is true. It does not say the
> context you're doing this is cannot sleep.
> 
> I'm not familiar enough with the PPC stuff to tell if the DIE_DABR_MATCH
> trap context is atomic or not and this Changelog doesn't tell me.
> 
> Anybody?

ping

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1494883

On Mon, Oct 03, 2016 at 03:29:32PM +0200, Jiri Olsa wrote:
> On Fri, Sep 23, 2016 at 06:37:47PM +0200, Peter Zijlstra wrote:
> > On Wed, Sep 21, 2016 at 03:55:34PM +0200, Jiri Olsa wrote:
> > > The trinity syscall fuzzer triggered following WARN on powerpc:
> > >   WARNING: CPU: 9 PID: 2998 at arch/powerpc/kernel/hw_breakpoint.c:278
> > >   ...
> > >   NIP [c00000000093aedc] .hw_breakpoint_handler+0x28c/0x2b0
> > >   LR [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0
> > >   Call Trace:
> > >   [c0000002f7933580] [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0 (unreliable)
> > >   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
> > >   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
> > >   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
> > >   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
> > >   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
> > > 
> > > Followed by lockdep warning:
> > >   ===============================
> > >   [ INFO: suspicious RCU usage. ]
> > >   4.8.0-rc5+ #7 Tainted: G        W
> > >   -------------------------------
> > >   ./include/linux/rcupdate.h:556 Illegal context switch in RCU read-side critical section!
> > > 
> > >   other info that might help us debug this:
> > > 
> > >   rcu_scheduler_active = 1, debug_locks = 0
> > >   2 locks held by ls/2998:
> > >    #0:  (rcu_read_lock){......}, at: [<c0000000000f6a00>] .__atomic_notifier_call_chain+0x0/0x1c0
> > >    #1:  (rcu_read_lock){......}, at: [<c00000000093ac50>] .hw_breakpoint_handler+0x0/0x2b0
> > > 
> > >   stack backtrace:
> > >   CPU: 9 PID: 2998 Comm: ls Tainted: G        W       4.8.0-rc5+ #7
> > >   Call Trace:
> > >   [c0000002f7933150] [c00000000094b1f8] .dump_stack+0xe0/0x14c (unreliable)
> > >   [c0000002f79331e0] [c00000000013c468] .lockdep_rcu_suspicious+0x138/0x180
> > >   [c0000002f7933270] [c0000000001005d8] .___might_sleep+0x278/0x2e0
> > >   [c0000002f7933300] [c000000000935584] .mutex_lock_nested+0x64/0x5a0
> > >   [c0000002f7933410] [c00000000023084c] .perf_event_ctx_lock_nested+0x16c/0x380
> > >   [c0000002f7933500] [c000000000230a80] .perf_event_disable+0x20/0x60
> > >   [c0000002f7933580] [c00000000093aeec] .hw_breakpoint_handler+0x29c/0x2b0
> > >   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
> > >   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
> > >   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
> > >   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
> > >   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
> > > 
> > 
> > Well, that lockdep warning only says you should not be taking sleeping
> > locks while holding rcu_read_lock(), which is true. It does not say the
> > context you're doing this is cannot sleep.
> > 
> > I'm not familiar enough with the PPC stuff to tell if the DIE_DABR_MATCH
> > trap context is atomic or not and this Changelog doesn't tell me.
> > 
> > Anybody?
> 
> ping

So I think all the DIE notifiers are atomic, which means this would
indeed be the thing to do. That said, I didn't see anything similar on
other BP implementations.

So it would be good to also explain why PPC needs this in the first
place.

[toc] | [prev] | [next] | [standalone]

#1495193 — Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic context

Peter Zijlstra <peterz@infradead.org> writes:

> On Mon, Oct 03, 2016 at 03:29:32PM +0200, Jiri Olsa wrote:
>> On Fri, Sep 23, 2016 at 06:37:47PM +0200, Peter Zijlstra wrote:
>> > On Wed, Sep 21, 2016 at 03:55:34PM +0200, Jiri Olsa wrote:
>> > >   stack backtrace:
>> > >   CPU: 9 PID: 2998 Comm: ls Tainted: G        W       4.8.0-rc5+ #7
>> > >   Call Trace:
>> > >   [c0000002f7933150] [c00000000094b1f8] .dump_stack+0xe0/0x14c (unreliable)
>> > >   [c0000002f79331e0] [c00000000013c468] .lockdep_rcu_suspicious+0x138/0x180
>> > >   [c0000002f7933270] [c0000000001005d8] .___might_sleep+0x278/0x2e0
>> > >   [c0000002f7933300] [c000000000935584] .mutex_lock_nested+0x64/0x5a0
>> > >   [c0000002f7933410] [c00000000023084c] .perf_event_ctx_lock_nested+0x16c/0x380
>> > >   [c0000002f7933500] [c000000000230a80] .perf_event_disable+0x20/0x60
>> > >   [c0000002f7933580] [c00000000093aeec] .hw_breakpoint_handler+0x29c/0x2b0
>> > >   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
>> > >   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
>> > >   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
>> > >   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
>> > >   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
>> > 
>> > Well, that lockdep warning only says you should not be taking sleeping
>> > locks while holding rcu_read_lock(), which is true. It does not say the
>> > context you're doing this is cannot sleep.
>> > 
>> > I'm not familiar enough with the PPC stuff to tell if the DIE_DABR_MATCH
>> > trap context is atomic or not and this Changelog doesn't tell me.
>> 
>> ping
>
> So I think all the DIE notifiers are atomic, which means this would
> indeed be the thing to do. That said, I didn't see anything similar on
> other BP implementations.

Seems everyone is being called from the same notifier, which is atomic,
but powerpc is the only arch that does perf_event_disable().

> So it would be good to also explain why PPC needs this in the first
> place.

Unfortunately I don't really know the code, and the original author is AWOL.

But AFAICS perf_event_disable() is only called here:

	if (!stepped) {
		WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
			"0x%lx will be disabled.", info->address);
		perf_event_disable(bp);
		goto out;
	}

Which is where we cope with the possibility that we couldn't emulate the
instruction that hit the breakpoint. Seems that is not an issue on x86,
or it's handled elsewhere?

We should fix emulate_step() if it failed to emulate something it
should have, but there will always be the possibility that it fails.

Instead of calling perf_event_disable() we could just add a flag to
arch_hw_breakpoint that says we hit an error on the event, and block
reinstalling it in arch_install_hw_breakpoint().

cheers

[toc] | [prev] | [next] | [standalone]

#1495232

On Tue, Oct 04, 2016 at 03:29:33PM +1100, Michael Ellerman wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> > So it would be good to also explain why PPC needs this in the first
> > place.
> 
> Unfortunately I don't really know the code, and the original author is AWOL.
> 
> But AFAICS perf_event_disable() is only called here:
> 
> 	if (!stepped) {
> 		WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
> 			"0x%lx will be disabled.", info->address);
> 		perf_event_disable(bp);
> 		goto out;
> 	}
> 
> Which is where we cope with the possibility that we couldn't emulate the
> instruction that hit the breakpoint. Seems that is not an issue on x86,
> or it's handled elsewhere?

I don't think x86 ever needs to emulate things on hw breakpoint
(although I could be mistaken), but I would expect ARM to maybe need
so, and I couldn't find a disable there either.

Will?

> We should fix emulate_step() if it failed to emulate something it
> should have, but there will always be the possibility that it fails.
> 
> Instead of calling perf_event_disable() we could just add a flag to
> arch_hw_breakpoint that says we hit an error on the event, and block
> reinstalling it in arch_install_hw_breakpoint().

Possible..

[toc] | [prev] | [next] | [standalone]

#1498316

On Tue, Oct 04, 2016 at 09:06:18AM +0200, Peter Zijlstra wrote:
> On Tue, Oct 04, 2016 at 03:29:33PM +1100, Michael Ellerman wrote:
> > Peter Zijlstra <peterz@infradead.org> writes:
> > > So it would be good to also explain why PPC needs this in the first
> > > place.
> > 
> > Unfortunately I don't really know the code, and the original author is AWOL.
> > 
> > But AFAICS perf_event_disable() is only called here:
> > 
> > 	if (!stepped) {
> > 		WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
> > 			"0x%lx will be disabled.", info->address);
> > 		perf_event_disable(bp);
> > 		goto out;
> > 	}
> > 
> > Which is where we cope with the possibility that we couldn't emulate the
> > instruction that hit the breakpoint. Seems that is not an issue on x86,
> > or it's handled elsewhere?
> 
> I don't think x86 ever needs to emulate things on hw breakpoint
> (although I could be mistaken), but I would expect ARM to maybe need
> so, and I couldn't find a disable there either.
> 
> Will?

We don't do any emulation, so no need for us to call perf_event_disable
in the hw_breakpoint "overflow" path. We do play some awful games to
fake up a single-step, but I don't think perf core needs to care about
it.

Will

[toc] | [prev] | [next] | [standalone]

#1495746

On Tue, Oct 04, 2016 at 03:29:33PM +1100, Michael Ellerman wrote:

SNIP

> Which is where we cope with the possibility that we couldn't emulate the
> instruction that hit the breakpoint. Seems that is not an issue on x86,
> or it's handled elsewhere?
> 
> We should fix emulate_step() if it failed to emulate something it
> should have, but there will always be the possibility that it fails.
> 
> Instead of calling perf_event_disable() we could just add a flag to
> arch_hw_breakpoint that says we hit an error on the event, and block
> reinstalling it in arch_install_hw_breakpoint().

ok, might be easier.. I'll check on that

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1496027

On Wed, Oct 05, 2016 at 10:09:21AM +0200, Jiri Olsa wrote:
> On Tue, Oct 04, 2016 at 03:29:33PM +1100, Michael Ellerman wrote:
> 
> SNIP
> 
> > Which is where we cope with the possibility that we couldn't emulate the
> > instruction that hit the breakpoint. Seems that is not an issue on x86,
> > or it's handled elsewhere?
> > 
> > We should fix emulate_step() if it failed to emulate something it
> > should have, but there will always be the possibility that it fails.
> > 
> > Instead of calling perf_event_disable() we could just add a flag to
> > arch_hw_breakpoint that says we hit an error on the event, and block
> > reinstalling it in arch_install_hw_breakpoint().
> 
> ok, might be easier.. I'll check on that

so staring on that I think disabling is the right way here.. 

we need the event to be unscheduled and not scheduled back
again, I don't see better way at the moment

jirka

[toc] | [prev] | [next] | [standalone]

#1496193

On Wed, Oct 05, 2016 at 09:53:38PM +0200, Jiri Olsa wrote:
> On Wed, Oct 05, 2016 at 10:09:21AM +0200, Jiri Olsa wrote:
> > On Tue, Oct 04, 2016 at 03:29:33PM +1100, Michael Ellerman wrote:
> > 
> > SNIP
> > 
> > > Which is where we cope with the possibility that we couldn't emulate the
> > > instruction that hit the breakpoint. Seems that is not an issue on x86,
> > > or it's handled elsewhere?
> > > 
> > > We should fix emulate_step() if it failed to emulate something it
> > > should have, but there will always be the possibility that it fails.
> > > 
> > > Instead of calling perf_event_disable() we could just add a flag to
> > > arch_hw_breakpoint that says we hit an error on the event, and block
> > > reinstalling it in arch_install_hw_breakpoint().
> > 
> > ok, might be easier.. I'll check on that
> 
> so staring on that I think disabling is the right way here.. 
> 
> we need the event to be unscheduled and not scheduled back
> again, I don't see better way at the moment

OK, can you resend the patch with updated Changelog that explains these
things?

[toc] | [prev] | [next] | [standalone]

#1496630 — [PATCHv2] perf powerpc: Don't call perf_event_disable from atomic context

On Thu, Oct 06, 2016 at 09:24:15AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 05, 2016 at 09:53:38PM +0200, Jiri Olsa wrote:
> > On Wed, Oct 05, 2016 at 10:09:21AM +0200, Jiri Olsa wrote:
> > > On Tue, Oct 04, 2016 at 03:29:33PM +1100, Michael Ellerman wrote:
> > > 
> > > SNIP
> > > 
> > > > Which is where we cope with the possibility that we couldn't emulate the
> > > > instruction that hit the breakpoint. Seems that is not an issue on x86,
> > > > or it's handled elsewhere?
> > > > 
> > > > We should fix emulate_step() if it failed to emulate something it
> > > > should have, but there will always be the possibility that it fails.
> > > > 
> > > > Instead of calling perf_event_disable() we could just add a flag to
> > > > arch_hw_breakpoint that says we hit an error on the event, and block
> > > > reinstalling it in arch_install_hw_breakpoint().
> > > 
> > > ok, might be easier.. I'll check on that
> > 
> > so staring on that I think disabling is the right way here.. 
> > 
> > we need the event to be unscheduled and not scheduled back
> > again, I don't see better way at the moment
> 
> OK, can you resend the patch with updated Changelog that explains these
> things?

attached

thanks,
jirka


---
The trinity syscall fuzzer triggered following WARN on powerpc:
  WARNING: CPU: 9 PID: 2998 at arch/powerpc/kernel/hw_breakpoint.c:278
  ...
  NIP [c00000000093aedc] .hw_breakpoint_handler+0x28c/0x2b0
  LR [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0
  Call Trace:
  [c0000002f7933580] [c00000000093aed8] .hw_breakpoint_handler+0x288/0x2b0 (unreliable)
  [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
  [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
  [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
  [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
  [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48

Followed by lockdep warning:
  ===============================
  [ INFO: suspicious RCU usage. ]
  4.8.0-rc5+ #7 Tainted: G        W
  -------------------------------
  ./include/linux/rcupdate.h:556 Illegal context switch in RCU read-side critical section!

  other info that might help us debug this:

  rcu_scheduler_active = 1, debug_locks = 0
  2 locks held by ls/2998:
   #0:  (rcu_read_lock){......}, at: [<c0000000000f6a00>] .__atomic_notifier_call_chain+0x0/0x1c0
   #1:  (rcu_read_lock){......}, at: [<c00000000093ac50>] .hw_breakpoint_handler+0x0/0x2b0

  stack backtrace:
  CPU: 9 PID: 2998 Comm: ls Tainted: G        W       4.8.0-rc5+ #7
  Call Trace:
  [c0000002f7933150] [c00000000094b1f8] .dump_stack+0xe0/0x14c (unreliable)
  [c0000002f79331e0] [c00000000013c468] .lockdep_rcu_suspicious+0x138/0x180
  [c0000002f7933270] [c0000000001005d8] .___might_sleep+0x278/0x2e0
  [c0000002f7933300] [c000000000935584] .mutex_lock_nested+0x64/0x5a0
  [c0000002f7933410] [c00000000023084c] .perf_event_ctx_lock_nested+0x16c/0x380
  [c0000002f7933500] [c000000000230a80] .perf_event_disable+0x20/0x60
  [c0000002f7933580] [c00000000093aeec] .hw_breakpoint_handler+0x29c/0x2b0
  [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
  [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
  [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
  [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
  [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48

While it looks like the first WARN is probably valid, the other one is
triggered by disabling event via perf_event_disable from atomic context.

The event is disabled here in case we were not able to emulate
the instruction that hit the breakpoint. By disabling the event
we unschedule the event and make sure it's not scheduled back.

But we can't call perf_event_disable from atomic context, instead
we need to use event's pending_disable irq_work way to disable it.

Adding new function for that:
  perf_event_disable_inatomic(event)

Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/powerpc/kernel/hw_breakpoint.c |  2 +-
 include/linux/perf_event.h          |  1 +
 kernel/events/core.c                | 11 ++++++++---
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index aec9a1b1d25b..4d3bcbbf626a 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -275,7 +275,7 @@ int __kprobes hw_breakpoint_handler(struct die_args *args)
 	if (!stepped) {
 		WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
 			"0x%lx will be disabled.", info->address);
-		perf_event_disable(bp);
+		perf_event_disable_inatomic(bp);
 		goto out;
 	}
 	/*
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 5c5362584aba..c794fd84a595 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1248,6 +1248,7 @@ extern u64 perf_swevent_set_period(struct perf_event *event);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
 extern void perf_event_disable_local(struct perf_event *event);
+extern void perf_event_disable_inatomic(struct perf_event *event);
 extern void perf_event_task_tick(void);
 #else /* !CONFIG_PERF_EVENTS: */
 static inline void *
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7c0d263f6bc5..3d650ccf4def 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1960,6 +1960,13 @@ void perf_event_disable(struct perf_event *event)
 }
 EXPORT_SYMBOL_GPL(perf_event_disable);
 
+void perf_event_disable_inatomic(struct perf_event *event)
+{
+	event->pending_kill    = POLL_HUP;
+	event->pending_disable = 1;
+	irq_work_queue(&event->pending);
+}
+
 static void perf_set_shadow_time(struct perf_event *event,
 				 struct perf_event_context *ctx,
 				 u64 tstamp)
@@ -7074,9 +7081,7 @@ static int __perf_event_overflow(struct perf_event *event,
 	event->pending_kill = POLL_IN;
 	if (events && atomic_dec_and_test(&event->event_limit)) {
 		ret = 1;
-		event->pending_kill = POLL_HUP;
-		event->pending_disable = 1;
-		irq_work_queue(&event->pending);
+		perf_event_disable_inatomic(event);
 	}
 
 	event->overflow_handler(event, data, regs);
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web