Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1508097 > unrolled thread
| Started by | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| First post | 2016-10-25 11:10 +0200 |
| Last post | 2016-10-26 17:30 +0200 |
| Articles | 5 — 3 participants |
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.
Re: [lkp] [perf powerpc] 18d1796d0b: [No primary change] Peter Zijlstra <peterz@infradead.org> - 2016-10-25 11:10 +0200
Re: [LKP] [lkp] [perf powerpc] 18d1796d0b: [No primary change] "Huang\, Ying" <ying.huang@intel.com> - 2016-10-26 04:10 +0200
[PATCHv3] perf powerpc: Don't call perf_event_disable from atomic context Jiri Olsa <jolsa@redhat.com> - 2016-10-26 11:50 +0200
Re: [PATCHv3] perf powerpc: Don't call perf_event_disable from atomic context Peter Zijlstra <peterz@infradead.org> - 2016-10-26 17:20 +0200
Re: [PATCHv3] perf powerpc: Don't call perf_event_disable from atomic context Jiri Olsa <jolsa@redhat.com> - 2016-10-26 17:30 +0200
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-10-25 11:10 +0200 |
| Subject | Re: [lkp] [perf powerpc] 18d1796d0b: [No primary change] |
| Message-ID | <sw5U6-3BL-33@gated-at.bofh.it> |
On Tue, Oct 25, 2016 at 02:40:13PM +0800, kernel test robot wrote:
> [will-it-scale] perf-stat.branch-miss-rate +7.4% regression
> Reply-To: kernel test robot <xiaolong.ye@intel.com>
> User-Agent: Heirloom mailx 12.5 6/20/10
>
>
> FYI, we noticed a +7.4% regression of perf-stat.branch-miss-rate due to commit:
>
> commit 18d1796d0b45762ec6f58c5ed2ad3f7510ffbaa9 ("perf powerpc: Don't call perf_event_disable from atomic context")
> https://github.com/0day-ci/linux Jiri-Olsa/perf-powerpc-Don-t-call-perf_event_disable-from-atomic-context/20161006-203500
>
> in testcase: will-it-scale
> on test machine: 32 threads Intel(R) Xeon(R) CPU E5-2680 0 @ 2.70GHz with 64G memory
> with following parameters:
>
> test: poll2
> cpufreq_governor: performance
>
> Will It Scale takes a testcase and runs it from 1 through to n parallel copies to see if the testcase will scale. It builds both a process and threads based test in order to see any differences between the two.
> Details are as below:
> -------------------------------------------------------------------------------------------------->
>
>
> To reproduce:
>
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/wfg/lkp-tests.git
> cd lkp-tests
> bin/lkp install job.yaml # job file is attached in this email
> bin/lkp run job.yaml
>
> =========================================================================================
> compiler/cpufreq_governor/kconfig/rootfs/tbox_group/test/testcase:
> gcc-6/performance/x86_64-rhel-7.2/debian-x86_64-2016-08-31.cgz/lkp-sb03/poll2/will-it-scale
>
> commit:
> 41aad2a6d4 (" perf/core improvements and fixes:")
> 18d1796d0b ("perf powerpc: Don't call perf_event_disable from atomic context")
>
> 41aad2a6d4fcdda8 18d1796d0b45762ec6f58c5ed2
> ---------------- --------------------------
> fail:runs %reproduction fail:runs
> | | |
> %stddev %change %stddev
> \ | \
> 0.19 ± 0% +7.4% 0.21 ± 0% perf-stat.branch-miss-rate%
> 9.591e+09 ± 1% +9.1% 1.047e+10 ± 0% perf-stat.branch-misses
> 1.962e+09 ± 0% +2.3% 2.008e+09 ± 1% perf-stat.cache-references
> 51.18 ± 2% +5.6% 54.06 ± 1% perf-stat.iTLB-load-miss-rate%
> 46430577 ± 5% -6.9% 43241506 ± 2% perf-stat.iTLB-loads
> 9.90 ± 4% +9.3% 10.82 ± 4% turbostat.Pkg%pc2
> 62066 ± 24% +34.7% 83582 ± 11% numa-meminfo.node1.Active
> 49531 ± 30% +42.9% 70778 ± 13% numa-meminfo.node1.Active(anon)
> 27883 ±100% -100.0% 0.00 ± -1% latency_stats.avg.proc_cgroup_show.proc_single_show.seq_read.__vfs_read.vfs_read.SyS_read.do_syscall_64.return_from_SYSCALL_64
> 27883 ±100% -100.0% 0.00 ± -1% latency_stats.max.proc_cgroup_show.proc_single_show.seq_read.__vfs_read.vfs_read.SyS_read.do_syscall_64.return_from_SYSCALL_64
> 32685 ± 38% +88.5% 61603 ±147% latency_stats.sum.call_rwsem_down_write_failed.path_openat.do_filp_open.do_sys_open.SyS_open.entry_SYSCALL_64_fastpath
> 27883 ±100% -100.0% 0.00 ± -1% latency_stats.sum.proc_cgroup_show.proc_single_show.seq_read.__vfs_read.vfs_read.SyS_read.do_syscall_64.return_from_SYSCALL_64
> 92795 ± 4% -8.6% 84853 ± 6% numa-vmstat.node0.numa_hit
> 92782 ± 4% -8.5% 84851 ± 6% numa-vmstat.node0.numa_local
> 12381 ± 30% +42.9% 17694 ± 13% numa-vmstat.node1.nr_active_anon
> 12381 ± 30% +42.9% 17694 ± 13% numa-vmstat.node1.nr_zone_active_anon
> 21.80 ± 59% -69.8% 6.58 ± 83% sched_debug.cpu.clock.stddev
> 21.80 ± 59% -69.8% 6.58 ± 83% sched_debug.cpu.clock_task.stddev
> 0.00 ± 23% -34.3% 0.00 ± 20% sched_debug.cpu.next_balance.stddev
> 35829 ± 9% -18.4% 29221 ± 6% sched_debug.cpu.nr_switches.max
> 8361 ± 6% -13.4% 7243 ± 7% sched_debug.cpu.nr_switches.stddev
> 8.43 ± 11% -25.2% 6.30 ± 12% sched_debug.cpu.nr_uninterruptible.stddev
> 18057 ± 6% -14.3% 15482 ± 8% sched_debug.cpu.sched_count.stddev
>
ARGH... so what is the normal metric for this test and did that change?
And why can't I still find that? These reports suck!
The result doesn't make sense, my gcc inlines the function call, the
emitted code is very similar to the old code, with exception of one
extra symbol.
Are you sure this isn't simple run to run variation?
[toc] | [next] | [standalone]
| From | "Huang\, Ying" <ying.huang@intel.com> |
|---|---|
| Date | 2016-10-26 04:10 +0200 |
| Subject | Re: [LKP] [lkp] [perf powerpc] 18d1796d0b: [No primary change] |
| Message-ID | <swlPb-5zq-5@gated-at.bofh.it> |
| In reply to | #1508097 |
Peter Zijlstra <peterz@infradead.org> writes:
> On Tue, Oct 25, 2016 at 02:40:13PM +0800, kernel test robot wrote:
>> [will-it-scale] perf-stat.branch-miss-rate +7.4% regression
>> Reply-To: kernel test robot <xiaolong.ye@intel.com>
>> User-Agent: Heirloom mailx 12.5 6/20/10
>>
>>
>> FYI, we noticed a +7.4% regression of perf-stat.branch-miss-rate due to commit:
>>
>> commit 18d1796d0b45762ec6f58c5ed2ad3f7510ffbaa9 ("perf powerpc: Don't call perf_event_disable from atomic context")
>> https://github.com/0day-ci/linux Jiri-Olsa/perf-powerpc-Don-t-call-perf_event_disable-from-atomic-context/20161006-203500
>>
>> in testcase: will-it-scale
>> on test machine: 32 threads Intel(R) Xeon(R) CPU E5-2680 0 @ 2.70GHz with 64G memory
>> with following parameters:
>>
>> test: poll2
>> cpufreq_governor: performance
>>
>> Will It Scale takes a testcase and runs it from 1 through to n parallel copies to see if the testcase will scale. It builds both a process and threads based test in order to see any differences between the two.
>
>> Details are as below:
>> -------------------------------------------------------------------------------------------------->
>>
>>
>> To reproduce:
>>
>> git clone git://git.kernel.org/pub/scm/linux/kernel/git/wfg/lkp-tests.git
>> cd lkp-tests
>> bin/lkp install job.yaml # job file is attached in this email
>> bin/lkp run job.yaml
>>
>> =========================================================================================
>> compiler/cpufreq_governor/kconfig/rootfs/tbox_group/test/testcase:
>> gcc-6/performance/x86_64-rhel-7.2/debian-x86_64-2016-08-31.cgz/lkp-sb03/poll2/will-it-scale
>>
>> commit:
>> 41aad2a6d4 (" perf/core improvements and fixes:")
>> 18d1796d0b ("perf powerpc: Don't call perf_event_disable from atomic context")
>>
>> 41aad2a6d4fcdda8 18d1796d0b45762ec6f58c5ed2
>> ---------------- --------------------------
>> fail:runs %reproduction fail:runs
>> | | |
>> %stddev %change %stddev
>> \ | \
>> 0.19 . 0% +7.4% 0.21 . 0% perf-stat.branch-miss-rate%
>> 9.591e+09 . 1% +9.1% 1.047e+10 . 0% perf-stat.branch-misses
>> 1.962e+09 . 0% +2.3% 2.008e+09 . 1% perf-stat.cache-references
>> 51.18 . 2% +5.6% 54.06 . 1% perf-stat.iTLB-load-miss-rate%
>> 46430577 . 5% -6.9% 43241506 . 2% perf-stat.iTLB-loads
>> 9.90 . 4% +9.3% 10.82 . 4% turbostat.Pkg%pc2
>> 62066 . 24% +34.7% 83582 . 11% numa-meminfo.node1.Active
>> 49531 . 30% +42.9% 70778 . 13% numa-meminfo.node1.Active(anon)
>> 27883 .100% -100.0% 0.00 . -1% latency_stats.avg.proc_cgroup_show.proc_single_show.seq_read.__vfs_read.vfs_read.SyS_read.do_syscall_64.return_from_SYSCALL_64
>> 27883 .100% -100.0% 0.00 . -1% latency_stats.max.proc_cgroup_show.proc_single_show.seq_read.__vfs_read.vfs_read.SyS_read.do_syscall_64.return_from_SYSCALL_64
>> 32685 . 38% +88.5% 61603 .147% latency_stats.sum.call_rwsem_down_write_failed.path_openat.do_filp_open.do_sys_open.SyS_open.entry_SYSCALL_64_fastpath
>> 27883 .100% -100.0% 0.00 . -1% latency_stats.sum.proc_cgroup_show.proc_single_show.seq_read.__vfs_read.vfs_read.SyS_read.do_syscall_64.return_from_SYSCALL_64
>> 92795 . 4% -8.6% 84853 . 6% numa-vmstat.node0.numa_hit
>> 92782 . 4% -8.5% 84851 . 6% numa-vmstat.node0.numa_local
>> 12381 . 30% +42.9% 17694 . 13% numa-vmstat.node1.nr_active_anon
>> 12381 . 30% +42.9% 17694 . 13% numa-vmstat.node1.nr_zone_active_anon
>> 21.80 . 59% -69.8% 6.58 . 83% sched_debug.cpu.clock.stddev
>> 21.80 . 59% -69.8% 6.58 . 83% sched_debug.cpu.clock_task.stddev
>> 0.00 . 23% -34.3% 0.00 . 20% sched_debug.cpu.next_balance.stddev
>> 35829 . 9% -18.4% 29221 . 6% sched_debug.cpu.nr_switches.max
>> 8361 . 6% -13.4% 7243 . 7% sched_debug.cpu.nr_switches.stddev
>> 8.43 . 11% -25.2% 6.30 . 12% sched_debug.cpu.nr_uninterruptible.stddev
>> 18057 . 6% -14.3% 15482 . 8% sched_debug.cpu.sched_count.stddev
>>
>
> ARGH... so what is the normal metric for this test and did that change?
> And why can't I still find that? These reports suck!
There is observable changes between the benchmark (will-it-scale)
scores. That is said in the subject of the mail: "[No primary
change]". But apparently, that is not clear. We will improve that to
make it more clear.
> The result doesn't make sense, my gcc inlines the function call, the
> emitted code is very similar to the old code, with exception of one
> extra symbol.
>
> Are you sure this isn't simple run to run variation?
The reported change is perf-stat.branch-miss-rate%, which is changed
from 0.19% to 0.21%. That is too small. So, please ignore this
report. We will be more careful in the future.
Best Regards,
Huang, Ying
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-10-26 11:50 +0200 |
| Subject | [PATCHv3] perf powerpc: Don't call perf_event_disable from atomic context |
| Message-ID | <swt0l-1OS-5@gated-at.bofh.it> |
| In reply to | #1508753 |
On Wed, Oct 26, 2016 at 10:09:23AM +0800, Huang, Ying wrote:
SNIP
> > ARGH... so what is the normal metric for this test and did that change?
> > And why can't I still find that? These reports suck!
>
> There is observable changes between the benchmark (will-it-scale)
> scores. That is said in the subject of the mail: "[No primary
> change]". But apparently, that is not clear. We will improve that to
> make it more clear.
>
> > The result doesn't make sense, my gcc inlines the function call, the
> > emitted code is very similar to the old code, with exception of one
> > extra symbol.
> >
> > Are you sure this isn't simple run to run variation?
>
> The reported change is perf-stat.branch-miss-rate%, which is changed
> from 0.19% to 0.21%. That is too small. So, please ignore this
> report. We will be more careful in the future.
>
hi,
thanks for clarification
attaching v3 patch with complete changelog,
I tested and seems to work fine
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, kill)
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 9781c69eae57..58024eecbd9e 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -275,7 +275,7 @@ int 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, 0);
goto out;
}
/*
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 060d0ede88df..055bc837bfc1 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1257,6 +1257,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, int kill);
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 c6e47e97b33f..04477983945e 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, int kill)
+{
+ event->pending_kill = kill;
+ 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, POLL_HUP);
}
READ_ONCE(event->overflow_handler)(event, data, regs);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-10-26 17:20 +0200 |
| Subject | Re: [PATCHv3] perf powerpc: Don't call perf_event_disable from atomic context |
| Message-ID | <swy9H-5BD-13@gated-at.bofh.it> |
| In reply to | #1509009 |
On Wed, Oct 26, 2016 at 11:48:24AM +0200, Jiri Olsa wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c6e47e97b33f..04477983945e 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, int kill)
> +{
> + event->pending_kill = kill;
> + 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, POLL_HUP);
> }
So the pending_kill stuff is independent of the disable here. No need to
combine the two. I've change the patch as per the below.
That is, pending_kill is part of pending_wakeup, not of pending_disable.
Here we simply use both, its just that on disable we need a different
kind of wakeup (HANGUP instead of IN).
See how after ->overflow_handler() we send a wakeup if there's a
registered signal.
---
Subject: perf powerpc: Don't call perf_event_disable from atomic context
From: Jiri Olsa <jolsa@redhat.com>
Date: Wed, 26 Oct 2016 11:48:24 +0200
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.
Cc: Michael Neuling <mikey@neuling.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Huang Ying" <ying.huang@intel.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20161026094824.GA21397@krava
---
arch/powerpc/kernel/hw_breakpoint.c | 2 +-
include/linux/perf_event.h | 1 +
kernel/events/core.c | 10 ++++++++--
3 files changed, 10 insertions(+), 3 deletions(-)
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -275,7 +275,7 @@ int hw_breakpoint_handler(struct die_arg
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;
}
/*
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1257,6 +1257,7 @@ extern u64 perf_swevent_set_period(struc
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 *
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1960,6 +1960,12 @@ void perf_event_disable(struct perf_even
}
EXPORT_SYMBOL_GPL(perf_event_disable);
+void perf_event_disable_inatomic(struct perf_event *event)
+{
+ 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)
@@ -7075,8 +7081,8 @@ static int __perf_event_overflow(struct
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);
}
READ_ONCE(event->overflow_handler)(event, data, regs);
[toc] | [prev] | [next] | [standalone]
| From | Jiri Olsa <jolsa@redhat.com> |
|---|---|
| Date | 2016-10-26 17:30 +0200 |
| Subject | Re: [PATCHv3] perf powerpc: Don't call perf_event_disable from atomic context |
| Message-ID | <swyjo-5Fl-7@gated-at.bofh.it> |
| In reply to | #1509525 |
On Wed, Oct 26, 2016 at 05:12:49PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 26, 2016 at 11:48:24AM +0200, Jiri Olsa wrote:
>
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index c6e47e97b33f..04477983945e 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, int kill)
> > +{
> > + event->pending_kill = kill;
> > + 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, POLL_HUP);
> > }
>
> So the pending_kill stuff is independent of the disable here. No need to
> combine the two. I've change the patch as per the below.
>
> That is, pending_kill is part of pending_wakeup, not of pending_disable.
> Here we simply use both, its just that on disable we need a different
> kind of wakeup (HANGUP instead of IN).
>
> See how after ->overflow_handler() we send a wakeup if there's a
> registered signal.
ok, seems good
thanks,
jirka
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web