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

Groups > linux.kernel > #1306480 > unrolled thread

[RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

Back to article view | Back to linux.kernel

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

Contents

  [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users Peter Zijlstra <peterz@infradead.org> - 2016-01-11 17:40 +0100
    Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call()  users Peter Zijlstra <peterz@infradead.org> - 2016-01-13 12:00 +0100
      Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-13 14:50 +0100
        Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call()  users Peter Zijlstra <peterz@infradead.org> - 2016-01-13 18:40 +0100
    Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-13 16:10 +0100
      Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-13 16:50 +0100
      Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call()  users Peter Zijlstra <peterz@infradead.org> - 2016-01-13 19:20 +0100
        Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call()  users Peter Zijlstra <peterz@infradead.org> - 2016-01-13 21:50 +0100
      Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call()  users Peter Zijlstra <peterz@infradead.org> - 2016-01-14 11:50 +0100
        Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call()  users Peter Zijlstra <peterz@infradead.org> - 2016-01-14 17:40 +0100

#1306480 — [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

There is one common bug left in all the event_function_call() users,
between loading ctx->task and getting to the remote_function(),
ctx->task can already have been changed.

Therefore we need to double check and retry if ctx->task != current.

Insert another trampoline specific to event_function_call() that
checks for this and further validates state. This also allows getting
rid of the active/inactive functions.


Note: Stephane, can you please look at __perf_event_enable()?

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/perf_event.h    |    2 
 kernel/events/core.c          |  362 ++++++++++++++++++------------------------
 kernel/events/hw_breakpoint.c |    2 
 3 files changed, 164 insertions(+), 202 deletions(-)

--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1044,7 +1044,7 @@ extern void perf_swevent_put_recursion_c
 extern u64 perf_swevent_set_period(struct perf_event *event);
 extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
-extern int __perf_event_disable(void *info);
+extern void perf_event_disable_local(struct perf_event *event);
 extern void perf_event_task_tick(void);
 #else /* !CONFIG_PERF_EVENTS: */
 static inline void *
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -126,6 +126,28 @@ static int cpu_function_call(int cpu, re
 	return data.ret;
 }
 
+static inline struct perf_cpu_context *
+__get_cpu_context(struct perf_event_context *ctx)
+{
+	return this_cpu_ptr(ctx->pmu->pmu_cpu_context);
+}
+
+static void perf_ctx_lock(struct perf_cpu_context *cpuctx,
+			  struct perf_event_context *ctx)
+{
+	raw_spin_lock(&cpuctx->ctx.lock);
+	if (ctx)
+		raw_spin_lock(&ctx->lock);
+}
+
+static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
+			    struct perf_event_context *ctx)
+{
+	if (ctx)
+		raw_spin_unlock(&ctx->lock);
+	raw_spin_unlock(&cpuctx->ctx.lock);
+}
+
 /*
  * On task ctx scheduling...
  *
@@ -158,21 +180,96 @@ static int cpu_function_call(int cpu, re
  * If ctx->nr_events, then ctx->is_active and cpuctx->task_ctx are set.
  */
 
-static void event_function_call(struct perf_event *event,
-				int (*active)(void *),
-				void (*inactive)(void *),
-				void *data)
+typedef void (*event_f)(struct perf_event *, struct perf_cpu_context *,
+			struct perf_event_context *, void *);
+
+struct event_function_struct {
+	struct perf_event *event;
+	event_f func;
+	void *data;
+};
+
+static int event_function(void *info)
+{
+	struct event_function_struct *efs = info;
+	struct perf_event *event = efs->event;
+	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;
+
+	WARN_ON_ONCE(!irqs_disabled());
+
+	/*
+	 * Since we do the IPI call without holding ctx->lock things can have
+	 * changed, double check we hit the task we set out to hit.
+	 *
+	 * If ctx->task == current, we know things must remain valid because
+	 * we have IRQs disabled so we cannot schedule.
+	 */
+	if (ctx->task) {
+		if (ctx->task != current)
+			return -EAGAIN;
+
+		WARN_ON_ONCE(task_ctx != ctx);
+	} else {
+		WARN_ON_ONCE(&cpuctx->ctx != ctx);
+	}
+
+	perf_ctx_lock(cpuctx, task_ctx);
+	/*
+	 * Now that we hold locks, double check state. Paranoia pays.
+	 */
+	if (task_ctx) {
+		WARN_ON_ONCE(task_ctx->task != current);
+		/*
+		 * We only use event_function_call() on established contexts,
+		 * and event_function() is only ever called when active (or
+		 * rather, we'll have bailed in task_function_call() or the
+		 * above ctx->task != current test), therefore we must have
+		 * ctx->is_active here.
+		 */
+		WARN_ON_ONCE(!ctx->is_active);
+		/*
+		 * And since we have ctx->is_active, cpuctx->task_ctx must
+		 * match.
+		 */
+		WARN_ON_ONCE(cpuctx->task_ctx != task_ctx);
+	}
+	efs->func(event, cpuctx, ctx, efs->data);
+	perf_ctx_unlock(cpuctx, task_ctx);
+
+	return 0;
+}
+
+static void event_function_local(struct perf_event *event, event_f func, void *data)
+{
+	struct event_function_struct efs = {
+		.event = event,
+		.func = func,
+		.data = data,
+	};
+
+	int ret = event_function(&efs);
+	WARN_ON_ONCE(ret);
+}
+
+static void event_function_call(struct perf_event *event, event_f func, void *data)
 {
 	struct perf_event_context *ctx = event->ctx;
 	struct task_struct *task = ctx->task;
+	struct event_function_struct efs = {
+		.event = event,
+		.func = func,
+		.data = data,
+	};
 
 	if (!task) {
-		cpu_function_call(event->cpu, active, data);
+		cpu_function_call(event->cpu, event_function, &efs);
 		return;
 	}
 
 again:
-	if (!task_function_call(task, active, data))
+	if (!task_function_call(task, event_function, &efs))
 		return;
 
 	raw_spin_lock_irq(&ctx->lock);
@@ -185,7 +282,7 @@ static void event_function_call(struct p
 		raw_spin_unlock_irq(&ctx->lock);
 		goto again;
 	}
-	inactive(data);
+	func(event, NULL, ctx, data);
 	raw_spin_unlock_irq(&ctx->lock);
 }
 
@@ -400,28 +497,6 @@ static inline u64 perf_event_clock(struc
 	return event->clock();
 }
 
-static inline struct perf_cpu_context *
-__get_cpu_context(struct perf_event_context *ctx)
-{
-	return this_cpu_ptr(ctx->pmu->pmu_cpu_context);
-}
-
-static void perf_ctx_lock(struct perf_cpu_context *cpuctx,
-			  struct perf_event_context *ctx)
-{
-	raw_spin_lock(&cpuctx->ctx.lock);
-	if (ctx)
-		raw_spin_lock(&ctx->lock);
-}
-
-static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
-			    struct perf_event_context *ctx)
-{
-	if (ctx)
-		raw_spin_unlock(&ctx->lock);
-	raw_spin_unlock(&cpuctx->ctx.lock);
-}
-
 #ifdef CONFIG_CGROUP_PERF
 
 static inline bool
