* [PATCH 00/12] perf: more fixes
@ 2016-02-24 17:45 Peter Zijlstra
2016-02-24 17:45 ` [PATCH 01/12] perf: Close install vs exit race Peter Zijlstra
` (12 more replies)
0 siblings, 13 replies; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
About half of these patches are syz-kaller inspired, the rest is reported
fallout from the last round of fixes.
With these patches syz-kaller can still trigger some fail; most notably some
NMI watchdog triggers and a very sporadic unthrottle bug (much like last time).
But stability is otherwise greatly improved.
Please all, have a good hard look at these patches.
Thanks to Alexander for really spending some time with them.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 01/12] perf: Close install vs exit race
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:03 ` [tip:perf/urgent] perf: Close install vs. " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 02/12] perf: Do not double free Peter Zijlstra
` (11 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-1.patch --]
[-- Type: text/plain, Size: 2848 bytes --]
The following scenario:
CPU0 CPU1
ctx = find_get_ctx();
perf_event_exit_task_context()
mutex_lock(&ctx->mutex);
perf_install_in_context(ctx, ...);
/* NO-OP */
mutex_unlock(&ctx->mutex);
...
perf_release()
WARN_ON_ONCE(event->state != STATE_EXIT);
Since the event doesn't pass through perf_remove_from_context()
because perf_install_in_context() NO-OPs because the ctx is dead, and
perf_event_exit_task_context() will not observe the event because its
not attached yet, the event->state will not be set.
Solve this by revalidating ctx->task after we acquire ctx->mutex and
failing the event creation as a whole.
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2158,13 +2158,15 @@ perf_install_in_context(struct perf_even
*/
raw_spin_lock_irq(&ctx->lock);
task = ctx->task;
+
/*
- * Worse, we cannot even rely on the ctx actually existing anymore. If
- * between find_get_context() and perf_install_in_context() the task
- * went through perf_event_exit_task() its dead and we should not be
- * adding new events.
+ * If between ctx = find_get_context() and mutex_lock(&ctx->mutex) the
+ * ctx gets destroyed, we must not install an event into it.
+ *
+ * This is normally tested for after we acquire the mutex, so this is
+ * a sanity check.
*/
- if (task == TASK_TOMBSTONE) {
+ if (WARN_ON_ONCE(task == TASK_TOMBSTONE)) {
raw_spin_unlock_irq(&ctx->lock);
return;
}
@@ -8389,10 +8391,19 @@ SYSCALL_DEFINE5(perf_event_open,
if (move_group) {
gctx = group_leader->ctx;
mutex_lock_double(&gctx->mutex, &ctx->mutex);
+ if (gctx->task == TASK_TOMBSTONE) {
+ err = -ESRCH;
+ goto err_locked;
+ }
} else {
mutex_lock(&ctx->mutex);
}
+ if (ctx->task == TASK_TOMBSTONE) {
+ err = -ESRCH;
+ goto err_locked;
+ }
+
if (!perf_event_validate_size(event)) {
err = -E2BIG;
goto err_locked;
@@ -8563,12 +8574,14 @@ perf_event_create_kernel_counter(struct
WARN_ON_ONCE(ctx->parent_ctx);
mutex_lock(&ctx->mutex);
+ if (ctx->task == TASK_TOMBSTONE) {
+ err = -ESRCH;
+ goto err_unlock;
+ }
+
if (!exclusive_event_installable(event, ctx)) {
- mutex_unlock(&ctx->mutex);
- perf_unpin_context(ctx);
- put_ctx(ctx);
err = -EBUSY;
- goto err_free;
+ goto err_unlock;
}
perf_install_in_context(ctx, event, cpu);
@@ -8577,6 +8590,10 @@ perf_event_create_kernel_counter(struct
return event;
+err_unlock:
+ mutex_unlock(&ctx->mutex);
+ perf_unpin_context(ctx);
+ put_ctx(ctx);
err_free:
free_event(event);
err:
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 02/12] perf: Do not double free
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-02-24 17:45 ` [PATCH 01/12] perf: Close install vs exit race Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:04 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 03/12] perf: Allow perf_release() with !event->ctx Peter Zijlstra
` (10 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-2.patch --]
[-- Type: text/plain, Size: 841 bytes --]
In case of: err_file: fput(event_file), we'll end up calling
perf_release() which in turn will free the event.
Do not then free the event _again_.
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8520,7 +8520,12 @@ SYSCALL_DEFINE5(perf_event_open,
perf_unpin_context(ctx);
put_ctx(ctx);
err_alloc:
- free_event(event);
+ /*
+ * If event_file is set, the fput() above will have called ->release()
+ * and that will take care of freeing the event.
+ */
+ if (!event_file)
+ free_event(event);
err_cpus:
put_online_cpus();
err_task:
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 03/12] perf: Allow perf_release() with !event->ctx
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-02-24 17:45 ` [PATCH 01/12] perf: Close install vs exit race Peter Zijlstra
2016-02-24 17:45 ` [PATCH 02/12] perf: Do not double free Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:04 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 04/12] perf: Only update context time when active Peter Zijlstra
` (9 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-3.patch --]
[-- Type: text/plain, Size: 1344 bytes --]
In the err_file: fput(event_file) case, the event will not yet have
been attached to a context. However perf_release() does assume it has
been. Cure this.
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3754,9 +3754,19 @@ static void put_event(struct perf_event
*/
int perf_event_release_kernel(struct perf_event *event)
{
- struct perf_event_context *ctx;
+ struct perf_event_context *ctx = event->ctx;
struct perf_event *child, *tmp;
+ /*
+ * If we got here through err_file: fput(event_file); we will not have
+ * attached to a context yet.
+ */
+ if (!ctx) {
+ WARN_ON_ONCE(event->attach_state &
+ (PERF_ATTACH_CONTEXT|PERF_ATTACH_GROUP));
+ goto no_ctx;
+ }
+
if (!is_kernel_event(event))
perf_remove_from_owner(event);
@@ -3832,8 +3842,8 @@ int perf_event_release_kernel(struct per
}
mutex_unlock(&event->child_mutex);
- /* Must be the last reference */
- put_event(event);
+no_ctx:
+ put_event(event); /* Must be the 'last' reference */
return 0;
}
EXPORT_SYMBOL_GPL(perf_event_release_kernel);
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 04/12] perf: Only update context time when active
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (2 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 03/12] perf: Allow perf_release() with !event->ctx Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:04 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 05/12] perf: Fix cloning Peter Zijlstra
` (8 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-4a.patch --]
[-- Type: text/plain, Size: 697 bytes --]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2170,12 +2170,12 @@ perf_install_in_context(struct perf_even
raw_spin_unlock_irq(&ctx->lock);
return;
}
- update_context_time(ctx);
- /*
- * Update cgrp time only if current cgrp matches event->cgrp.
- * Must be done before calling add_event_to_ctx().
- */
- update_cgrp_time_from_event(event);
+
+ if (ctx->is_active) {
+ update_context_time(ctx);
+ update_cgrp_time_from_event(event);
+ }
+
add_event_to_ctx(event, ctx);
raw_spin_unlock_irq(&ctx->lock);
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 05/12] perf: Fix cloning
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (3 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 04/12] perf: Only update context time when active Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:05 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 06/12] perf: Fix race between event install and jump_labels Peter Zijlstra
` (7 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-5.patch --]
[-- Type: text/plain, Size: 4857 bytes --]
Alexander reported that when the 'original' context gets destroyed, no
new clones happen.
This can happen irrespective of the ctx switch optimization, any task
can die, even the parent, and we want to continue monitoring the task
hierarchy until we either close the event or no tasks are left in the
hierarchy.
perf_event_init_context() will attempt to pin the 'parent' context
during clone(). At that point current is the parent, and since current
cannot have exited while executing clone(), its context cannot have
passed through perf_event_exit_task_context(). Therefore
perf_pin_task_context() cannot observe ctx->task == TASK_TOMBSTONE.
However, since inherit_event() does:
if (parent_event->parent)
parent_event = parent_event->parent;
it looks at the 'original' event when it does: is_orphaned_event().
This can return true if the context that contains the this event has
passed through perf_event_exit_task_context(). And thus we'll fail to
clone the perf context.
Fix this by adding a new state: STATE_DEAD, which is set by
perf_release() to indicate that the filedesc (or kernel reference) is
dead and there are no observers for our data left.
Only for STATE_DEAD will is_orphaned_event() be true and inhibit
cloning.
STATE_EXIT is otherwise preserved such that is_event_hup() remains
functional and will report when the observed task hierarchy becomes
empty.
Fixes: c6e5b73242d2 ("perf: Synchronously clean up child events")
Reported-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/perf_event.h | 1 +
kernel/events/core.c | 29 ++++++++++++++---------------
2 files changed, 15 insertions(+), 15 deletions(-)
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -397,6 +397,7 @@ struct pmu {
* enum perf_event_active_state - the states of a event
*/
enum perf_event_active_state {
+ PERF_EVENT_STATE_DEAD = -4,
PERF_EVENT_STATE_EXIT = -3,
PERF_EVENT_STATE_ERROR = -2,
PERF_EVENT_STATE_OFF = -1,
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1645,7 +1645,7 @@ static void perf_group_detach(struct per
static bool is_orphaned_event(struct perf_event *event)
{
- return event->state == PERF_EVENT_STATE_EXIT;
+ return event->state == PERF_EVENT_STATE_DEAD;
}
static inline int pmu_filter_match(struct perf_event *event)
@@ -1732,7 +1732,6 @@ group_sched_out(struct perf_event *group
}
#define DETACH_GROUP 0x01UL
-#define DETACH_STATE 0x02UL
/*
* Cross CPU call to remove a performance event
@@ -1752,8 +1751,6 @@ __perf_remove_from_context(struct perf_e
if (flags & DETACH_GROUP)
perf_group_detach(event);
list_del_event(event, ctx);
- if (flags & DETACH_STATE)
- event->state = PERF_EVENT_STATE_EXIT;
if (!ctx->nr_events && ctx->is_active) {
ctx->is_active = 0;
@@ -3776,22 +3773,24 @@ int perf_event_release_kernel(struct per
ctx = perf_event_ctx_lock(event);
WARN_ON_ONCE(ctx->parent_ctx);
- perf_remove_from_context(event, DETACH_GROUP | DETACH_STATE);
- perf_event_ctx_unlock(event, ctx);
+ perf_remove_from_context(event, DETACH_GROUP);
+ raw_spin_lock_irq(&ctx->lock);
/*
- * At this point we must have event->state == PERF_EVENT_STATE_EXIT,
- * either from the above perf_remove_from_context() or through
- * perf_event_exit_event().
+ * Mark this even as STATE_DEAD, there is no external reference to it
+ * anymore.
*
- * Therefore, anybody acquiring event->child_mutex after the below
- * loop _must_ also see this, most importantly inherit_event() which
- * will avoid placing more children on the list.
+ * Anybody acquiring event->child_mutex after the below loop _must_
+ * also see this, most importantly inherit_event() which will avoid
+ * placing more children on the list.
*
* Thus this guarantees that we will in fact observe and kill _ALL_
* child events.
*/
- WARN_ON_ONCE(event->state != PERF_EVENT_STATE_EXIT);
+ event->state = PERF_EVENT_STATE_DEAD;
+ raw_spin_unlock_irq(&ctx->lock);
+
+ perf_event_ctx_unlock(event, ctx);
again:
mutex_lock(&event->child_mutex);
@@ -4004,7 +4003,7 @@ static bool is_event_hup(struct perf_eve
{
bool no_children;
- if (event->state != PERF_EVENT_STATE_EXIT)
+ if (event->state > PERF_EVENT_STATE_EXIT)
return false;
mutex_lock(&event->child_mutex);
@@ -8731,7 +8730,7 @@ perf_event_exit_event(struct perf_event
if (parent_event)
perf_group_detach(child_event);
list_del_event(child_event, child_ctx);
- child_event->state = PERF_EVENT_STATE_EXIT; /* see perf_event_release_kernel() */
+ child_event->state = PERF_EVENT_STATE_EXIT; /* is_event_hup() */
raw_spin_unlock_irq(&child_ctx->lock);
/*
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 06/12] perf: Fix race between event install and jump_labels
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (4 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 05/12] perf: Fix cloning Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:05 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 07/12] perf: Cure event->pending_disable race Peter Zijlstra
` (6 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-6.patch --]
[-- Type: text/plain, Size: 4579 bytes --]
perf_install_in_context() relies upon the context switch hooks to have
scheduled in events when the IPI misses its target -- after all, if
the task has moved from the CPU (or wasn't running at all), it will
have to context switch to run elsewhere.
This however doesn't appear to be happening.
It is possible for the IPI to not happen (task wasn't running) only to
later observe the task running with an inactive context.
The only possible explanation is that the context switch hooks are not
called. Therefore put in a sync_sched() after toggling the jump_label
to guarantee all CPUs will have them enabled before we install an
event.
A simple if (0->1) sync_sched() will not in fact work, because any
further increment can race and complete before the sync_sched().
Therefore we must jump through some hoops.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/perf_event.h | 6 ++---
kernel/events/core.c | 49 +++++++++++++++++++++++++++++++++++++--------
2 files changed, 44 insertions(+), 11 deletions(-)
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -906,7 +906,7 @@ perf_sw_event_sched(u32 event_id, u64 nr
}
}
-extern struct static_key_deferred perf_sched_events;
+extern struct static_key_false perf_sched_events;
static __always_inline bool
perf_sw_migrate_enabled(void)
@@ -925,7 +925,7 @@ static inline void perf_event_task_migra
static inline void perf_event_task_sched_in(struct task_struct *prev,
struct task_struct *task)
{
- if (static_key_false(&perf_sched_events.key))
+ if (static_branch_unlikely(&perf_sched_events))
__perf_event_task_sched_in(prev, task);
if (perf_sw_migrate_enabled() && task->sched_migrated) {
@@ -942,7 +942,7 @@ static inline void perf_event_task_sched
{
perf_sw_event_sched(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 0);
- if (static_key_false(&perf_sched_events.key))
+ if (static_branch_unlikely(&perf_sched_events))
__perf_event_task_sched_out(prev, next);
}
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -321,7 +321,13 @@ enum event_type_t {
* perf_sched_events : >0 events exist
* perf_cgroup_events: >0 per-cpu cgroup events exist on this cpu
*/
-struct static_key_deferred perf_sched_events __read_mostly;
+
+static void perf_sched_delayed(struct work_struct *work);
+DEFINE_STATIC_KEY_FALSE(perf_sched_events);
+static DECLARE_DELAYED_WORK(perf_sched_work, perf_sched_delayed);
+static DEFINE_MUTEX(perf_sched_mutex);
+static atomic_t perf_sched_count;
+
static DEFINE_PER_CPU(atomic_t, perf_cgroup_events);
static DEFINE_PER_CPU(int, perf_sched_cb_usages);
@@ -3542,12 +3548,22 @@ static void unaccount_event(struct perf_
if (has_branch_stack(event))
dec = true;
- if (dec)
- static_key_slow_dec_deferred(&perf_sched_events);
+ if (dec) {
+ if (!atomic_add_unless(&perf_sched_count, -1, 1))
+ schedule_delayed_work(&perf_sched_work, HZ);
+ }
unaccount_event_cpu(event, event->cpu);
}
+static void perf_sched_delayed(struct work_struct *work)
+{
+ mutex_lock(&perf_sched_mutex);
+ if (atomic_dec_and_test(&perf_sched_count))
+ static_branch_disable(&perf_sched_events);
+ mutex_unlock(&perf_sched_mutex);
+}
+
/*
* The following implement mutual exclusion of events on "exclusive" pmus
* (PERF_PMU_CAP_EXCLUSIVE). Such pmus can only have one event scheduled
@@ -7786,8 +7802,28 @@ static void account_event(struct perf_ev
if (is_cgroup_event(event))
inc = true;
- if (inc)
- static_key_slow_inc(&perf_sched_events.key);
+ if (inc) {
+ if (atomic_inc_not_zero(&perf_sched_count))
+ goto enabled;
+
+ mutex_lock(&perf_sched_mutex);
+ if (!atomic_read(&perf_sched_count)) {
+ static_branch_enable(&perf_sched_events);
+ /*
+ * Guarantee that all CPUs observe they key change and
+ * call the perf scheduling hooks before proceeding to
+ * install events that need them.
+ */
+ synchronize_sched();
+ }
+ /*
+ * Now that we have waited for the sync_sched(), allow further
+ * increments to by-pass the mutex.
+ */
+ atomic_inc(&perf_sched_count);
+ mutex_unlock(&perf_sched_mutex);
+ }
+enabled:
account_event_cpu(event, event->cpu);
}
@@ -9352,9 +9388,6 @@ void __init perf_event_init(void)
ret = init_hw_breakpoint();
WARN(ret, "hw_breakpoint initialization failed with: %d", ret);
- /* do not patch jump label more than once per second */
- jump_label_rate_limit(&perf_sched_events, HZ);
-
/*
* Build time assertion that we keep the data_head at the intended
* location. IOW, validation we got the __reserved[] size right.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 07/12] perf: Cure event->pending_disable race
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (5 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 06/12] perf: Fix race between event install and jump_labels Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:06 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 08/12] perf: Introduce EVENT_TIME Peter Zijlstra
` (5 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-9.patch --]
[-- Type: text/plain, Size: 1466 bytes --]
Because event_sched_out() checks event->pending_disable _before_
actually disabling the event, it can happen that the event fires after
it checks but before it gets disabled.
This would leave event->pending_disable set and the queued irq_work
will try and process it.
However, if the event trigger was during schedule(), the event might
have been de-scheduled by the time the irq_work runs, and
perf_event_disable_local() will fail.
Fix this by checking event->pending_disable _after_ we call
event->pmu->del(). This depends on the latter being a compiler
barrier, such that the compiler does not lift the load and re-creates
the problem.
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1696,14 +1696,14 @@ event_sched_out(struct perf_event *event
perf_pmu_disable(event->pmu);
+ event->tstamp_stopped = tstamp;
+ event->pmu->del(event, 0);
+ event->oncpu = -1;
event->state = PERF_EVENT_STATE_INACTIVE;
if (event->pending_disable) {
event->pending_disable = 0;
event->state = PERF_EVENT_STATE_OFF;
}
- event->tstamp_stopped = tstamp;
- event->pmu->del(event, 0);
- event->oncpu = -1;
if (!is_software_event(event))
cpuctx->active_oncpu--;
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 08/12] perf: Introduce EVENT_TIME
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (6 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 07/12] perf: Cure event->pending_disable race Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:06 ` [tip:perf/urgent] perf: Fix ctx time tracking by introducing EVENT_TIME tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 09/12] perf: Fix scaling vs enable_on_exec Peter Zijlstra
` (4 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-10.patch --]
[-- Type: text/plain, Size: 4246 bytes --]
Currently any ctx_sched_in() call will re-start the ctx time tracking,
this means that calls like:
ctx_sched_in(.event_type = EVENT_PINNED);
ctx_sched_in(.event_type = EVENT_FLEXIBLE);
will have a hole in their ctx time tracking. This is likely harmless
but can confuse things a little. By adding EVENT_TIME, we can have the
first ctx_sched_in() (is_active: 0 -> !0) start the time and any
further ctx_sched_in() will leave the timestamps alone.
Secondly, this allows for an early disable like:
ctx_sched_out(.event_type = EVENT_TIME);
which would update the ctx time (if the ctx is active) and any further
calls to ctx_sched_out() would not further modify the ctx time.
For ctx_sched_in() any 0 -> !0 transition will automatically include
EVENT_TIME.
For ctx_sched_out(), any transition that clears EVENT_ALL will
automatically clear EVENT_TIME.
These two rules ensure that under normal circumstances we need not
bother with EVENT_TIME and get natural ctx time behaviour.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -314,6 +314,7 @@ static void event_function_call(struct p
enum event_type_t {
EVENT_FLEXIBLE = 0x1,
EVENT_PINNED = 0x2,
+ EVENT_TIME = 0x4,
EVENT_ALL = EVENT_FLEXIBLE | EVENT_PINNED,
};
@@ -1294,16 +1295,18 @@ static u64 perf_event_time(struct perf_e
/*
* Update the total_time_enabled and total_time_running fields for a event.
- * The caller of this function needs to hold the ctx->lock.
*/
static void update_event_times(struct perf_event *event)
{
struct perf_event_context *ctx = event->ctx;
u64 run_end;
+ lockdep_assert_held(&ctx->lock);
+
if (event->state < PERF_EVENT_STATE_INACTIVE ||
event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
return;
+
/*
* in cgroup mode, time_enabled represents
* the time the event was enabled AND active
@@ -2349,24 +2352,33 @@ static void ctx_sched_out(struct perf_ev
}
ctx->is_active &= ~event_type;
+ if (!(ctx->is_active & EVENT_ALL))
+ ctx->is_active = 0;
+
if (ctx->task) {
WARN_ON_ONCE(cpuctx->task_ctx != ctx);
if (!ctx->is_active)
cpuctx->task_ctx = NULL;
}
- update_context_time(ctx);
- update_cgrp_time_from_cpuctx(cpuctx);
- if (!ctx->nr_active)
+ is_active ^= ctx->is_active; /* changed bits */
+
+ if (is_active & EVENT_TIME) {
+ /* update (and stop) ctx time */
+ update_context_time(ctx);
+ update_cgrp_time_from_cpuctx(cpuctx);
+ }
+
+ if (!ctx->nr_active || !(is_active & EVENT_ALL))
return;
perf_pmu_disable(ctx->pmu);
- if ((is_active & EVENT_PINNED) && (event_type & EVENT_PINNED)) {
+ if (is_active & EVENT_PINNED) {
list_for_each_entry(event, &ctx->pinned_groups, group_entry)
group_sched_out(event, cpuctx, ctx);
}
- if ((is_active & EVENT_FLEXIBLE) && (event_type & EVENT_FLEXIBLE)) {
+ if (is_active & EVENT_FLEXIBLE) {
list_for_each_entry(event, &ctx->flexible_groups, group_entry)
group_sched_out(event, cpuctx, ctx);
}
@@ -2740,7 +2752,7 @@ ctx_sched_in(struct perf_event_context *
if (likely(!ctx->nr_events))
return;
- ctx->is_active |= event_type;
+ ctx->is_active |= (event_type | EVENT_TIME);
if (ctx->task) {
if (!is_active)
cpuctx->task_ctx = ctx;
@@ -2748,18 +2760,24 @@ ctx_sched_in(struct perf_event_context *
WARN_ON_ONCE(cpuctx->task_ctx != ctx);
}
- now = perf_clock();
- ctx->timestamp = now;
- perf_cgroup_set_timestamp(task, ctx);
+ is_active ^= ctx->is_active; /* changed bits */
+
+ if (is_active & EVENT_TIME) {
+ /* start ctx time */
+ now = perf_clock();
+ ctx->timestamp = now;
+ perf_cgroup_set_timestamp(task, ctx);
+ }
+
/*
* First go through the list and put on any pinned groups
* in order to give them the best chance of going on.
*/
- if (!(is_active & EVENT_PINNED) && (event_type & EVENT_PINNED))
+ if (is_active & EVENT_PINNED)
ctx_pinned_sched_in(ctx, cpuctx);
/* Then walk through the lower prio flexible groups */
- if (!(is_active & EVENT_FLEXIBLE) && (event_type & EVENT_FLEXIBLE))
+ if (is_active & EVENT_FLEXIBLE)
ctx_flexible_sched_in(ctx, cpuctx);
}
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 09/12] perf: Fix scaling vs enable_on_exec
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (7 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 08/12] perf: Introduce EVENT_TIME Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:07 ` [tip:perf/urgent] perf: Fix scaling vs. perf_event_enable_on_exec() tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 10/12] perf: Fix scaling vs perf_event_enable Peter Zijlstra
` (3 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-11.patch --]
[-- Type: text/plain, Size: 1009 bytes --]
The recent commit 3e349507d12d ("perf: Fix perf_enable_on_exec() event
scheduling") caused this by moving task_ctx_sched_out() from before
__perf_event_mask_enable() to after it.
The overlooked concequence of that change is that task_ctx_sched_out()
would update the ctx time fields, and now __perf_event_mask_enable()
uses stale time.
In order to fix this, explicitly stop our context's time before
enabling the event(s).
Fixes: 3e349507d12d ("perf: Fix perf_enable_on_exec() event scheduling")
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 1 +
1 file changed, 1 insertion(+)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3143,6 +3143,7 @@ static void perf_event_enable_on_exec(in
cpuctx = __get_cpu_context(ctx);
perf_ctx_lock(cpuctx, ctx);
+ ctx_sched_out(ctx, cpuctx, EVENT_TIME);
list_for_each_entry(event, &ctx->event_list, event_entry)
enabled |= event_enable_on_exec(event, ctx);
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 10/12] perf: Fix scaling vs perf_event_enable
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (8 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 09/12] perf: Fix scaling vs enable_on_exec Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:07 ` [tip:perf/urgent] perf: Fix scaling vs. perf_event_enable() tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 11/12] perf: Fix scaling vs perf_install_in_context Peter Zijlstra
` (2 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-11a.patch --]
[-- Type: text/plain, Size: 2646 bytes --]
Similar to the perf_enable_on_exec(), ensure that event timings are
consistent across perf_event_enable().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 42 +++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 19 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2069,14 +2069,27 @@ static void add_event_to_ctx(struct perf
event->tstamp_stopped = tstamp;
}
-static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
- struct perf_event_context *ctx);
+static void ctx_sched_out(struct perf_event_context *ctx,
+ struct perf_cpu_context *cpuctx,
+ enum event_type_t event_type);
static void
ctx_sched_in(struct perf_event_context *ctx,
struct perf_cpu_context *cpuctx,
enum event_type_t event_type,
struct task_struct *task);
+static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
+ struct perf_event_context *ctx)
+{
+ if (!cpuctx->task_ctx)
+ return;
+
+ if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
+ return;
+
+ ctx_sched_out(ctx, cpuctx, EVENT_ALL);
+}
+
static void perf_event_sched_in(struct perf_cpu_context *cpuctx,
struct perf_event_context *ctx,
struct task_struct *task)
@@ -2227,17 +2240,18 @@ static void __perf_event_enable(struct p
event->state <= PERF_EVENT_STATE_ERROR)
return;
- update_context_time(ctx);
+ if (ctx->is_active)
+ ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+
__perf_event_mark_enabled(event);
if (!ctx->is_active)
return;
if (!event_filter_match(event)) {
- if (is_cgroup_event(event)) {
- perf_cgroup_set_timestamp(current, ctx); // XXX ?
+ if (is_cgroup_event(event))
perf_cgroup_defer_enabled(event);
- }
+ ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
return;
}
@@ -2245,8 +2259,10 @@ static void __perf_event_enable(struct p
* If the event is in a group and isn't the group leader,
* then don't put it on unless the group is on.
*/
- if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
+ if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE) {
+ ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
return;
+ }
task_ctx = cpuctx->task_ctx;
if (ctx->task)
@@ -2658,18 +2674,6 @@ void __perf_event_task_sched_out(struct
perf_cgroup_sched_out(task, next);
}
-static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
- struct perf_event_context *ctx)
-{
- if (!cpuctx->task_ctx)
- return;
-
- if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
- return;
-
- ctx_sched_out(ctx, cpuctx, EVENT_ALL);
-}
-
/*
* Called with IRQs disabled
*/
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 11/12] perf: Fix scaling vs perf_install_in_context
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (9 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 10/12] perf: Fix scaling vs perf_event_enable Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:07 ` [tip:perf/urgent] perf: Fix scaling vs. perf_install_in_context() tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 12/12] perf: Robustify task_function_call() Peter Zijlstra
2016-03-10 14:39 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-12.patch --]
[-- Type: text/plain, Size: 5451 bytes --]
Completely reworks perf_install_in_context() (again!) in order to
ensure that there will be no ctx time hole between add_event_to_ctx()
and any potential ctx_sched_in().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 115 +++++++++++++++++++++++++++++++--------------------
1 file changed, 70 insertions(+), 45 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -276,10 +276,10 @@ static void event_function_call(struct p
return;
}
-again:
if (task == TASK_TOMBSTONE)
return;
+again:
if (!task_function_call(task, event_function, &efs))
return;
@@ -289,13 +289,15 @@ static void event_function_call(struct p
* a concurrent perf_event_context_sched_out().
*/
task = ctx->task;
- if (task != TASK_TOMBSTONE) {
- if (ctx->is_active) {
- raw_spin_unlock_irq(&ctx->lock);
- goto again;
- }
- func(event, NULL, ctx, data);
+ if (task == TASK_TOMBSTONE) {
+ raw_spin_unlock_irq(&ctx->lock);
+ return;
}
+ if (ctx->is_active) {
+ raw_spin_unlock_irq(&ctx->lock);
+ goto again;
+ }
+ func(event, NULL, ctx, data);
raw_spin_unlock_irq(&ctx->lock);
}
@@ -2116,49 +2118,68 @@ static void ctx_resched(struct perf_cpu_
/*
* Cross CPU call to install and enable a performance event
*
- * Must be called with ctx->mutex held
+ * Very similar to remote_function() + event_function() but cannot assume that
+ * things like ctx->is_active and cpuctx->task_ctx are set.
*/
static int __perf_install_in_context(void *info)
{
- struct perf_event_context *ctx = info;
+ struct perf_event *event = info;
+ struct perf_event_context *ctx = event->ctx;
struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
struct perf_event_context *task_ctx = cpuctx->task_ctx;
+ bool activate = true;
+ int ret = 0;
raw_spin_lock(&cpuctx->ctx.lock);
if (ctx->task) {
raw_spin_lock(&ctx->lock);
- /*
- * If we hit the 'wrong' task, we've since scheduled and
- * everything should be sorted, nothing to do!
- */
task_ctx = ctx;
- if (ctx->task != current)
+
+ /* If we're on the wrong CPU, try again */
+ if (task_cpu(ctx->task) != smp_processor_id()) {
+ ret = -ESRCH;
goto unlock;
+ }
/*
- * If task_ctx is set, it had better be to us.
+ * If we're on the right CPU, see if the task we target is
+ * current, if not we don't have to activate the ctx, a future
+ * context switch will do that for us.
*/
- WARN_ON_ONCE(cpuctx->task_ctx != ctx && cpuctx->task_ctx);
+ if (ctx->task != current)
+ activate = false;
+ else
+ WARN_ON_ONCE(cpuctx->task_ctx && cpuctx->task_ctx != ctx);
+
} else if (task_ctx) {
raw_spin_lock(&task_ctx->lock);
}
- ctx_resched(cpuctx, task_ctx);
+ if (activate) {
+ ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+ add_event_to_ctx(event, ctx);
+ ctx_resched(cpuctx, task_ctx);
+ } else {
+ add_event_to_ctx(event, ctx);
+ }
+
unlock:
perf_ctx_unlock(cpuctx, task_ctx);
- return 0;
+ return ret;
}
/*
- * Attach a performance event to a context
+ * Attach a performance event to a context.
+ *
+ * Very similar to event_function_call, see comment there.
*/
static void
perf_install_in_context(struct perf_event_context *ctx,
struct perf_event *event,
int cpu)
{
- struct task_struct *task = NULL;
+ struct task_struct *task = READ_ONCE(ctx->task);
lockdep_assert_held(&ctx->mutex);
@@ -2166,42 +2187,46 @@ perf_install_in_context(struct perf_even
if (event->cpu != -1)
event->cpu = cpu;
+ if (!task) {
+ cpu_function_call(cpu, __perf_install_in_context, event);
+ return;
+ }
+
+ /*
+ * Should not happen, we validate the ctx is still alive before calling.
+ */
+ if (WARN_ON_ONCE(task == TASK_TOMBSTONE))
+ return;
+
/*
* Installing events is tricky because we cannot rely on ctx->is_active
* to be set in case this is the nr_events 0 -> 1 transition.
- *
- * So what we do is we add the event to the list here, which will allow
- * a future context switch to DTRT and then send a racy IPI. If the IPI
- * fails to hit the right task, this means a context switch must have
- * happened and that will have taken care of business.
*/
- raw_spin_lock_irq(&ctx->lock);
- task = ctx->task;
-
+again:
/*
- * If between ctx = find_get_context() and mutex_lock(&ctx->mutex) the
- * ctx gets destroyed, we must not install an event into it.
- *
- * This is normally tested for after we acquire the mutex, so this is
- * a sanity check.
+ * Cannot use task_function_call() because we need to run on the task's
+ * CPU regardless of whether its current or not.
*/
+ if (!cpu_function_call(task_cpu(task), __perf_install_in_context, event))
+ return;
+
+ raw_spin_lock_irq(&ctx->lock);
+ task = ctx->task;
if (WARN_ON_ONCE(task == TASK_TOMBSTONE)) {
+ /*
+ * Cannot happen because we already checked above (which also
+ * cannot happen), and we hold ctx->mutex, which serializes us
+ * against perf_event_exit_task_context().
+ */
raw_spin_unlock_irq(&ctx->lock);
return;
}
-
- if (ctx->is_active) {
- update_context_time(ctx);
- update_cgrp_time_from_event(event);
- }
-
- add_event_to_ctx(event, ctx);
raw_spin_unlock_irq(&ctx->lock);
-
- if (task)
- task_function_call(task, __perf_install_in_context, ctx);
- else
- cpu_function_call(cpu, __perf_install_in_context, ctx);
+ /*
+ * Since !ctx->is_active doesn't mean anything, we must IPI
+ * unconditionally.
+ */
+ goto again;
}
/*
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 12/12] perf: Robustify task_function_call()
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (10 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 11/12] perf: Fix scaling vs perf_install_in_context Peter Zijlstra
@ 2016-02-24 17:45 ` Peter Zijlstra
2016-02-25 8:08 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-03-10 14:39 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
12 siblings, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-02-24 17:45 UTC (permalink / raw)
To: peterz, mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin, oleg
[-- Attachment #1: peterz-perf-more-fixes-7.patch --]
[-- Type: text/plain, Size: 2560 bytes --]
Since there is no serialization between task_function_call() doing
task_curr() and the other CPU doing context switches, we could end
up not sending an IPI even if we had to.
And I'm not sure I still buy my own argument we're OK.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -64,8 +64,17 @@ static void remote_function(void *data)
struct task_struct *p = tfc->p;
if (p) {
- tfc->ret = -EAGAIN;
- if (task_cpu(p) != smp_processor_id() || !task_curr(p))
+ /* -EAGAIN */
+ if (task_cpu(p) != smp_processor_id())
+ return;
+
+ /*
+ * Now that we're on right CPU with IRQs disabled, we can test
+ * if we hit the right task without races.
+ */
+
+ tfc->ret = -ESRCH; /* No such (running) process */
+ if (p != current)
return;
}
@@ -92,13 +101,17 @@ task_function_call(struct task_struct *p
.p = p,
.func = func,
.info = info,
- .ret = -ESRCH, /* No such (running) process */
+ .ret = -EAGAIN,
};
+ int ret;
- if (task_curr(p))
- smp_call_function_single(task_cpu(p), remote_function, &data, 1);
+ do {
+ ret = smp_call_function_single(task_cpu(p), remote_function, &data, 1);
+ if (!ret)
+ ret = data.ret;
+ } while (ret == -EAGAIN);
- return data.ret;
+ return ret;
}
/**
@@ -169,19 +182,6 @@ static bool is_kernel_event(struct perf_
* rely on ctx->is_active and therefore cannot use event_function_call().
* See perf_install_in_context().
*
- * This is because we need a ctx->lock serialized variable (ctx->is_active)
- * to reliably determine if a particular task/context is scheduled in. The
- * task_curr() use in task_function_call() is racy in that a remote context
- * switch is not a single atomic operation.
- *
- * As is, the situation is 'safe' because we set rq->curr before we do the
- * actual context switch. This means that task_curr() will fail early, but
- * we'll continue spinning on ctx->is_active until we've passed
- * perf_event_task_sched_out().
- *
- * Without this ctx->lock serialized variable we could have race where we find
- * the task (and hence the context) would not be active while in fact they are.
- *
* If ctx->nr_events, then ctx->is_active and cpuctx->task_ctx are set.
*/
@@ -212,7 +212,7 @@ static int event_function(void *info)
*/
if (ctx->task) {
if (ctx->task != current) {
- ret = -EAGAIN;
+ ret = -ESRCH;
goto unlock;
}
^ permalink raw reply [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Close install vs. exit race
2016-02-24 17:45 ` [PATCH 01/12] perf: Close install vs exit race Peter Zijlstra
@ 2016-02-25 8:03 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:03 UTC (permalink / raw)
To: linux-tip-commits
Cc: mingo, peterz, tglx, hpa, acme, torvalds, alexander.shishkin,
linux-kernel, jolsa
Commit-ID: 84c4e620d35f49f486a900af214ad12276afb386
Gitweb: http://git.kernel.org/tip/84c4e620d35f49f486a900af214ad12276afb386
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:40 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:32 +0100
perf: Close install vs. exit race
Consider the following scenario:
CPU0 CPU1
ctx = find_get_ctx();
perf_event_exit_task_context()
mutex_lock(&ctx->mutex);
perf_install_in_context(ctx, ...);
/* NO-OP */
mutex_unlock(&ctx->mutex);
...
perf_release()
WARN_ON_ONCE(event->state != STATE_EXIT);
Since the event doesn't pass through perf_remove_from_context()
because perf_install_in_context() NO-OPs because the ctx is dead, and
perf_event_exit_task_context() will not observe the event because its
not attached yet, the event->state will not be set.
Solve this by revalidating ctx->task after we acquire ctx->mutex and
failing the event creation as a whole.
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174947.626853419@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0d58522..d7b0316 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2158,13 +2158,15 @@ perf_install_in_context(struct perf_event_context *ctx,
*/
raw_spin_lock_irq(&ctx->lock);
task = ctx->task;
+
/*
- * Worse, we cannot even rely on the ctx actually existing anymore. If
- * between find_get_context() and perf_install_in_context() the task
- * went through perf_event_exit_task() its dead and we should not be
- * adding new events.
+ * If between ctx = find_get_context() and mutex_lock(&ctx->mutex) the
+ * ctx gets destroyed, we must not install an event into it.
+ *
+ * This is normally tested for after we acquire the mutex, so this is
+ * a sanity check.
*/
- if (task == TASK_TOMBSTONE) {
+ if (WARN_ON_ONCE(task == TASK_TOMBSTONE)) {
raw_spin_unlock_irq(&ctx->lock);
return;
}
@@ -8389,10 +8391,19 @@ SYSCALL_DEFINE5(perf_event_open,
if (move_group) {
gctx = group_leader->ctx;
mutex_lock_double(&gctx->mutex, &ctx->mutex);
+ if (gctx->task == TASK_TOMBSTONE) {
+ err = -ESRCH;
+ goto err_locked;
+ }
} else {
mutex_lock(&ctx->mutex);
}
+ if (ctx->task == TASK_TOMBSTONE) {
+ err = -ESRCH;
+ goto err_locked;
+ }
+
if (!perf_event_validate_size(event)) {
err = -E2BIG;
goto err_locked;
@@ -8563,12 +8574,14 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
WARN_ON_ONCE(ctx->parent_ctx);
mutex_lock(&ctx->mutex);
+ if (ctx->task == TASK_TOMBSTONE) {
+ err = -ESRCH;
+ goto err_unlock;
+ }
+
if (!exclusive_event_installable(event, ctx)) {
- mutex_unlock(&ctx->mutex);
- perf_unpin_context(ctx);
- put_ctx(ctx);
err = -EBUSY;
- goto err_free;
+ goto err_unlock;
}
perf_install_in_context(ctx, event, cpu);
@@ -8577,6 +8590,10 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
return event;
+err_unlock:
+ mutex_unlock(&ctx->mutex);
+ perf_unpin_context(ctx);
+ put_ctx(ctx);
err_free:
free_event(event);
err:
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Do not double free
2016-02-24 17:45 ` [PATCH 02/12] perf: Do not double free Peter Zijlstra
@ 2016-02-25 8:04 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:04 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, tglx, alexander.shishkin, torvalds, hpa, jolsa,
linux-kernel, peterz, mingo
Commit-ID: 130056275ade730e7a79c110212c8815202773ee
Gitweb: http://git.kernel.org/tip/130056275ade730e7a79c110212c8815202773ee
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:41 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:32 +0100
perf: Do not double free
In case of: err_file: fput(event_file), we'll end up calling
perf_release() which in turn will free the event.
Do not then free the event _again_.
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174947.697350349@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d7b0316..211a5ce 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8520,7 +8520,12 @@ err_context:
perf_unpin_context(ctx);
put_ctx(ctx);
err_alloc:
- free_event(event);
+ /*
+ * If event_file is set, the fput() above will have called ->release()
+ * and that will take care of freeing the event.
+ */
+ if (!event_file)
+ free_event(event);
err_cpus:
put_online_cpus();
err_task:
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Allow perf_release() with !event->ctx
2016-02-24 17:45 ` [PATCH 03/12] perf: Allow perf_release() with !event->ctx Peter Zijlstra
@ 2016-02-25 8:04 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:04 UTC (permalink / raw)
To: linux-tip-commits
Cc: jolsa, linux-kernel, acme, torvalds, mingo, peterz, hpa, tglx,
alexander.shishkin
Commit-ID: a4f4bb6d0c69d0bb573f1d9e6f1b806f9b038b19
Gitweb: http://git.kernel.org/tip/a4f4bb6d0c69d0bb573f1d9e6f1b806f9b038b19
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:42 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:33 +0100
perf: Allow perf_release() with !event->ctx
In the err_file: fput(event_file) case, the event will not yet have
been attached to a context. However perf_release() does assume it has
been. Cure this.
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174947.793996260@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 211a5ce..d5299e2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3754,9 +3754,19 @@ static void put_event(struct perf_event *event)
*/
int perf_event_release_kernel(struct perf_event *event)
{
- struct perf_event_context *ctx;
+ struct perf_event_context *ctx = event->ctx;
struct perf_event *child, *tmp;
+ /*
+ * If we got here through err_file: fput(event_file); we will not have
+ * attached to a context yet.
+ */
+ if (!ctx) {
+ WARN_ON_ONCE(event->attach_state &
+ (PERF_ATTACH_CONTEXT|PERF_ATTACH_GROUP));
+ goto no_ctx;
+ }
+
if (!is_kernel_event(event))
perf_remove_from_owner(event);
@@ -3832,8 +3842,8 @@ again:
}
mutex_unlock(&event->child_mutex);
- /* Must be the last reference */
- put_event(event);
+no_ctx:
+ put_event(event); /* Must be the 'last' reference */
return 0;
}
EXPORT_SYMBOL_GPL(perf_event_release_kernel);
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Only update context time when active
2016-02-24 17:45 ` [PATCH 04/12] perf: Only update context time when active Peter Zijlstra
@ 2016-02-25 8:04 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:04 UTC (permalink / raw)
To: linux-tip-commits
Cc: torvalds, peterz, tglx, mingo, alexander.shishkin, jolsa, acme,
hpa, linux-kernel
Commit-ID: 6f932e5be1503ab0783699e843db325d44c2fabb
Gitweb: http://git.kernel.org/tip/6f932e5be1503ab0783699e843db325d44c2fabb
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:43 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:33 +0100
perf: Only update context time when active
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174947.860690919@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d5299e2..64698fb 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2170,12 +2170,12 @@ perf_install_in_context(struct perf_event_context *ctx,
raw_spin_unlock_irq(&ctx->lock);
return;
}
- update_context_time(ctx);
- /*
- * Update cgrp time only if current cgrp matches event->cgrp.
- * Must be done before calling add_event_to_ctx().
- */
- update_cgrp_time_from_event(event);
+
+ if (ctx->is_active) {
+ update_context_time(ctx);
+ update_cgrp_time_from_event(event);
+ }
+
add_event_to_ctx(event, ctx);
raw_spin_unlock_irq(&ctx->lock);
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Fix cloning
2016-02-24 17:45 ` [PATCH 05/12] perf: Fix cloning Peter Zijlstra
@ 2016-02-25 8:05 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:05 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, peterz, hpa, jolsa, tglx, mingo, acme,
alexander.shishkin, torvalds
Commit-ID: a69b0ca4ac3bf5427b571f11cbf33f0a32b728d5
Gitweb: http://git.kernel.org/tip/a69b0ca4ac3bf5427b571f11cbf33f0a32b728d5
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:44 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:33 +0100
perf: Fix cloning
Alexander reported that when the 'original' context gets destroyed, no
new clones happen.
This can happen irrespective of the ctx switch optimization, any task
can die, even the parent, and we want to continue monitoring the task
hierarchy until we either close the event or no tasks are left in the
hierarchy.
perf_event_init_context() will attempt to pin the 'parent' context
during clone(). At that point current is the parent, and since current
cannot have exited while executing clone(), its context cannot have
passed through perf_event_exit_task_context(). Therefore
perf_pin_task_context() cannot observe ctx->task == TASK_TOMBSTONE.
However, since inherit_event() does:
if (parent_event->parent)
parent_event = parent_event->parent;
it looks at the 'original' event when it does: is_orphaned_event().
This can return true if the context that contains the this event has
passed through perf_event_exit_task_context(). And thus we'll fail to
clone the perf context.
Fix this by adding a new state: STATE_DEAD, which is set by
perf_release() to indicate that the filedesc (or kernel reference) is
dead and there are no observers for our data left.
Only for STATE_DEAD will is_orphaned_event() be true and inhibit
cloning.
STATE_EXIT is otherwise preserved such that is_event_hup() remains
functional and will report when the observed task hierarchy becomes
empty.
Reported-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Fixes: c6e5b73242d2 ("perf: Synchronously clean up child events")
Link: http://lkml.kernel.org/r/20160224174947.919845295@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
include/linux/perf_event.h | 1 +
kernel/events/core.c | 29 ++++++++++++++---------------
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index b35a61a..3915661 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -397,6 +397,7 @@ struct pmu {
* enum perf_event_active_state - the states of a event
*/
enum perf_event_active_state {
+ PERF_EVENT_STATE_DEAD = -4,
PERF_EVENT_STATE_EXIT = -3,
PERF_EVENT_STATE_ERROR = -2,
PERF_EVENT_STATE_OFF = -1,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 64698fb..92d6999 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1645,7 +1645,7 @@ out:
static bool is_orphaned_event(struct perf_event *event)
{
- return event->state == PERF_EVENT_STATE_EXIT;
+ return event->state == PERF_EVENT_STATE_DEAD;
}
static inline int pmu_filter_match(struct perf_event *event)
@@ -1732,7 +1732,6 @@ group_sched_out(struct perf_event *group_event,
}
#define DETACH_GROUP 0x01UL
-#define DETACH_STATE 0x02UL
/*
* Cross CPU call to remove a performance event
@@ -1752,8 +1751,6 @@ __perf_remove_from_context(struct perf_event *event,
if (flags & DETACH_GROUP)
perf_group_detach(event);
list_del_event(event, ctx);
- if (flags & DETACH_STATE)
- event->state = PERF_EVENT_STATE_EXIT;
if (!ctx->nr_events && ctx->is_active) {
ctx->is_active = 0;
@@ -3772,22 +3769,24 @@ int perf_event_release_kernel(struct perf_event *event)
ctx = perf_event_ctx_lock(event);
WARN_ON_ONCE(ctx->parent_ctx);
- perf_remove_from_context(event, DETACH_GROUP | DETACH_STATE);
- perf_event_ctx_unlock(event, ctx);
+ perf_remove_from_context(event, DETACH_GROUP);
+ raw_spin_lock_irq(&ctx->lock);
/*
- * At this point we must have event->state == PERF_EVENT_STATE_EXIT,
- * either from the above perf_remove_from_context() or through
- * perf_event_exit_event().
+ * Mark this even as STATE_DEAD, there is no external reference to it
+ * anymore.
*
- * Therefore, anybody acquiring event->child_mutex after the below
- * loop _must_ also see this, most importantly inherit_event() which
- * will avoid placing more children on the list.
+ * Anybody acquiring event->child_mutex after the below loop _must_
+ * also see this, most importantly inherit_event() which will avoid
+ * placing more children on the list.
*
* Thus this guarantees that we will in fact observe and kill _ALL_
* child events.
*/
- WARN_ON_ONCE(event->state != PERF_EVENT_STATE_EXIT);
+ event->state = PERF_EVENT_STATE_DEAD;
+ raw_spin_unlock_irq(&ctx->lock);
+
+ perf_event_ctx_unlock(event, ctx);
again:
mutex_lock(&event->child_mutex);
@@ -4000,7 +3999,7 @@ static bool is_event_hup(struct perf_event *event)
{
bool no_children;
- if (event->state != PERF_EVENT_STATE_EXIT)
+ if (event->state > PERF_EVENT_STATE_EXIT)
return false;
mutex_lock(&event->child_mutex);
@@ -8727,7 +8726,7 @@ perf_event_exit_event(struct perf_event *child_event,
if (parent_event)
perf_group_detach(child_event);
list_del_event(child_event, child_ctx);
- child_event->state = PERF_EVENT_STATE_EXIT; /* see perf_event_release_kernel() */
+ child_event->state = PERF_EVENT_STATE_EXIT; /* is_event_hup() */
raw_spin_unlock_irq(&child_ctx->lock);
/*
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Fix race between event install and jump_labels
2016-02-24 17:45 ` [PATCH 06/12] perf: Fix race between event install and jump_labels Peter Zijlstra
@ 2016-02-25 8:05 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:05 UTC (permalink / raw)
To: linux-tip-commits
Cc: torvalds, tglx, acme, alexander.shishkin, jolsa, mingo, peterz,
hpa, linux-kernel
Commit-ID: 9107c89e269d2738019861bb518e3d59bef01781
Gitweb: http://git.kernel.org/tip/9107c89e269d2738019861bb518e3d59bef01781
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:45 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:34 +0100
perf: Fix race between event install and jump_labels
perf_install_in_context() relies upon the context switch hooks to have
scheduled in events when the IPI misses its target -- after all, if
the task has moved from the CPU (or wasn't running at all), it will
have to context switch to run elsewhere.
This however doesn't appear to be happening.
It is possible for the IPI to not happen (task wasn't running) only to
later observe the task running with an inactive context.
The only possible explanation is that the context switch hooks are not
called. Therefore put in a sync_sched() after toggling the jump_label
to guarantee all CPUs will have them enabled before we install an
event.
A simple if (0->1) sync_sched() will not in fact work, because any
further increment can race and complete before the sync_sched().
Therefore we must jump through some hoops.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174947.980211985@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
include/linux/perf_event.h | 6 +++---
kernel/events/core.c | 49 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3915661..f5c5a3f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -906,7 +906,7 @@ perf_sw_event_sched(u32 event_id, u64 nr, u64 addr)
}
}
-extern struct static_key_deferred perf_sched_events;
+extern struct static_key_false perf_sched_events;
static __always_inline bool
perf_sw_migrate_enabled(void)
@@ -925,7 +925,7 @@ static inline void perf_event_task_migrate(struct task_struct *task)
static inline void perf_event_task_sched_in(struct task_struct *prev,
struct task_struct *task)
{
- if (static_key_false(&perf_sched_events.key))
+ if (static_branch_unlikely(&perf_sched_events))
__perf_event_task_sched_in(prev, task);
if (perf_sw_migrate_enabled() && task->sched_migrated) {
@@ -942,7 +942,7 @@ static inline void perf_event_task_sched_out(struct task_struct *prev,
{
perf_sw_event_sched(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 0);
- if (static_key_false(&perf_sched_events.key))
+ if (static_branch_unlikely(&perf_sched_events))
__perf_event_task_sched_out(prev, next);
}
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 92d6999..ea064ca 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -321,7 +321,13 @@ enum event_type_t {
* perf_sched_events : >0 events exist
* perf_cgroup_events: >0 per-cpu cgroup events exist on this cpu
*/
-struct static_key_deferred perf_sched_events __read_mostly;
+
+static void perf_sched_delayed(struct work_struct *work);
+DEFINE_STATIC_KEY_FALSE(perf_sched_events);
+static DECLARE_DELAYED_WORK(perf_sched_work, perf_sched_delayed);
+static DEFINE_MUTEX(perf_sched_mutex);
+static atomic_t perf_sched_count;
+
static DEFINE_PER_CPU(atomic_t, perf_cgroup_events);
static DEFINE_PER_CPU(int, perf_sched_cb_usages);
@@ -3536,12 +3542,22 @@ static void unaccount_event(struct perf_event *event)
if (has_branch_stack(event))
dec = true;
- if (dec)
- static_key_slow_dec_deferred(&perf_sched_events);
+ if (dec) {
+ if (!atomic_add_unless(&perf_sched_count, -1, 1))
+ schedule_delayed_work(&perf_sched_work, HZ);
+ }
unaccount_event_cpu(event, event->cpu);
}
+static void perf_sched_delayed(struct work_struct *work)
+{
+ mutex_lock(&perf_sched_mutex);
+ if (atomic_dec_and_test(&perf_sched_count))
+ static_branch_disable(&perf_sched_events);
+ mutex_unlock(&perf_sched_mutex);
+}
+
/*
* The following implement mutual exclusion of events on "exclusive" pmus
* (PERF_PMU_CAP_EXCLUSIVE). Such pmus can only have one event scheduled
@@ -7780,8 +7796,28 @@ static void account_event(struct perf_event *event)
if (is_cgroup_event(event))
inc = true;
- if (inc)
- static_key_slow_inc(&perf_sched_events.key);
+ if (inc) {
+ if (atomic_inc_not_zero(&perf_sched_count))
+ goto enabled;
+
+ mutex_lock(&perf_sched_mutex);
+ if (!atomic_read(&perf_sched_count)) {
+ static_branch_enable(&perf_sched_events);
+ /*
+ * Guarantee that all CPUs observe they key change and
+ * call the perf scheduling hooks before proceeding to
+ * install events that need them.
+ */
+ synchronize_sched();
+ }
+ /*
+ * Now that we have waited for the sync_sched(), allow further
+ * increments to by-pass the mutex.
+ */
+ atomic_inc(&perf_sched_count);
+ mutex_unlock(&perf_sched_mutex);
+ }
+enabled:
account_event_cpu(event, event->cpu);
}
@@ -9344,9 +9380,6 @@ void __init perf_event_init(void)
ret = init_hw_breakpoint();
WARN(ret, "hw_breakpoint initialization failed with: %d", ret);
- /* do not patch jump label more than once per second */
- jump_label_rate_limit(&perf_sched_events, HZ);
-
/*
* Build time assertion that we keep the data_head at the intended
* location. IOW, validation we got the __reserved[] size right.
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Cure event->pending_disable race
2016-02-24 17:45 ` [PATCH 07/12] perf: Cure event->pending_disable race Peter Zijlstra
@ 2016-02-25 8:06 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:06 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, mingo, alexander.shishkin, jolsa, torvalds, tglx, peterz,
linux-kernel, hpa
Commit-ID: 28a967c3a2f99fa3b5f762f25cb2a319d933571b
Gitweb: http://git.kernel.org/tip/28a967c3a2f99fa3b5f762f25cb2a319d933571b
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:46 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:34 +0100
perf: Cure event->pending_disable race
Because event_sched_out() checks event->pending_disable _before_
actually disabling the event, it can happen that the event fires after
it checks but before it gets disabled.
This would leave event->pending_disable set and the queued irq_work
will try and process it.
However, if the event trigger was during schedule(), the event might
have been de-scheduled by the time the irq_work runs, and
perf_event_disable_local() will fail.
Fix this by checking event->pending_disable _after_ we call
event->pmu->del(). This depends on the latter being a compiler
barrier, such that the compiler does not lift the load and re-creates
the problem.
Tested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174948.040469884@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ea064ca..de14b67 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1696,14 +1696,14 @@ event_sched_out(struct perf_event *event,
perf_pmu_disable(event->pmu);
+ event->tstamp_stopped = tstamp;
+ event->pmu->del(event, 0);
+ event->oncpu = -1;
event->state = PERF_EVENT_STATE_INACTIVE;
if (event->pending_disable) {
event->pending_disable = 0;
event->state = PERF_EVENT_STATE_OFF;
}
- event->tstamp_stopped = tstamp;
- event->pmu->del(event, 0);
- event->oncpu = -1;
if (!is_software_event(event))
cpuctx->active_oncpu--;
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Fix ctx time tracking by introducing EVENT_TIME
2016-02-24 17:45 ` [PATCH 08/12] perf: Introduce EVENT_TIME Peter Zijlstra
@ 2016-02-25 8:06 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:06 UTC (permalink / raw)
To: linux-tip-commits
Cc: mingo, tglx, peterz, alexander.shishkin, torvalds, jolsa,
linux-kernel, hpa, acme
Commit-ID: 3cbaa59069677920186dcf502632ca1df4329f80
Gitweb: http://git.kernel.org/tip/3cbaa59069677920186dcf502632ca1df4329f80
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:47 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:42:34 +0100
perf: Fix ctx time tracking by introducing EVENT_TIME
Currently any ctx_sched_in() call will re-start the ctx time tracking,
this means that calls like:
ctx_sched_in(.event_type = EVENT_PINNED);
ctx_sched_in(.event_type = EVENT_FLEXIBLE);
will have a hole in their ctx time tracking. This is likely harmless
but can confuse things a little. By adding EVENT_TIME, we can have the
first ctx_sched_in() (is_active: 0 -> !0) start the time and any
further ctx_sched_in() will leave the timestamps alone.
Secondly, this allows for an early disable like:
ctx_sched_out(.event_type = EVENT_TIME);
which would update the ctx time (if the ctx is active) and any further
calls to ctx_sched_out() would not further modify the ctx time.
For ctx_sched_in() any 0 -> !0 transition will automatically include
EVENT_TIME.
For ctx_sched_out(), any transition that clears EVENT_ALL will
automatically clear EVENT_TIME.
These two rules ensure that under normal circumstances we need not
bother with EVENT_TIME and get natural ctx time behaviour.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174948.100446561@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index de14b67..75bde93 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -314,6 +314,7 @@ again:
enum event_type_t {
EVENT_FLEXIBLE = 0x1,
EVENT_PINNED = 0x2,
+ EVENT_TIME = 0x4,
EVENT_ALL = EVENT_FLEXIBLE | EVENT_PINNED,
};
@@ -1294,16 +1295,18 @@ static u64 perf_event_time(struct perf_event *event)
/*
* Update the total_time_enabled and total_time_running fields for a event.
- * The caller of this function needs to hold the ctx->lock.
*/
static void update_event_times(struct perf_event *event)
{
struct perf_event_context *ctx = event->ctx;
u64 run_end;
+ lockdep_assert_held(&ctx->lock);
+
if (event->state < PERF_EVENT_STATE_INACTIVE ||
event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
return;
+
/*
* in cgroup mode, time_enabled represents
* the time the event was enabled AND active
@@ -2349,24 +2352,33 @@ static void ctx_sched_out(struct perf_event_context *ctx,
}
ctx->is_active &= ~event_type;
+ if (!(ctx->is_active & EVENT_ALL))
+ ctx->is_active = 0;
+
if (ctx->task) {
WARN_ON_ONCE(cpuctx->task_ctx != ctx);
if (!ctx->is_active)
cpuctx->task_ctx = NULL;
}
- update_context_time(ctx);
- update_cgrp_time_from_cpuctx(cpuctx);
- if (!ctx->nr_active)
+ is_active ^= ctx->is_active; /* changed bits */
+
+ if (is_active & EVENT_TIME) {
+ /* update (and stop) ctx time */
+ update_context_time(ctx);
+ update_cgrp_time_from_cpuctx(cpuctx);
+ }
+
+ if (!ctx->nr_active || !(is_active & EVENT_ALL))
return;
perf_pmu_disable(ctx->pmu);
- if ((is_active & EVENT_PINNED) && (event_type & EVENT_PINNED)) {
+ if (is_active & EVENT_PINNED) {
list_for_each_entry(event, &ctx->pinned_groups, group_entry)
group_sched_out(event, cpuctx, ctx);
}
- if ((is_active & EVENT_FLEXIBLE) && (event_type & EVENT_FLEXIBLE)) {
+ if (is_active & EVENT_FLEXIBLE) {
list_for_each_entry(event, &ctx->flexible_groups, group_entry)
group_sched_out(event, cpuctx, ctx);
}
@@ -2740,7 +2752,7 @@ ctx_sched_in(struct perf_event_context *ctx,
if (likely(!ctx->nr_events))
return;
- ctx->is_active |= event_type;
+ ctx->is_active |= (event_type | EVENT_TIME);
if (ctx->task) {
if (!is_active)
cpuctx->task_ctx = ctx;
@@ -2748,18 +2760,24 @@ ctx_sched_in(struct perf_event_context *ctx,
WARN_ON_ONCE(cpuctx->task_ctx != ctx);
}
- now = perf_clock();
- ctx->timestamp = now;
- perf_cgroup_set_timestamp(task, ctx);
+ is_active ^= ctx->is_active; /* changed bits */
+
+ if (is_active & EVENT_TIME) {
+ /* start ctx time */
+ now = perf_clock();
+ ctx->timestamp = now;
+ perf_cgroup_set_timestamp(task, ctx);
+ }
+
/*
* First go through the list and put on any pinned groups
* in order to give them the best chance of going on.
*/
- if (!(is_active & EVENT_PINNED) && (event_type & EVENT_PINNED))
+ if (is_active & EVENT_PINNED)
ctx_pinned_sched_in(ctx, cpuctx);
/* Then walk through the lower prio flexible groups */
- if (!(is_active & EVENT_FLEXIBLE) && (event_type & EVENT_FLEXIBLE))
+ if (is_active & EVENT_FLEXIBLE)
ctx_flexible_sched_in(ctx, cpuctx);
}
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Fix scaling vs. perf_event_enable_on_exec()
2016-02-24 17:45 ` [PATCH 09/12] perf: Fix scaling vs enable_on_exec Peter Zijlstra
@ 2016-02-25 8:07 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:07 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, peterz, acme, oleg, alexander.shishkin, jolsa,
tglx, mingo, torvalds, hpa
Commit-ID: 7fce250915efca0f8f51dddee3ae89bf30d86ca5
Gitweb: http://git.kernel.org/tip/7fce250915efca0f8f51dddee3ae89bf30d86ca5
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:48 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:43:34 +0100
perf: Fix scaling vs. perf_event_enable_on_exec()
The recent commit 3e349507d12d ("perf: Fix perf_enable_on_exec() event
scheduling") caused this by moving task_ctx_sched_out() from before
__perf_event_mask_enable() to after it.
The overlooked consequence of that change is that task_ctx_sched_out()
would update the ctx time fields, and now __perf_event_mask_enable()
uses stale time.
In order to fix this, explicitly stop our context's time before
enabling the event(s).
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Fixes: 3e349507d12d ("perf: Fix perf_enable_on_exec() event scheduling")
Link: http://lkml.kernel.org/r/20160224174948.159242158@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 75bde93..d003088 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3143,6 +3143,7 @@ static void perf_event_enable_on_exec(int ctxn)
cpuctx = __get_cpu_context(ctx);
perf_ctx_lock(cpuctx, ctx);
+ ctx_sched_out(ctx, cpuctx, EVENT_TIME);
list_for_each_entry(event, &ctx->event_list, event_entry)
enabled |= event_enable_on_exec(event, ctx);
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Fix scaling vs. perf_event_enable()
2016-02-24 17:45 ` [PATCH 10/12] perf: Fix scaling vs perf_event_enable Peter Zijlstra
@ 2016-02-25 8:07 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:07 UTC (permalink / raw)
To: linux-tip-commits
Cc: torvalds, peterz, hpa, alexander.shishkin, acme, tglx, mingo,
jolsa, linux-kernel
Commit-ID: bd2afa49d194c6412c333e9fdd48bc5d06bb465d
Gitweb: http://git.kernel.org/tip/bd2afa49d194c6412c333e9fdd48bc5d06bb465d
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:49 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:44:19 +0100
perf: Fix scaling vs. perf_event_enable()
Similar to the perf_enable_on_exec(), ensure that event timings are
consistent across perf_event_enable().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174948.218288698@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 42 +++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d003088..57c25fa 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2069,14 +2069,27 @@ static void add_event_to_ctx(struct perf_event *event,
event->tstamp_stopped = tstamp;
}
-static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
- struct perf_event_context *ctx);
+static void ctx_sched_out(struct perf_event_context *ctx,
+ struct perf_cpu_context *cpuctx,
+ enum event_type_t event_type);
static void
ctx_sched_in(struct perf_event_context *ctx,
struct perf_cpu_context *cpuctx,
enum event_type_t event_type,
struct task_struct *task);
+static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
+ struct perf_event_context *ctx)
+{
+ if (!cpuctx->task_ctx)
+ return;
+
+ if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
+ return;
+
+ ctx_sched_out(ctx, cpuctx, EVENT_ALL);
+}
+
static void perf_event_sched_in(struct perf_cpu_context *cpuctx,
struct perf_event_context *ctx,
struct task_struct *task)
@@ -2227,17 +2240,18 @@ static void __perf_event_enable(struct perf_event *event,
event->state <= PERF_EVENT_STATE_ERROR)
return;
- update_context_time(ctx);
+ if (ctx->is_active)
+ ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+
__perf_event_mark_enabled(event);
if (!ctx->is_active)
return;
if (!event_filter_match(event)) {
- if (is_cgroup_event(event)) {
- perf_cgroup_set_timestamp(current, ctx); // XXX ?
+ if (is_cgroup_event(event))
perf_cgroup_defer_enabled(event);
- }
+ ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
return;
}
@@ -2245,8 +2259,10 @@ static void __perf_event_enable(struct perf_event *event,
* If the event is in a group and isn't the group leader,
* then don't put it on unless the group is on.
*/
- if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
+ if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE) {
+ ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
return;
+ }
task_ctx = cpuctx->task_ctx;
if (ctx->task)
@@ -2658,18 +2674,6 @@ void __perf_event_task_sched_out(struct task_struct *task,
perf_cgroup_sched_out(task, next);
}
-static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
- struct perf_event_context *ctx)
-{
- if (!cpuctx->task_ctx)
- return;
-
- if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
- return;
-
- ctx_sched_out(ctx, cpuctx, EVENT_ALL);
-}
-
/*
* Called with IRQs disabled
*/
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Fix scaling vs. perf_install_in_context()
2016-02-24 17:45 ` [PATCH 11/12] perf: Fix scaling vs perf_install_in_context Peter Zijlstra
@ 2016-02-25 8:07 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:07 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, tglx, peterz, alexander.shishkin, linux-kernel, jolsa,
torvalds, mingo, hpa
Commit-ID: a096309bc4677f60caa8e93fcc613a55073c51d4
Gitweb: http://git.kernel.org/tip/a096309bc4677f60caa8e93fcc613a55073c51d4
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:50 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:44:29 +0100
perf: Fix scaling vs. perf_install_in_context()
Completely reworks perf_install_in_context() (again!) in order to
ensure that there will be no ctx time hole between add_event_to_ctx()
and any potential ctx_sched_in().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174948.279399438@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 115 +++++++++++++++++++++++++++++++--------------------
1 file changed, 70 insertions(+), 45 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 57c25fa..25edabd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -276,10 +276,10 @@ static void event_function_call(struct perf_event *event, event_f func, void *da
return;
}
-again:
if (task == TASK_TOMBSTONE)
return;
+again:
if (!task_function_call(task, event_function, &efs))
return;
@@ -289,13 +289,15 @@ again:
* a concurrent perf_event_context_sched_out().
*/
task = ctx->task;
- if (task != TASK_TOMBSTONE) {
- if (ctx->is_active) {
- raw_spin_unlock_irq(&ctx->lock);
- goto again;
- }
- func(event, NULL, ctx, data);
+ if (task == TASK_TOMBSTONE) {
+ raw_spin_unlock_irq(&ctx->lock);
+ return;
+ }
+ if (ctx->is_active) {
+ raw_spin_unlock_irq(&ctx->lock);
+ goto again;
}
+ func(event, NULL, ctx, data);
raw_spin_unlock_irq(&ctx->lock);
}
@@ -2116,49 +2118,68 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
/*
* Cross CPU call to install and enable a performance event
*
- * Must be called with ctx->mutex held
+ * Very similar to remote_function() + event_function() but cannot assume that
+ * things like ctx->is_active and cpuctx->task_ctx are set.
*/
static int __perf_install_in_context(void *info)
{
- struct perf_event_context *ctx = info;
+ struct perf_event *event = info;
+ struct perf_event_context *ctx = event->ctx;
struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
struct perf_event_context *task_ctx = cpuctx->task_ctx;
+ bool activate = true;
+ int ret = 0;
raw_spin_lock(&cpuctx->ctx.lock);
if (ctx->task) {
raw_spin_lock(&ctx->lock);
- /*
- * If we hit the 'wrong' task, we've since scheduled and
- * everything should be sorted, nothing to do!
- */
task_ctx = ctx;
- if (ctx->task != current)
+
+ /* If we're on the wrong CPU, try again */
+ if (task_cpu(ctx->task) != smp_processor_id()) {
+ ret = -ESRCH;
goto unlock;
+ }
/*
- * If task_ctx is set, it had better be to us.
+ * If we're on the right CPU, see if the task we target is
+ * current, if not we don't have to activate the ctx, a future
+ * context switch will do that for us.
*/
- WARN_ON_ONCE(cpuctx->task_ctx != ctx && cpuctx->task_ctx);
+ if (ctx->task != current)
+ activate = false;
+ else
+ WARN_ON_ONCE(cpuctx->task_ctx && cpuctx->task_ctx != ctx);
+
} else if (task_ctx) {
raw_spin_lock(&task_ctx->lock);
}
- ctx_resched(cpuctx, task_ctx);
+ if (activate) {
+ ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+ add_event_to_ctx(event, ctx);
+ ctx_resched(cpuctx, task_ctx);
+ } else {
+ add_event_to_ctx(event, ctx);
+ }
+
unlock:
perf_ctx_unlock(cpuctx, task_ctx);
- return 0;
+ return ret;
}
/*
- * Attach a performance event to a context
+ * Attach a performance event to a context.
+ *
+ * Very similar to event_function_call, see comment there.
*/
static void
perf_install_in_context(struct perf_event_context *ctx,
struct perf_event *event,
int cpu)
{
- struct task_struct *task = NULL;
+ struct task_struct *task = READ_ONCE(ctx->task);
lockdep_assert_held(&ctx->mutex);
@@ -2166,42 +2187,46 @@ perf_install_in_context(struct perf_event_context *ctx,
if (event->cpu != -1)
event->cpu = cpu;
+ if (!task) {
+ cpu_function_call(cpu, __perf_install_in_context, event);
+ return;
+ }
+
+ /*
+ * Should not happen, we validate the ctx is still alive before calling.
+ */
+ if (WARN_ON_ONCE(task == TASK_TOMBSTONE))
+ return;
+
/*
* Installing events is tricky because we cannot rely on ctx->is_active
* to be set in case this is the nr_events 0 -> 1 transition.
- *
- * So what we do is we add the event to the list here, which will allow
- * a future context switch to DTRT and then send a racy IPI. If the IPI
- * fails to hit the right task, this means a context switch must have
- * happened and that will have taken care of business.
*/
- raw_spin_lock_irq(&ctx->lock);
- task = ctx->task;
-
+again:
/*
- * If between ctx = find_get_context() and mutex_lock(&ctx->mutex) the
- * ctx gets destroyed, we must not install an event into it.
- *
- * This is normally tested for after we acquire the mutex, so this is
- * a sanity check.
+ * Cannot use task_function_call() because we need to run on the task's
+ * CPU regardless of whether its current or not.
*/
+ if (!cpu_function_call(task_cpu(task), __perf_install_in_context, event))
+ return;
+
+ raw_spin_lock_irq(&ctx->lock);
+ task = ctx->task;
if (WARN_ON_ONCE(task == TASK_TOMBSTONE)) {
+ /*
+ * Cannot happen because we already checked above (which also
+ * cannot happen), and we hold ctx->mutex, which serializes us
+ * against perf_event_exit_task_context().
+ */
raw_spin_unlock_irq(&ctx->lock);
return;
}
-
- if (ctx->is_active) {
- update_context_time(ctx);
- update_cgrp_time_from_event(event);
- }
-
- add_event_to_ctx(event, ctx);
raw_spin_unlock_irq(&ctx->lock);
-
- if (task)
- task_function_call(task, __perf_install_in_context, ctx);
- else
- cpu_function_call(cpu, __perf_install_in_context, ctx);
+ /*
+ * Since !ctx->is_active doesn't mean anything, we must IPI
+ * unconditionally.
+ */
+ goto again;
}
/*
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf: Robustify task_function_call()
2016-02-24 17:45 ` [PATCH 12/12] perf: Robustify task_function_call() Peter Zijlstra
@ 2016-02-25 8:08 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-02-25 8:08 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, mingo, torvalds, alexander.shishkin, hpa, peterz,
linux-kernel, tglx, jolsa
Commit-ID: 0da4cf3e0a68c97ef811569804616a811f786729
Gitweb: http://git.kernel.org/tip/0da4cf3e0a68c97ef811569804616a811f786729
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 24 Feb 2016 18:45:51 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Feb 2016 08:44:29 +0100
perf: Robustify task_function_call()
Since there is no serialization between task_function_call() doing
task_curr() and the other CPU doing context switches, we could end
up not sending an IPI even if we had to.
And I'm not sure I still buy my own argument we're OK.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dvyukov@google.com
Cc: eranian@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160224174948.340031200@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 25edabd..6146148 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -64,8 +64,17 @@ static void remote_function(void *data)
struct task_struct *p = tfc->p;
if (p) {
- tfc->ret = -EAGAIN;
- if (task_cpu(p) != smp_processor_id() || !task_curr(p))
+ /* -EAGAIN */
+ if (task_cpu(p) != smp_processor_id())
+ return;
+
+ /*
+ * Now that we're on right CPU with IRQs disabled, we can test
+ * if we hit the right task without races.
+ */
+
+ tfc->ret = -ESRCH; /* No such (running) process */
+ if (p != current)
return;
}
@@ -92,13 +101,17 @@ task_function_call(struct task_struct *p, remote_function_f func, void *info)
.p = p,
.func = func,
.info = info,
- .ret = -ESRCH, /* No such (running) process */
+ .ret = -EAGAIN,
};
+ int ret;
- if (task_curr(p))
- smp_call_function_single(task_cpu(p), remote_function, &data, 1);
+ do {
+ ret = smp_call_function_single(task_cpu(p), remote_function, &data, 1);
+ if (!ret)
+ ret = data.ret;
+ } while (ret == -EAGAIN);
- return data.ret;
+ return ret;
}
/**
@@ -169,19 +182,6 @@ static bool is_kernel_event(struct perf_event *event)
* rely on ctx->is_active and therefore cannot use event_function_call().
* See perf_install_in_context().
*
- * This is because we need a ctx->lock serialized variable (ctx->is_active)
- * to reliably determine if a particular task/context is scheduled in. The
- * task_curr() use in task_function_call() is racy in that a remote context
- * switch is not a single atomic operation.
- *
- * As is, the situation is 'safe' because we set rq->curr before we do the
- * actual context switch. This means that task_curr() will fail early, but
- * we'll continue spinning on ctx->is_active until we've passed
- * perf_event_task_sched_out().
- *
- * Without this ctx->lock serialized variable we could have race where we find
- * the task (and hence the context) would not be active while in fact they are.
- *
* If ctx->nr_events, then ctx->is_active and cpuctx->task_ctx are set.
*/
@@ -212,7 +212,7 @@ static int event_function(void *info)
*/
if (ctx->task) {
if (ctx->task != current) {
- ret = -EAGAIN;
+ ret = -ESRCH;
goto unlock;
}
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
` (11 preceding siblings ...)
2016-02-24 17:45 ` [PATCH 12/12] perf: Robustify task_function_call() Peter Zijlstra
@ 2016-03-10 14:39 ` Peter Zijlstra
2016-03-10 14:44 ` Vince Weaver
` (2 more replies)
12 siblings, 3 replies; 36+ messages in thread
From: Peter Zijlstra @ 2016-03-10 14:39 UTC (permalink / raw)
To: mingo, alexander.shishkin, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin,
oleg, Borislav Petkov
On Wed, Feb 24, 2016 at 06:45:39PM +0100, Peter Zijlstra wrote:
> With these patches syz-kaller can still trigger some fail; most notably some
> NMI watchdog triggers and a very sporadic unthrottle bug (much like last time).
So the below seems to make the sporadic unthrottle thing much less
likely in that I haven't seen it in several hours, my machine keeps
dying on NMI watchdog bits.
Boris, who has been running syz-kaller on AMD hardware and was hitting a
very similar bug with the AMD-IBS code, says its not fixed it for him,
so maybe there's still more to find.
---
Subject: perf: Fix unthrottle
Its possible to IOC_PERIOD while the event is throttled, this would
re-start the event and the next tick would then try to unthrottle it,
and find the event wasn't actually stopped anymore.
This would tickle a WARN in the x86-pmu code which isn't expecting to
start a !stopped event.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 712570dddacd..d39477390415 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4210,6 +4210,14 @@ static void __perf_event_period(struct perf_event *event,
active = (event->state == PERF_EVENT_STATE_ACTIVE);
if (active) {
perf_pmu_disable(ctx->pmu);
+ /*
+ * We could be throttled; unthrottle now to avoid the tick
+ * trying to unthrottle while we already re-started the event.
+ */
+ if (event->hw.interrupts == MAX_INTERRUPTS) {
+ event->hw.interrupts = 0;
+ perf_log_throttle(event, 1);
+ }
event->pmu->stop(event, PERF_EF_UPDATE);
}
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-10 14:39 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
@ 2016-03-10 14:44 ` Vince Weaver
2016-03-11 14:23 ` Peter Zijlstra
2016-03-15 15:38 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-03-11 10:12 ` Alexander Shishkin
2016-03-21 9:48 ` [tip:perf/urgent] perf/core: Fix the unthrottle logic tip-bot for Peter Zijlstra
2 siblings, 2 replies; 36+ messages in thread
From: Vince Weaver @ 2016-03-10 14:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: mingo, alexander.shishkin, eranian, linux-kernel, dvyukov, andi,
jolsa, panand, sasha.levin, oleg, Borislav Petkov
On Thu, 10 Mar 2016, Peter Zijlstra wrote:
> On Wed, Feb 24, 2016 at 06:45:39PM +0100, Peter Zijlstra wrote:
>
> Boris, who has been running syz-kaller on AMD hardware and was hitting a
> very similar bug with the AMD-IBS code, says its not fixed it for him,
> so maybe there's still more to find.
sorry I am not being much help with this big syz-kaller bug hunt, but I
just wanted to chime in that there's a (probably the same) long standing
IBS bug that perf_fuzzer hits all the time too, it was bad enough
that I had to stop running perf_fuzzer on my AMD box.
Vince
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-10 14:39 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-03-10 14:44 ` Vince Weaver
@ 2016-03-11 10:12 ` Alexander Shishkin
2016-03-21 9:48 ` [tip:perf/urgent] perf/core: Fix the unthrottle logic tip-bot for Peter Zijlstra
2 siblings, 0 replies; 36+ messages in thread
From: Alexander Shishkin @ 2016-03-11 10:12 UTC (permalink / raw)
To: Peter Zijlstra, mingo, eranian
Cc: linux-kernel, vince, dvyukov, andi, jolsa, panand, sasha.levin,
oleg, Borislav Petkov
Peter Zijlstra <peterz@infradead.org> writes:
> On Wed, Feb 24, 2016 at 06:45:39PM +0100, Peter Zijlstra wrote:
>
>> With these patches syz-kaller can still trigger some fail; most notably some
>> NMI watchdog triggers and a very sporadic unthrottle bug (much like last time).
>
> So the below seems to make the sporadic unthrottle thing much less
> likely in that I haven't seen it in several hours, my machine keeps
> dying on NMI watchdog bits.
>
> Boris, who has been running syz-kaller on AMD hardware and was hitting a
> very similar bug with the AMD-IBS code, says its not fixed it for him,
> so maybe there's still more to find.
>
> ---
> Subject: perf: Fix unthrottle
>
> Its possible to IOC_PERIOD while the event is throttled, this would
> re-start the event and the next tick would then try to unthrottle it,
> and find the event wasn't actually stopped anymore.
>
> This would tickle a WARN in the x86-pmu code which isn't expecting to
> start a !stopped event.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
FWIW,
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cheers,
--
Alex
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-10 14:44 ` Vince Weaver
@ 2016-03-11 14:23 ` Peter Zijlstra
2016-03-11 15:41 ` Borislav Petkov
2016-03-21 9:49 ` [tip:perf/urgent] perf/x86/ibs: Fix IBS throttle tip-bot for Peter Zijlstra
2016-03-15 15:38 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
1 sibling, 2 replies; 36+ messages in thread
From: Peter Zijlstra @ 2016-03-11 14:23 UTC (permalink / raw)
To: Vince Weaver
Cc: mingo, alexander.shishkin, eranian, linux-kernel, dvyukov, andi,
jolsa, panand, sasha.levin, oleg, Borislav Petkov
On Thu, Mar 10, 2016 at 09:44:30AM -0500, Vince Weaver wrote:
> On Thu, 10 Mar 2016, Peter Zijlstra wrote:
>
> > On Wed, Feb 24, 2016 at 06:45:39PM +0100, Peter Zijlstra wrote:
> >
> > Boris, who has been running syz-kaller on AMD hardware and was hitting a
> > very similar bug with the AMD-IBS code, says its not fixed it for him,
> > so maybe there's still more to find.
>
> sorry I am not being much help with this big syz-kaller bug hunt, but I
> just wanted to chime in that there's a (probably the same) long standing
> IBS bug that perf_fuzzer hits all the time too, it was bad enough
> that I had to stop running perf_fuzzer on my AMD box.
The below seems to fix the IBS issue tickled by syz-kaller on my
machine. I've not yet ran perf-fuzzer, which seems able to tickle a
different set of bugs.
---
Subject: perf, amb: Fix IBS throttle
When the IBS IRQ handler get a !0 return from perf_event_overflow;
meaning it should throttle the event, it only disables it, it doesn't
call perf_ibs_stop().
This confuses the state machine, as we'll use pmu::start() ->
perf_ibs_start() to unthrottle.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/events/amd/ibs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 51087c29b2c2..7956d29762ef 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -599,7 +599,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
throttle = perf_event_overflow(event, &data, ®s);
out:
if (throttle)
- perf_ibs_disable_event(perf_ibs, hwc, *config);
+ perf_ibs_stop(event, 0);
else
perf_ibs_enable_event(perf_ibs, hwc, period >> 4);
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-11 14:23 ` Peter Zijlstra
@ 2016-03-11 15:41 ` Borislav Petkov
2016-03-21 9:49 ` [tip:perf/urgent] perf/x86/ibs: Fix IBS throttle tip-bot for Peter Zijlstra
1 sibling, 0 replies; 36+ messages in thread
From: Borislav Petkov @ 2016-03-11 15:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Vince Weaver, mingo, alexander.shishkin, eranian, linux-kernel,
dvyukov, andi, jolsa, panand, sasha.levin, oleg
On Fri, Mar 11, 2016 at 03:23:46PM +0100, Peter Zijlstra wrote:
> The below seems to fix the IBS issue tickled by syz-kaller on my
> machine. I've not yet ran perf-fuzzer, which seems able to tickle a
> different set of bugs.
>
> ---
> Subject: perf, amb: Fix IBS throttle
amd
but you know already.
>
> When the IBS IRQ handler get a !0 return from perf_event_overflow;
> meaning it should throttle the event, it only disables it, it doesn't
> call perf_ibs_stop().
>
> This confuses the state machine, as we'll use pmu::start() ->
> perf_ibs_start() to unthrottle.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Borislav Petkov <bp@suse.de>
I'll leave it running for an additional hour but if I haven't said
anything until then, all is good.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-10 14:44 ` Vince Weaver
2016-03-11 14:23 ` Peter Zijlstra
@ 2016-03-15 15:38 ` Peter Zijlstra
2016-03-16 22:59 ` Peter Zijlstra
1 sibling, 1 reply; 36+ messages in thread
From: Peter Zijlstra @ 2016-03-15 15:38 UTC (permalink / raw)
To: Vince Weaver
Cc: mingo, alexander.shishkin, eranian, linux-kernel, dvyukov, andi,
jolsa, panand, sasha.levin, oleg, Borislav Petkov
On Thu, Mar 10, 2016 at 09:44:30AM -0500, Vince Weaver wrote:
> On Thu, 10 Mar 2016, Peter Zijlstra wrote:
>
> > On Wed, Feb 24, 2016 at 06:45:39PM +0100, Peter Zijlstra wrote:
> >
> > Boris, who has been running syz-kaller on AMD hardware and was hitting a
> > very similar bug with the AMD-IBS code, says its not fixed it for him,
> > so maybe there's still more to find.
>
> sorry I am not being much help with this big syz-kaller bug hunt, but I
> just wanted to chime in that there's a (probably the same) long standing
> IBS bug that perf_fuzzer hits all the time too, it was bad enough
> that I had to stop running perf_fuzzer on my AMD box.
Running perf_fuzzer on that AMD box is still producing lots of fail, I
seen long strings of dazed and confused msgs, indicating we have a
'spurious' NMI problem somewhere.
And occasionally it locks up..
So we're not there yet.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-15 15:38 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
@ 2016-03-16 22:59 ` Peter Zijlstra
2016-03-16 23:10 ` Peter Zijlstra
2016-03-17 11:54 ` Borislav Petkov
0 siblings, 2 replies; 36+ messages in thread
From: Peter Zijlstra @ 2016-03-16 22:59 UTC (permalink / raw)
To: Vince Weaver
Cc: mingo, alexander.shishkin, eranian, linux-kernel, dvyukov, andi,
jolsa, panand, sasha.levin, oleg, Borislav Petkov
On Tue, Mar 15, 2016 at 04:38:30PM +0100, Peter Zijlstra wrote:
> Running perf_fuzzer on that AMD box is still producing lots of fail, I
> seen long strings of dazed and confused msgs, indicating we have a
> 'spurious' NMI problem somewhere.
>
> And occasionally it locks up..
>
> So we're not there yet.
So the below appears to alleviate some of this; but the hangs are
quicker now, so maybe I just made it worse.
---
Subject: perf, ibs: Fix race with IBS_STARTING state
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Mar 16 23:55:21 CET 2016
While tracing the IBS bits I saw the NMI hitting between clearing
IBS_STARTING and the actual MSR writes to disable the counter.
Since IBS_STARTING was cleared, the handler assumed these were spurious
NMIs and because STOPPING wasn't set yet either, insta-triggered an
"Unknown NMI".
Cure this by clearing IBS_STARTING after disabling the hardware.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/events/amd/ibs.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -376,7 +376,13 @@ static void perf_ibs_start(struct perf_e
hwc->state = 0;
perf_ibs_set_period(perf_ibs, hwc, &period);
+ /*
+ * Set STARTED before enabling the hardware, such that
+ * a subsequent NMI must observe it. Then clear STOPPING
+ * such that we don't consume NMIs by accident.
+ */
set_bit(IBS_STARTED, pcpu->state);
+ clear_bit(IBS_STOPPING, pcpu->state);
perf_ibs_enable_event(perf_ibs, hwc, period >> 4);
perf_event_update_userpage(event);
@@ -390,7 +396,7 @@ static void perf_ibs_stop(struct perf_ev
u64 config;
int stopping;
- stopping = test_and_clear_bit(IBS_STARTED, pcpu->state);
+ stopping = test_bit(IBS_STARTED, pcpu->state);
if (!stopping && (hwc->state & PERF_HES_UPTODATE))
return;
@@ -398,8 +404,24 @@ static void perf_ibs_stop(struct perf_ev
rdmsrl(hwc->config_base, config);
if (stopping) {
+ /*
+ * Set STOPPING before disabling the hardware, such that it
+ * must be visible to NMIs the moment we clear the EN bit,
+ * at which point we can generate an !VALID sample which
+ * we need to consume.
+ */
set_bit(IBS_STOPPING, pcpu->state);
perf_ibs_disable_event(perf_ibs, hwc, config);
+ /*
+ * Clear STARTED after disabling the hardware; if it were
+ * cleared before an NMI hitting after the clear but before
+ * clearing the EN bit might think it a spurious NMI and not
+ * handle it.
+ *
+ * Clearing it after, however, creates the problem of the NMI
+ * handler seeing STARTED but not having a valid sample.
+ */
+ clear_bit(IBS_STARTED, pcpu->state);
WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
hwc->state |= PERF_HES_STOPPED;
}
@@ -527,20 +549,24 @@ static int perf_ibs_handle_irq(struct pe
u64 *buf, *config, period;
if (!test_bit(IBS_STARTED, pcpu->state)) {
+fail:
/*
* Catch spurious interrupts after stopping IBS: After
* disabling IBS there could be still incoming NMIs
* with samples that even have the valid bit cleared.
* Mark all this NMIs as handled.
*/
- return test_and_clear_bit(IBS_STOPPING, pcpu->state) ? 1 : 0;
+ if (test_and_clear_bit(IBS_STOPPING, pcpu->state))
+ return 1;
+
+ return 0;
}
msr = hwc->config_base;
buf = ibs_data.regs;
rdmsrl(msr, *buf);
if (!(*buf++ & perf_ibs->valid_mask))
- return 0;
+ goto fail;
config = &ibs_data.regs[0];
perf_ibs_event_update(perf_ibs, event, config);
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-16 22:59 ` Peter Zijlstra
@ 2016-03-16 23:10 ` Peter Zijlstra
2016-03-17 11:54 ` Borislav Petkov
1 sibling, 0 replies; 36+ messages in thread
From: Peter Zijlstra @ 2016-03-16 23:10 UTC (permalink / raw)
To: Vince Weaver
Cc: mingo, alexander.shishkin, eranian, linux-kernel, dvyukov, andi,
jolsa, panand, sasha.levin, oleg, Borislav Petkov
On Wed, Mar 16, 2016 at 11:59:33PM +0100, Peter Zijlstra wrote:
> Subject: perf, ibs: Fix race with IBS_STARTING state
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Mar 16 23:55:21 CET 2016
>
> While tracing the IBS bits I saw the NMI hitting between clearing
> IBS_STARTING and the actual MSR writes to disable the counter.
>
> Since IBS_STARTING was cleared, the handler assumed these were spurious
> NMIs and because STOPPING wasn't set yet either, insta-triggered an
> "Unknown NMI".
>
> Cure this by clearing IBS_STARTING after disabling the hardware.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> arch/x86/events/amd/ibs.c | 32 +++++++++++++++++++++++++++++---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -376,7 +376,13 @@ static void perf_ibs_start(struct perf_e
> hwc->state = 0;
>
> perf_ibs_set_period(perf_ibs, hwc, &period);
> + /*
> + * Set STARTED before enabling the hardware, such that
> + * a subsequent NMI must observe it. Then clear STOPPING
> + * such that we don't consume NMIs by accident.
> + */
> set_bit(IBS_STARTED, pcpu->state);
> + clear_bit(IBS_STOPPING, pcpu->state);
> perf_ibs_enable_event(perf_ibs, hwc, period >> 4);
Also, all those atomic ops are probably entirely overkill and we could
use the non-atomic ops. This is all strictly cpu local. But I didn't
want to change too much at once, esp. while there's still problems.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 00/12] perf: more fixes
2016-03-16 22:59 ` Peter Zijlstra
2016-03-16 23:10 ` Peter Zijlstra
@ 2016-03-17 11:54 ` Borislav Petkov
1 sibling, 0 replies; 36+ messages in thread
From: Borislav Petkov @ 2016-03-17 11:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Vince Weaver, mingo, alexander.shishkin, eranian, linux-kernel,
dvyukov, andi, jolsa, panand, sasha.levin, oleg
On Wed, Mar 16, 2016 at 11:59:33PM +0100, Peter Zijlstra wrote:
> On Tue, Mar 15, 2016 at 04:38:30PM +0100, Peter Zijlstra wrote:
>
> > Running perf_fuzzer on that AMD box is still producing lots of fail, I
> > seen long strings of dazed and confused msgs, indicating we have a
> > 'spurious' NMI problem somewhere.
> >
> > And occasionally it locks up..
> >
> > So we're not there yet.
>
> So the below appears to alleviate some of this; but the hangs are
> quicker now, so maybe I just made it worse.
>
> ---
> Subject: perf, ibs: Fix race with IBS_STARTING state
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Mar 16 23:55:21 CET 2016
>
> While tracing the IBS bits I saw the NMI hitting between clearing
> IBS_STARTING and the actual MSR writes to disable the counter.
>
> Since IBS_STARTING was cleared, the handler assumed these were spurious
> NMIs and because STOPPING wasn't set yet either, insta-triggered an
> "Unknown NMI".
>
> Cure this by clearing IBS_STARTING after disabling the hardware.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Borislav Petkov <bp@suse.de>
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf/core: Fix the unthrottle logic
2016-03-10 14:39 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-03-10 14:44 ` Vince Weaver
2016-03-11 10:12 ` Alexander Shishkin
@ 2016-03-21 9:48 ` tip-bot for Peter Zijlstra
2 siblings, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-03-21 9:48 UTC (permalink / raw)
To: linux-tip-commits
Cc: mingo, dvlasenk, hpa, linux-kernel, eranian, vincent.weaver,
peterz, brgerst, acme, bp, dsahern, tglx, jolsa, luto, torvalds,
namhyung, alexander.shishkin
Commit-ID: 1e02cd40f15190b78fcc6b3f50c952fb4028e9a5
Gitweb: http://git.kernel.org/tip/1e02cd40f15190b78fcc6b3f50c952fb4028e9a5
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 10 Mar 2016 15:39:24 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 21 Mar 2016 09:08:14 +0100
perf/core: Fix the unthrottle logic
Its possible to IOC_PERIOD while the event is throttled, this would
re-start the event and the next tick would then try to unthrottle it,
and find the event wasn't actually stopped anymore.
This would tickle a WARN in the x86-pmu code which isn't expecting to
start a !stopped event.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: dvyukov@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Cc: vince@deater.net
Link: http://lkml.kernel.org/r/20160310143924.GR6356@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 712570d..d394773 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4210,6 +4210,14 @@ static void __perf_event_period(struct perf_event *event,
active = (event->state == PERF_EVENT_STATE_ACTIVE);
if (active) {
perf_pmu_disable(ctx->pmu);
+ /*
+ * We could be throttled; unthrottle now to avoid the tick
+ * trying to unthrottle while we already re-started the event.
+ */
+ if (event->hw.interrupts == MAX_INTERRUPTS) {
+ event->hw.interrupts = 0;
+ perf_log_throttle(event, 1);
+ }
event->pmu->stop(event, PERF_EF_UPDATE);
}
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [tip:perf/urgent] perf/x86/ibs: Fix IBS throttle
2016-03-11 14:23 ` Peter Zijlstra
2016-03-11 15:41 ` Borislav Petkov
@ 2016-03-21 9:49 ` tip-bot for Peter Zijlstra
1 sibling, 0 replies; 36+ messages in thread
From: tip-bot for Peter Zijlstra @ 2016-03-21 9:49 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, jolsa, peterz, namhyung, vince, luto, bp, hpa, mingo,
dsahern, eranian, torvalds, vincent.weaver, dvlasenk, brgerst,
tglx, alexander.shishkin, linux-kernel
Commit-ID: 0158b83f7508851cda9e62c4b14678c5bd492cb2
Gitweb: http://git.kernel.org/tip/0158b83f7508851cda9e62c4b14678c5bd492cb2
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 11 Mar 2016 15:23:46 +0100
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 21 Mar 2016 09:08:15 +0100
perf/x86/ibs: Fix IBS throttle
When the IBS IRQ handler get a !0 return from perf_event_overflow;
meaning it should throttle the event, it only disables it, it doesn't
call perf_ibs_stop().
This confuses the state machine, as we'll use pmu::start() ->
perf_ibs_start() to unthrottle.
Tested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vince@deater.net>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: dvyukov@google.com
Cc: oleg@redhat.com
Cc: panand@redhat.com
Cc: sasha.levin@oracle.com
Link: http://lkml.kernel.org/r/20160311142346.GE6344@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/events/amd/ibs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 51087c2..7956d29 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -599,7 +599,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
throttle = perf_event_overflow(event, &data, ®s);
out:
if (throttle)
- perf_ibs_disable_event(perf_ibs, hwc, *config);
+ perf_ibs_stop(event, 0);
else
perf_ibs_enable_event(perf_ibs, hwc, period >> 4);
^ permalink raw reply related [flat|nested] 36+ messages in thread
end of thread, other threads:[~2016-03-21 9:53 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-24 17:45 [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-02-24 17:45 ` [PATCH 01/12] perf: Close install vs exit race Peter Zijlstra
2016-02-25 8:03 ` [tip:perf/urgent] perf: Close install vs. " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 02/12] perf: Do not double free Peter Zijlstra
2016-02-25 8:04 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 03/12] perf: Allow perf_release() with !event->ctx Peter Zijlstra
2016-02-25 8:04 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 04/12] perf: Only update context time when active Peter Zijlstra
2016-02-25 8:04 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 05/12] perf: Fix cloning Peter Zijlstra
2016-02-25 8:05 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 06/12] perf: Fix race between event install and jump_labels Peter Zijlstra
2016-02-25 8:05 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 07/12] perf: Cure event->pending_disable race Peter Zijlstra
2016-02-25 8:06 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 08/12] perf: Introduce EVENT_TIME Peter Zijlstra
2016-02-25 8:06 ` [tip:perf/urgent] perf: Fix ctx time tracking by introducing EVENT_TIME tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 09/12] perf: Fix scaling vs enable_on_exec Peter Zijlstra
2016-02-25 8:07 ` [tip:perf/urgent] perf: Fix scaling vs. perf_event_enable_on_exec() tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 10/12] perf: Fix scaling vs perf_event_enable Peter Zijlstra
2016-02-25 8:07 ` [tip:perf/urgent] perf: Fix scaling vs. perf_event_enable() tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 11/12] perf: Fix scaling vs perf_install_in_context Peter Zijlstra
2016-02-25 8:07 ` [tip:perf/urgent] perf: Fix scaling vs. perf_install_in_context() tip-bot for Peter Zijlstra
2016-02-24 17:45 ` [PATCH 12/12] perf: Robustify task_function_call() Peter Zijlstra
2016-02-25 8:08 ` [tip:perf/urgent] " tip-bot for Peter Zijlstra
2016-03-10 14:39 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-03-10 14:44 ` Vince Weaver
2016-03-11 14:23 ` Peter Zijlstra
2016-03-11 15:41 ` Borislav Petkov
2016-03-21 9:49 ` [tip:perf/urgent] perf/x86/ibs: Fix IBS throttle tip-bot for Peter Zijlstra
2016-03-15 15:38 ` [PATCH 00/12] perf: more fixes Peter Zijlstra
2016-03-16 22:59 ` Peter Zijlstra
2016-03-16 23:10 ` Peter Zijlstra
2016-03-17 11:54 ` Borislav Petkov
2016-03-11 10:12 ` Alexander Shishkin
2016-03-21 9:48 ` [tip:perf/urgent] perf/core: Fix the unthrottle logic tip-bot for Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.