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

[Michael Ellerman]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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