@@ -1684,38 +1759,22 @@ group_sched_out(struct perf_event *group
 		cpuctx->exclusive = 0;
 }
 
-struct remove_event {
-	struct perf_event *event;
-	bool detach_group;
-};
-
-static void ___perf_remove_from_context(void *info)
-{
-	struct remove_event *re = info;
-	struct perf_event *event = re->event;
-	struct perf_event_context *ctx = event->ctx;
-
-	if (re->detach_group)
-		perf_group_detach(event);
-	list_del_event(event, ctx);
-}
-
 /*
  * Cross CPU call to remove a performance event
  *
  * We disable the event on the hardware level first. After that we
  * remove it from the context list.
  */
-static int __perf_remove_from_context(void *info)
+static void
+__perf_remove_from_context(struct perf_event *event,
+			   struct perf_cpu_context *cpuctx,
+			   struct perf_event_context *ctx,
+			   void *info)
 {
-	struct remove_event *re = info;
-	struct perf_event *event = re->event;
-	struct perf_event_context *ctx = event->ctx;
-	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
+	bool detach_group = (unsigned long)info;
 
-	raw_spin_lock(&ctx->lock);
 	event_sched_out(event, cpuctx, ctx);
-	if (re->detach_group)
+	if (detach_group)
 		perf_group_detach(event);
 	list_del_event(event, ctx);
 
@@ -1726,17 +1785,11 @@ static int __perf_remove_from_context(vo
 			cpuctx->task_ctx = NULL;
 		}
 	}
-	raw_spin_unlock(&ctx->lock);
-
-	return 0;
 }
 
 /*
  * Remove the event from a task's (or a CPU's) list of events.
  *
- * CPU events are removed with a smp call. For task events we only
- * call when the task is on a CPU.
- *
  * If event->ctx is a cloned context, callers must make sure that
  * every task struct that event->ctx->task could possibly point to
  * remains valid.  This is OK when called from perf_release since
@@ -1746,71 +1799,31 @@ static int __perf_remove_from_context(vo
  */
 static void perf_remove_from_context(struct perf_event *event, bool detach_group)
 {
-	struct perf_event_context *ctx = event->ctx;
-	struct remove_event re = {
-		.event = event,
-		.detach_group = detach_group,
-	};
-
-	lockdep_assert_held(&ctx->mutex);
+	lockdep_assert_held(&event->ctx->mutex);
 
 	event_function_call(event, __perf_remove_from_context,
-			    ___perf_remove_from_context, &re);
+			    (void *)(unsigned long)detach_group);
 }
 
 /*
  * Cross CPU call to disable a performance event
  */
-int __perf_event_disable(void *info)
-{
-	struct perf_event *event = info;
-	struct perf_event_context *ctx = event->ctx;
-	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
-
-	/*
-	 * If this is a per-task event, need to check whether this
-	 * event's task is the current task on this cpu.
-	 *
-	 * Can trigger due to concurrent perf_event_context_sched_out()
-	 * flipping contexts around.
-	 */
-	if (ctx->task && cpuctx->task_ctx != ctx)
-		return -EINVAL;
-
-	raw_spin_lock(&ctx->lock);
-
-	/*
-	 * If the event is on, turn it off.
-	 * If it is in error state, leave it in error state.
-	 */
-	if (event->state >= PERF_EVENT_STATE_INACTIVE) {
-		update_context_time(ctx);
-		update_cgrp_time_from_event(event);
-		update_group_times(event);
-		if (event == event->group_leader)
-			group_sched_out(event, cpuctx, ctx);
-		else
-			event_sched_out(event, cpuctx, ctx);
-		event->state = PERF_EVENT_STATE_OFF;
-	}
-
-	raw_spin_unlock(&ctx->lock);
-
-	return 0;
-}
-
-void ___perf_event_disable(void *info)
+static void __perf_event_disable(struct perf_event *event,
+				 struct perf_cpu_context *cpuctx,
+				 struct perf_event_context *ctx,
+				 void *info)
 {
-	struct perf_event *event = info;
+	if (event->state < PERF_EVENT_STATE_INACTIVE)
+		return;
 
-	/*
-	 * Since we have the lock this context can't be scheduled
-	 * in, so we can change the state safely.
-	 */
-	if (event->state == PERF_EVENT_STATE_INACTIVE) {
-		update_group_times(event);
-		event->state = PERF_EVENT_STATE_OFF;
-	}
+	update_context_time(ctx);
+	update_cgrp_time_from_event(event);
+	update_group_times(event);
+	if (event == event->group_leader)
+		group_sched_out(event, cpuctx, ctx);
+	else
+		event_sched_out(event, cpuctx, ctx);
+	event->state = PERF_EVENT_STATE_OFF;
 }
 
 /*
@@ -1837,8 +1850,12 @@ static void _perf_event_disable(struct p
 	}
 	raw_spin_unlock_irq(&ctx->lock);
 
-	event_function_call(event, __perf_event_disable,
-			    ___perf_event_disable, event);
+	event_function_call(event, __perf_event_disable, NULL);
+}
+
+void perf_event_disable_local(struct perf_event *event)
+{
+	event_function_local(event, __perf_event_disable, NULL);
 }
 
 /*
@@ -2202,44 +2219,28 @@ static void __perf_event_mark_enabled(st
 /*
  * Cross CPU call to enable a performance event
  */
-static int __perf_event_enable(void *info)
+static void __perf_event_enable(struct perf_event *event,
+				struct perf_cpu_context *cpuctx,
+				struct perf_event_context *ctx,
+				void *info)
 {
-	struct perf_event *event = info;
-	struct perf_event_context *ctx = event->ctx;
 	struct perf_event *leader = event->group_leader;
-	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
-	struct perf_event_context *task_ctx = cpuctx->task_ctx;
-
-	/*
-	 * There's a time window between 'ctx->is_active' check
-	 * in perf_event_enable function and this place having:
-	 *   - IRQs on
-	 *   - ctx->lock unlocked
-	 *
-	 * where the task could be killed and 'ctx' deactivated
-	 * by perf_event_exit_task.
-	 */
-	if (!ctx->is_active)
-		return -EINVAL;
-
-	perf_ctx_lock(cpuctx, task_ctx);
-	WARN_ON_ONCE(&cpuctx->ctx != ctx && task_ctx != ctx);
-	update_context_time(ctx);
 
 	if (event->state >= PERF_EVENT_STATE_INACTIVE)
-		goto unlock;
-
-	/*
-	 * set current task's cgroup time reference point
-	 */
-	perf_cgroup_set_timestamp(current, ctx);
+		return;
 
+	update_context_time(ctx);
 	__perf_event_mark_enabled(event);
 
+	if (!ctx->is_active)
+		return;
+
 	if (!event_filter_match(event)) {
-		if (is_cgroup_event(event))
+		if (is_cgroup_event(event)) {
+			perf_cgroup_set_timestamp(current, ctx); // XXX ?
 			perf_cgroup_defer_enabled(event);
-		goto unlock;
+		}
+		return;
 	}
 
 	/*
@@ -2247,19 +2248,9 @@ static int __perf_event_enable(void *inf
 	 * then don't put it on unless the group is on.
 	 */
 	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
-		goto unlock;
-
-	ctx_resched(cpuctx, task_ctx);
-
-unlock:
-	perf_ctx_unlock(cpuctx, task_ctx);
-
-	return 0;
-}
+		return;
 
-void ___perf_event_enable(void *info)
-{
-	__perf_event_mark_enabled((struct perf_event *)info);
+	ctx_resched(cpuctx, ctx);
 }
 
 /*
@@ -2292,8 +2283,7 @@ static void _perf_event_enable(struct pe
 		event->state = PERF_EVENT_STATE_OFF;
 	raw_spin_unlock_irq(&ctx->lock);
 
-	event_function_call(event, __perf_event_enable,
-			    ___perf_event_enable, event);
+	event_function_call(event, __perf_event_enable, NULL);
 }
 
 /*
@@ -4095,36 +4085,14 @@ static void perf_event_for_each(struct p
 		perf_event_for_each_child(sibling, func);
 }
 
-struct period_event {
-	struct perf_event *event;
-	u64 value;
-};
-
-static void ___perf_event_period(void *info)
-{
-	struct period_event *pe = info;
-	struct perf_event *event = pe->event;
-	u64 value = pe->value;
-
-	if (event->attr.freq) {
-		event->attr.sample_freq = value;
-	} else {
-		event->attr.sample_period = value;
-		event->hw.sample_period = value;
-	}
-
-	local64_set(&event->hw.period_left, 0);
-}
-
-static int __perf_event_period(void *info)
+static void __perf_event_period(struct perf_event *event,
+				struct perf_cpu_context *cpuctx,
+				struct perf_event_context *ctx,
+				void *info)
 {
-	struct period_event *pe = info;
-	struct perf_event *event = pe->event;
-	struct perf_event_context *ctx = event->ctx;
-	u64 value = pe->value;
+	u64 value = *((u64 *)info);
 	bool active;
 
-	raw_spin_lock(&ctx->lock);
 	if (event->attr.freq) {
 		event->attr.sample_freq = value;
 	} else {
@@ -4144,14 +4112,10 @@ static int __perf_event_period(void *inf
 		event->pmu->start(event, PERF_EF_RELOAD);
 		perf_pmu_enable(ctx->pmu);
 	}
-	raw_spin_unlock(&ctx->lock);
-
-	return 0;
 }
 
 static int perf_event_period(struct perf_event *event, u64 __user *arg)
 {
-	struct period_event pe = { .event = event, };
 	u64 value;
 
 	if (!is_sampling_event(event))
@@ -4166,10 +4130,7 @@ static int perf_event_period(struct perf
 	if (event->attr.freq && value > sysctl_perf_event_sample_rate)
 		return -EINVAL;
 
-	pe.value = value;
-
-	event_function_call(event, __perf_event_period,
-			    ___perf_event_period, &pe);
+	event_function_call(event, __perf_event_period, &value);
 
 	return 0;
 }
@@ -4941,7 +4902,7 @@ static void perf_pending_event(struct ir
 
 	if (event->pending_disable) {
 		event->pending_disable = 0;
-		__perf_event_disable(event);
+		perf_event_disable_local(event);
 	}
 
 	if (event->pending_wakeup) {
@@ -9239,13 +9200,14 @@ static void perf_event_init_cpu(int cpu)
 #if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC_CORE
 static void __perf_event_exit_context(void *__info)
 {
-	struct remove_event re = { .detach_group = true };
 	struct perf_event_context *ctx = __info;
+	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
+	struct perf_event *event;
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(re.event, &ctx->event_list, event_entry)
-		__perf_remove_from_context(&re);
-	rcu_read_unlock();
+	raw_spin_lock(&ctx->lock);
+	list_for_each_entry(event, &ctx->event_list, event_entry)
+		__perf_remove_from_context(event, cpuctx, ctx, (void *)(unsigned long)true);
+	raw_spin_unlock(&ctx->lock);
 }
 
 static void perf_event_exit_cpu_context(int cpu)
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -444,7 +444,7 @@ int modify_user_hw_breakpoint(struct per
 	 * current task.
 	 */
 	if (irqs_disabled() && bp->ctx && bp->ctx->task == current)
-		__perf_event_disable(bp);
+		perf_event_disable_local(bp);
 	else
 		perf_event_disable(bp);
 

[toc] | [next] | [standalone]

#1308283 — Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

On Mon, Jan 11, 2016 at 05:25:10PM +0100, Peter Zijlstra wrote:
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2202,44 +2219,28 @@ static void __perf_event_mark_enabled(st
>  /*
>   * Cross CPU call to enable a performance event
>   */
> +static void __perf_event_enable(struct perf_event *event,
> +				struct perf_cpu_context *cpuctx,
> +				struct perf_event_context *ctx,
> +				void *info)
>  {
>  	struct perf_event *leader = event->group_leader;
> -	struct perf_event_context *task_ctx = cpuctx->task_ctx;

deleted too much ^^

>  
>  	if (event->state >= PERF_EVENT_STATE_INACTIVE)
> +		return;
>  
> +	update_context_time(ctx);
>  	__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 ?
>  			perf_cgroup_defer_enabled(event);
> +		}
> +		return;
>  	}
>  
>  	/*
> @@ -2247,19 +2248,9 @@ static int __perf_event_enable(void *inf
>  	 * then don't put it on unless the group is on.
>  	 */
>  	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
> +		return;
>  
> +	ctx_resched(cpuctx, ctx);
>  }


--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2225,6 +2243,7 @@ static void __perf_event_enable(struct p
 				void *info)
 {
 	struct perf_event *leader = event->group_leader;
+	struct perf_event_context *task_ctx;
 
 	if (event->state >= PERF_EVENT_STATE_INACTIVE)
 		return;
@@ -2250,7 +2269,11 @@ static void __perf_event_enable(struct p
 	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
 		return;
 
-	ctx_resched(cpuctx, ctx);
+	task_ctx = cpuctx->task_ctx;
+	if (ctx->task)
+		WARN_ON_ONCE(task_ctx != ctx);
+
+	ctx_resched(cpuctx, task_ctx);
 }
 
 /*

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

#1308419

Peter Zijlstra <peterz@infradead.org> writes:

> @@ -2250,7 +2269,11 @@ static void __perf_event_enable(struct p
>  	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
>  		return;
>  
> -	ctx_resched(cpuctx, ctx);
> +	task_ctx = cpuctx->task_ctx;
> +	if (ctx->task)
> +		WARN_ON_ONCE(task_ctx != ctx);
> +
> +	ctx_resched(cpuctx, task_ctx);

Afaict, ctx_resched() path already does this in task_ctx_sched_out().

Regards,
--
Alex

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

#1308678 — Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

On Wed, Jan 13, 2016 at 03:46:58PM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > @@ -2250,7 +2269,11 @@ static void __perf_event_enable(struct p
> >  	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE)
> >  		return;
> >  
> > -	ctx_resched(cpuctx, ctx);
> > +	task_ctx = cpuctx->task_ctx;
> > +	if (ctx->task)
> > +		WARN_ON_ONCE(task_ctx != ctx);
> > +
> > +	ctx_resched(cpuctx, task_ctx);
> 
> Afaict, ctx_resched() path already does this in task_ctx_sched_out().

It does not; that got changed somewhere along the way :-)

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

#1308489

I think I caught one, below.

Peter Zijlstra <peterz@infradead.org> writes:

> +static int event_function(void *info)
> +{
> +	struct event_function_struct *efs = info;
> +	struct perf_event *event = efs->event;
> +	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;
> +
> +	WARN_ON_ONCE(!irqs_disabled());
> +
> +	/*
> +	 * Since we do the IPI call without holding ctx->lock things can have
> +	 * changed, double check we hit the task we set out to hit.
> +	 *
> +	 * If ctx->task == current, we know things must remain valid because
> +	 * we have IRQs disabled so we cannot schedule.
> +	 */
> +	if (ctx->task) {
> +		if (ctx->task != current)
> +			return -EAGAIN;
> +
> +		WARN_ON_ONCE(task_ctx != ctx);

Looks like between dropping ctx::lock in event_function_call() and here,
cpuctx::task_ctx may still become NULL.

> +	} else {
> +		WARN_ON_ONCE(&cpuctx->ctx != ctx);
> +	}
> +
> +	perf_ctx_lock(cpuctx, task_ctx);
> +	/*
> +	 * Now that we hold locks, double check state. Paranoia pays.
> +	 */
> +	if (task_ctx) {
> +		WARN_ON_ONCE(task_ctx->task != current);
> +		/*
> +		 * We only use event_function_call() on established contexts,
> +		 * and event_function() is only ever called when active (or
> +		 * rather, we'll have bailed in task_function_call() or the
> +		 * above ctx->task != current test), therefore we must have
> +		 * ctx->is_active here.
> +		 */
> +		WARN_ON_ONCE(!ctx->is_active);
> +		/*
> +		 * And since we have ctx->is_active, cpuctx->task_ctx must
> +		 * match.
> +		 */
> +		WARN_ON_ONCE(cpuctx->task_ctx != task_ctx);
> +	}
> +	efs->func(event, cpuctx, ctx, efs->data);

In which case we probably don't want to call the callback.

Not sure if this is what Dmitry ran into, his logs contain warnings from
this function, but hard to tell exactly which ones.

Regards,
--
Alex

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

#1308526

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> Not sure if this is what Dmitry ran into, his logs contain warnings from
> this function, but hard to tell exactly which ones.

Looks like it is:

>  2016/01/12 12:08:26 qemu-12: saving crash 'WARNING: CPU: 2 PID: 26946
>  at kernel/events/core.c:213 event_function+0x4ba/0x590()' to
>  crash-qemu-12-1452596906889159952

https://gist.github.com/dvyukov/3b3a4993ade37e344636#file-gistfile1-txt-L213

Regards,
--
Alex

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

#1308701 — Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

On Wed, Jan 13, 2016 at 05:00:50PM +0200, Alexander Shishkin wrote:
> I think I caught one, below.
> 
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > +static int event_function(void *info)
> > +{
> > +	struct event_function_struct *efs = info;
> > +	struct perf_event *event = efs->event;
> > +	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;
> > +
> > +	WARN_ON_ONCE(!irqs_disabled());
> > +
> > +	/*
> > +	 * Since we do the IPI call without holding ctx->lock things can have
> > +	 * changed, double check we hit the task we set out to hit.
> > +	 *
> > +	 * If ctx->task == current, we know things must remain valid because
> > +	 * we have IRQs disabled so we cannot schedule.
> > +	 */
> > +	if (ctx->task) {
> > +		if (ctx->task != current)
> > +			return -EAGAIN;
> > +
> > +		WARN_ON_ONCE(task_ctx != ctx);
> 
> Looks like between dropping ctx::lock in event_function_call() and here,
> cpuctx::task_ctx may still become NULL.

Hmm yes I think you're right. If the event is being migrated to another
context concurrently the remove_from_context() might have gone through,
we'll still be waiting for sync_rcu and then this (say enabled) happens
and we're looking at a 'dead' context.

Thanks!

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

#1308805 — Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

On Wed, Jan 13, 2016 at 07:10:16PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 13, 2016 at 05:00:50PM +0200, Alexander Shishkin wrote:
> > I think I caught one, below.
> > 
> > Peter Zijlstra <peterz@infradead.org> writes:
> > 
> > > +static int event_function(void *info)
> > > +{
> > > +	struct event_function_struct *efs = info;
> > > +	struct perf_event *event = efs->event;
> > > +	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;
> > > +
> > > +	WARN_ON_ONCE(!irqs_disabled());
> > > +
> > > +	/*
> > > +	 * Since we do the IPI call without holding ctx->lock things can have
> > > +	 * changed, double check we hit the task we set out to hit.
> > > +	 *
> > > +	 * If ctx->task == current, we know things must remain valid because
> > > +	 * we have IRQs disabled so we cannot schedule.
> > > +	 */
> > > +	if (ctx->task) {
> > > +		if (ctx->task != current)
> > > +			return -EAGAIN;
> > > +
> > > +		WARN_ON_ONCE(task_ctx != ctx);
> > 
> > Looks like between dropping ctx::lock in event_function_call() and here,
> > cpuctx::task_ctx may still become NULL.
> 
> Hmm yes I think you're right. If the event is being migrated to another
> context concurrently the remove_from_context() might have gone through,
> we'll still be waiting for sync_rcu and then this (say enabled) happens
> and we're looking at a 'dead' context.

Hmm, no. This cannot be, all event_function_call() users should hold
ctx->mutex.

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

#1309171 — Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

On Wed, Jan 13, 2016 at 05:00:50PM +0200, Alexander Shishkin wrote:
> I think I caught one, below.
> 
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > +static int event_function(void *info)
> > +{
> > +	struct event_function_struct *efs = info;
> > +	struct perf_event *event = efs->event;
> > +	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;
> > +
> > +	WARN_ON_ONCE(!irqs_disabled());
> > +
> > +	/*
> > +	 * Since we do the IPI call without holding ctx->lock things can have
> > +	 * changed, double check we hit the task we set out to hit.
> > +	 *
> > +	 * If ctx->task == current, we know things must remain valid because
> > +	 * we have IRQs disabled so we cannot schedule.
> > +	 */
> > +	if (ctx->task) {
> > +		if (ctx->task != current)
> > +			return -EAGAIN;
> > +
> > +		WARN_ON_ONCE(task_ctx != ctx);
> 
> Looks like between dropping ctx::lock in event_function_call() and here,
> cpuctx::task_ctx may still become NULL.

You were indeed correct, and I've found out how and why.

Its a race with perf_event_exit_task(), which will schedule the context
out. I think there's a bunch of races around this area, but in general I
was planning on making event_function_call() a no-op if !(event->attach
& PERF_ATTACH_CONTEXT).

You cannot rely on ->is_active here, because while the context will be
inactive, you still do not want it to call ->func().

Let me prod at this a bit more.

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

#1309472 — Re: [RFC][PATCH 12/12] perf: Collapse and fix event_function_call() users

On Thu, Jan 14, 2016 at 11:44:49AM +0100, Peter Zijlstra wrote:
> On Wed, Jan 13, 2016 at 05:00:50PM +0200, Alexander Shishkin wrote:
> > I think I caught one, below.

> > Looks like between dropping ctx::lock in event_function_call() and here,
> > cpuctx::task_ctx may still become NULL.
> 
> You were indeed correct, and I've found out how and why.
> 
> Its a race with perf_event_exit_task(), which will schedule the context
> out. I think there's a bunch of races around this area, but in general I
> was planning on making event_function_call() a no-op if !(event->attach
> & PERF_ATTACH_CONTEXT).
> 
> You cannot rely on ->is_active here, because while the context will be
> inactive, you still do not want it to call ->func().
> 
> Let me prod at this a bit more.

Does this make sense?

---
Subject: perf: Fix perf_event_exit_task() race
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Jan 14 16:05:37 CET 2016

There is a race against perf_event_exit_task() vs
event_function_call(),find_get_context(),perf_install_in_context()
(iow, everyone).

Since there is no permanent marker on a context that its dead, it is
quite possible that we access (and even modify) a context after its
passed through perf_event_exit_task().

For instance, find_get_context() might find the context still
installed, but by the time we get to perf_install_in_context() it
might already have passed through perf_event_exit_task() and be
considered dead, we will however still add the event to it.

Solve this by marking a ctx dead by setting its ctx->task value to -1,
it must be !0 so we still know its a (former) task context.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |  151 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 85 insertions(+), 66 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -148,6 +148,13 @@ static void perf_ctx_unlock(struct perf_
 	raw_spin_unlock(&cpuctx->ctx.lock);
 }
 
+#define TASK_TOMBSTONE ((void *)-1L)
+
+static bool is_kernel_event(struct perf_event *event)
+{
+	return event->owner == TASK_TOMBSTONE;
+}
+
 /*
  * On task ctx scheduling...
  *
@@ -196,31 +203,21 @@ static int event_function(void *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;
+	int ret = 0;
 
 	WARN_ON_ONCE(!irqs_disabled());
 
+	perf_ctx_lock(cpuctx, task_ctx);
 	/*
 	 * Since we do the IPI call without holding ctx->lock things can have
 	 * changed, double check we hit the task we set out to hit.
-	 *
-	 * If ctx->task == current, we know things must remain valid because
-	 * we have IRQs disabled so we cannot schedule.
 	 */
 	if (ctx->task) {
-		if (ctx->task != current)
-			return -EAGAIN;
-
-		WARN_ON_ONCE(task_ctx != ctx);
-	} else {
-		WARN_ON_ONCE(&cpuctx->ctx != ctx);
-	}
+		if (ctx->task != current) {
+			ret = -EAGAIN;
+			goto unlock;
+		}
 
-	perf_ctx_lock(cpuctx, task_ctx);
-	/*
-	 * Now that we hold locks, double check state. Paranoia pays.
-	 */
-	if (task_ctx) {
-		WARN_ON_ONCE(task_ctx->task != current);
 		/*
 		 * We only use event_function_call() on established contexts,
 		 * and event_function() is only ever called when active (or
@@ -233,12 +230,16 @@ static int event_function(void *info)
 		 * And since we have ctx->is_active, cpuctx->task_ctx must
 		 * match.
 		 */
-		WARN_ON_ONCE(cpuctx->task_ctx != task_ctx);
+		WARN_ON_ONCE(task_ctx != ctx);
+	} else {
+		WARN_ON_ONCE(&cpuctx->ctx != ctx);
 	}
+
 	efs->func(event, cpuctx, ctx, efs->data);
+unlock:
 	perf_ctx_unlock(cpuctx, task_ctx);
 
-	return 0;
+	return ret;
 }
 
 static void event_function_local(struct perf_event *event, event_f func, void *data)
@@ -256,7 +257,7 @@ static void event_function_local(struct
 static void event_function_call(struct perf_event *event, event_f func, void *data)
 {
 	struct perf_event_context *ctx = event->ctx;
-	struct task_struct *task = ctx->task;
+	struct task_struct *task = READ_ONCE(ctx->task); /* verified in event_function */
 	struct event_function_struct efs = {
 		.event = event,
 		.func = func,
@@ -278,30 +279,28 @@ static void event_function_call(struct p
 	}
 
 again:
+	if (task == TASK_TOMBSTONE)
+		return;
+
 	if (!task_function_call(task, event_function, &efs))
 		return;
 
 	raw_spin_lock_irq(&ctx->lock);
-	if (ctx->is_active) {
-		/*
-		 * Reload the task pointer, it might have been changed by
-		 * a concurrent perf_event_context_sched_out().
-		 */
-		task = ctx->task;
-		raw_spin_unlock_irq(&ctx->lock);
-		goto again;
+	/*
+	 * Reload the task pointer, it might have been changed by
+	 * 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);
 	}
-	func(event, NULL, ctx, data);
 	raw_spin_unlock_irq(&ctx->lock);
 }
 
-#define EVENT_OWNER_KERNEL ((void *) -1)
-
-static bool is_kernel_event(struct perf_event *event)
-{
-	return event->owner == EVENT_OWNER_KERNEL;
-}
-
 #define PERF_FLAG_ALL (PERF_FLAG_FD_NO_GROUP |\
 		       PERF_FLAG_FD_OUTPUT  |\
 		       PERF_FLAG_PID_CGROUP |\
@@ -1025,7 +1024,7 @@ static void put_ctx(struct perf_event_co
 	if (atomic_dec_and_test(&ctx->refcount)) {
 		if (ctx->parent_ctx)
 			put_ctx(ctx->parent_ctx);
-		if (ctx->task)
+		if (ctx->task && ctx->task != TASK_TOMBSTONE)
 			put_task_struct(ctx->task);
 		call_rcu(&ctx->rcu_head, free_ctx);
 	}
@@ -1186,6 +1185,7 @@ static u64 primary_event_id(struct perf_
 
 /*
  * Get the perf_event_context for a task and lock it.
+ *
  * This has to cope with with the fact that until it is locked,
  * the context could get moved to another task.
  */
@@ -1226,10 +1226,13 @@ perf_lock_task_context(struct task_struc
 			goto retry;
 		}
 
-		if (!atomic_inc_not_zero(&ctx->refcount)) {
+		if (ctx->task == TASK_TOMBSTONE ||
+		    !atomic_inc_not_zero(&ctx->refcount)) {
 			raw_spin_unlock(&ctx->lock);
 			ctx = NULL;
 		}
+
+		WARN_ON_ONCE(ctx->task != task);
 	}
 	rcu_read_unlock();
 	if (!ctx)
@@ -2140,23 +2143,27 @@ static int  __perf_install_in_context(vo
 	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
 	struct perf_event_context *task_ctx = cpuctx->task_ctx;
 
+	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)
-			return 0;
+			goto unlock;
 
 		/*
 		 * If task_ctx is set, it had better be to us.
 		 */
 		WARN_ON_ONCE(cpuctx->task_ctx != ctx && cpuctx->task_ctx);
-		task_ctx = ctx;
+	} else if (task_ctx) {
+		raw_spin_lock(&task_ctx->lock);
 	}
 
-	perf_ctx_lock(cpuctx, task_ctx);
 	ctx_resched(cpuctx, task_ctx);
+unlock:
 	perf_ctx_unlock(cpuctx, task_ctx);
 
 	return 0;
@@ -2188,6 +2195,17 @@ perf_install_in_context(struct perf_even
 	 * happened and that will have taken care of business.
 	 */
 	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 (task == TASK_TOMBSTONE) {
+		raw_spin_unlock_irq(&ctx->lock);
+		return;
+	}
 	update_context_time(ctx);
 	/*
 	 * Update cgrp time only if current cgrp matches event->cgrp.
@@ -2195,7 +2213,6 @@ perf_install_in_context(struct perf_even
 	 */
 	update_cgrp_time_from_event(event);
 	add_event_to_ctx(event, ctx);
-	task = ctx->task;
 	raw_spin_unlock_irq(&ctx->lock);
 
 	if (task)
@@ -2538,17 +2555,21 @@ static void perf_event_context_sched_out
 		raw_spin_lock(&ctx->lock);
 		raw_spin_lock_nested(&next_ctx->lock, SINGLE_DEPTH_NESTING);
 		if (context_equiv(ctx, next_ctx)) {
-			/*
-			 * XXX do we need a memory barrier of sorts
-			 * wrt to rcu_dereference() of perf_event_ctxp
-			 */
-			task->perf_event_ctxp[ctxn] = next_ctx;
-			next->perf_event_ctxp[ctxn] = ctx;
-			ctx->task = next;
-			next_ctx->task = task;
+			WRITE_ONCE(ctx->task, next);
+			WRITE_ONCE(next_ctx->task, task);
 
 			swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
 
+			/*
+			 * RCU_INIT_POINTER here is safe because we've not
+			 * modified the ctx and the above modification of
+			 * ctx->task and ctx->task_ctx_data are immaterial
+			 * since those values are always verified under
+			 * ctx->lock which we're now holding.
+			 */
+			RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], next_ctx);
+			RCU_INIT_POINTER(next->perf_event_ctxp[ctxn], ctx);
+
 			do_switch = 0;
 
 			perf_event_sync_stat(ctx, next_ctx);
@@ -8545,7 +8566,7 @@ perf_event_create_kernel_counter(struct
 	}
 
 	/* Mark owner so we could distinguish it from user events. */
-	event->owner = EVENT_OWNER_KERNEL;
+	event->owner = TASK_TOMBSTONE;
 
 	account_event(event);
 
@@ -8725,28 +8746,26 @@ __perf_event_exit_task(struct perf_event
 
 static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
 {
-	struct perf_event *child_event, *next;
 	struct perf_event_context *child_ctx, *clone_ctx = NULL;
+	struct perf_event *child_event, *next;
+	unsigned long flags;
+
+	WARN_ON_ONCE(child != current);
 
-	if (likely(!child->perf_event_ctxp[ctxn]))
+	child_ctx = perf_lock_task_context(child, ctxn, &flags);
+	if (!child_ctx)
 		return;
 
-	local_irq_disable();
-	WARN_ON_ONCE(child != current);
-	/*
-	 * We can't reschedule here because interrupts are disabled,
-	 * and child must be current.
-	 */
-	child_ctx = rcu_dereference_raw(child->perf_event_ctxp[ctxn]);
+	task_ctx_sched_out(__get_cpu_context(child_ctx), child_ctx);
 
 	/*
-	 * Take the context lock here so that if find_get_context is
-	 * reading child->perf_event_ctxp, we wait until it has
-	 * incremented the context's refcount before we do put_ctx below.
+	 * Now that the context is inactive, destroy the task <-> ctx relation
+	 * and mark the context dead.
 	 */
-	raw_spin_lock(&child_ctx->lock);
-	task_ctx_sched_out(__get_cpu_context(child_ctx), child_ctx);
-	child->perf_event_ctxp[ctxn] = NULL;
+	RCU_INIT_POINTER(child->perf_event_ctxp[ctxn], NULL);
+	put_ctx(child_ctx); /* cannot be last */
+	WRITE_ONCE(child_ctx->task, TASK_TOMBSTONE);
+	put_task_struct(current); /* cannot be last */
 
 	/*
 	 * If this context is a clone; unclone it so it can't get
@@ -8755,7 +8774,7 @@ static void perf_event_exit_task_context
 	 */
 	clone_ctx = unclone_ctx(child_ctx);
 	update_context_time(child_ctx);
-	raw_spin_unlock_irq(&child_ctx->lock);
+	raw_spin_unlock_irqrestore(&child_ctx->lock, flags);
 
 	if (clone_ctx)
 		put_ctx(clone_ctx);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web