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

Groups > linux.kernel > #1282918 > unrolled thread

[PATCH 2/7] perf: Generalize task_function_call()ers

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

  [PATCH 2/7] perf: Generalize task_function_call()ers Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-12-03 11:40 +0100
    Re: [PATCH 2/7] perf: Generalize task_function_call()ers Peter Zijlstra <peterz@infradead.org> - 2015-12-03 18:40 +0100
      Re: [PATCH 2/7] perf: Generalize task_function_call()ers Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-12-08 17:50 +0100
        Re: [PATCH 2/7] perf: Generalize task_function_call()ers Peter Zijlstra <peterz@infradead.org> - 2015-12-08 18:00 +0100

#1282918 — [PATCH 2/7] perf: Generalize task_function_call()ers

A number of places in perf core (perf_event_enable, perf_event_disable,
perf_install_in_context, perf_remove_from_context) are performing the
same tapdance around task_function_call() to modify an event and/or its
context on its target cpu. The sequence mainly consists of re-trying
task_function_call() until it either succeeds or is no longer necessary
or the event's context has gone inactive, in which case the action can
be performed on the caller's cpu provided the context lock is still
held. The complication that all callers are dealing with is that they
have to release the context lock before calling task_function_call(),
which gives an opportunity for the context in question to get scheduled
out, so then they have to re-acquire the lock, check whether that is
the case and re-try the cross-call.

This patch creates a more generic helper that either executes a callback
on the target cpu or returns with ctx::lock locked, so that the caller
can perform its action on an inactive context. This patch also converts
the existing users of this sequence to use the new helper.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h |   1 +
 kernel/events/core.c       | 190 +++++++++++++++------------------------------
 2 files changed, 64 insertions(+), 127 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index f9828a48f1..55290d609e 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -402,6 +402,7 @@ enum perf_event_active_state {
 	PERF_EVENT_STATE_OFF		= -1,
 	PERF_EVENT_STATE_INACTIVE	=  0,
 	PERF_EVENT_STATE_ACTIVE		=  1,
+	PERF_EVENT_STATE_NONE		=  2,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5854fcf7f0..0d3296f600 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -126,6 +126,59 @@ static int cpu_function_call(int cpu, remote_function_f func, void *info)
 	return data.ret;
 }
 
+static int
+remote_call_or_ctx_lock(struct perf_event *event, remote_function_f func,
+			void *info, enum perf_event_active_state retry_state)
+{
+	struct perf_event_context *ctx = event->ctx;
+	struct task_struct *task = ctx->task;
+	unsigned int retry = 1;
+
+	if (!task) {
+		/*
+		 * Per cpu events are removed via an smp call. The removal can
+		 * fail if the CPU is currently offline, but in that case we
+		 * already called __perf_remove_from_context from
+		 * perf_event_exit_cpu.
+		 */
+		cpu_function_call(event->cpu, func, info);
+		return 0;
+	}
+
+	raw_spin_lock_irq(&ctx->lock);
+	do {
+		/*
+		 * Reload the task pointer, it might have been changed by
+		 * a concurrent perf_event_context_sched_out().
+		 */
+		task = ctx->task;
+
+		/*
+		 * If the context is inactive, we don't need a cross call to
+		 * fiddle with the event so long as the ctx::lock is held.
+		 */
+		if (!ctx->is_active)
+			break;
+
+		raw_spin_unlock_irq(&ctx->lock);
+
+		if (!task_function_call(task, func, info))
+			return 0;
+
+		raw_spin_lock_irq(&ctx->lock);
+
+		if (retry_state <= PERF_EVENT_STATE_ACTIVE)
+			retry = event->state == retry_state;
+	} while (retry);
+
+	/*
+	 * No luck: leaving with ctx::lock held and interrupts disabled
+	 * so that the caller can safely fiddle with event or context.
+	 */
+
+	return -ENXIO;
+}
+
 #define EVENT_OWNER_KERNEL ((void *) -1)
 
 static bool is_kernel_event(struct perf_event *event)
@@ -1673,7 +1726,6 @@ static int __perf_remove_from_context(void *info)
 static void perf_remove_from_context(struct perf_event *event, bool detach_group)
 {
 	struct perf_event_context *ctx = event->ctx;
-	struct task_struct *task = ctx->task;
 	struct remove_event re = {
 		.event = event,
 		.detach_group = detach_group,
@@ -1681,36 +1733,10 @@ static void perf_remove_from_context(struct perf_event *event, bool detach_group
 
 	lockdep_assert_held(&ctx->mutex);
 
-	if (!task) {
-		/*
-		 * Per cpu events are removed via an smp call. The removal can
-		 * fail if the CPU is currently offline, but in that case we
-		 * already called __perf_remove_from_context from
-		 * perf_event_exit_cpu.
-		 */
-		cpu_function_call(event->cpu, __perf_remove_from_context, &re);
-		return;
-	}
-
-retry:
-	if (!task_function_call(task, __perf_remove_from_context, &re))
+	if (!remote_call_or_ctx_lock(event, __perf_remove_from_context, &re,
+				     PERF_EVENT_STATE_NONE))
 		return;
 
-	raw_spin_lock_irq(&ctx->lock);
-	/*
-	 * If we failed to find a running task, but find the context active now
-	 * that we've acquired the ctx->lock, retry.
-	 */
-	if (ctx->is_active) {
-		raw_spin_unlock_irq(&ctx->lock);
-		/*
-		 * Reload the task pointer, it might have been changed by
-		 * a concurrent perf_event_context_sched_out().
-		 */
-		task = ctx->task;
-		goto retry;
-	}
-
 	/*
 	 * Since the task isn't running, its safe to remove the event, us
 	 * holding the ctx->lock ensures the task won't get scheduled in.
@@ -1778,34 +1804,10 @@ int __perf_event_disable(void *info)
 static void _perf_event_disable(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
-	struct task_struct *task = ctx->task;
-
-	if (!task) {
-		/*
-		 * Disable the event on the cpu that it's on
-		 */
-		cpu_function_call(event->cpu, __perf_event_disable, event);
-		return;
-	}
 
-retry:
-	if (!task_function_call(task, __perf_event_disable, event))
+	if (!remote_call_or_ctx_lock(event, __perf_event_disable, event, PERF_EVENT_STATE_ACTIVE))
 		return;
 
-	raw_spin_lock_irq(&ctx->lock);
-	/*
-	 * If the event is still active, we need to retry the cross-call.
-	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
-		raw_spin_unlock_irq(&ctx->lock);
-		/*
-		 * Reload the task pointer, it might have been changed by
-		 * a concurrent perf_event_context_sched_out().
-		 */
-		task = ctx->task;
-		goto retry;
-	}
-
 	/*
 	 * Since we have the lock this context can't be scheduled
 	 * in, so we can change the state safely.
@@ -2143,42 +2145,16 @@ perf_install_in_context(struct perf_event_context *ctx,
 			struct perf_event *event,
 			int cpu)
 {
-	struct task_struct *task = ctx->task;
-
 	lockdep_assert_held(&ctx->mutex);
 
 	event->ctx = ctx;
 	if (event->cpu != -1)
 		event->cpu = cpu;
 
-	if (!task) {
-		/*
-		 * Per cpu events are installed via an smp call and
-		 * the install is always successful.
-		 */
-		cpu_function_call(cpu, __perf_install_in_context, event);
-		return;
-	}
-
-retry:
-	if (!task_function_call(task, __perf_install_in_context, event))
+	if (!remote_call_or_ctx_lock(event, __perf_install_in_context, event,
+				     PERF_EVENT_STATE_NONE))
 		return;
 
-	raw_spin_lock_irq(&ctx->lock);
-	/*
-	 * If we failed to find a running task, but find the context active now
-	 * that we've acquired the ctx->lock, retry.
-	 */
-	if (ctx->is_active) {
-		raw_spin_unlock_irq(&ctx->lock);
-		/*
-		 * Reload the task pointer, it might have been changed by
-		 * a concurrent perf_event_context_sched_out().
-		 */
-		task = ctx->task;
-		goto retry;
-	}
-
 	/*
 	 * Since the task isn't running, its safe to add the event, us holding
 	 * the ctx->lock ensures the task won't get scheduled in.
@@ -2299,15 +2275,6 @@ unlock:
 static void _perf_event_enable(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
-	struct task_struct *task = ctx->task;
-
-	if (!task) {
-		/*
-		 * Enable the event on the cpu that it's on
-		 */
-		cpu_function_call(event->cpu, __perf_event_enable, event);
-		return;
-	}
 
 	raw_spin_lock_irq(&ctx->lock);
 	if (event->state >= PERF_EVENT_STATE_INACTIVE)
@@ -2322,32 +2289,13 @@ static void _perf_event_enable(struct perf_event *event)
 	 */
 	if (event->state == PERF_EVENT_STATE_ERROR)
 		event->state = PERF_EVENT_STATE_OFF;
-
-retry:
-	if (!ctx->is_active) {
-		__perf_event_mark_enabled(event);
-		goto out;
-	}
-
 	raw_spin_unlock_irq(&ctx->lock);
 
-	if (!task_function_call(task, __perf_event_enable, event))
+	if (!remote_call_or_ctx_lock(event, __perf_event_enable, event, PERF_EVENT_STATE_OFF))
 		return;
 
-	raw_spin_lock_irq(&ctx->lock);
-
-	/*
-	 * If the context is active and the event is still off,
-	 * we need to retry the cross-call.
-	 */
-	if (ctx->is_active && event->state == PERF_EVENT_STATE_OFF) {
-		/*
-		 * task could have been flipped by a concurrent
-		 * perf_event_context_sched_out()
-		 */
-		task = ctx->task;
-		goto retry;
-	}
+	if (!ctx->is_active)
+		__perf_event_mark_enabled(event);
 
 out:
 	raw_spin_unlock_irq(&ctx->lock);
@@ -4209,22 +4157,10 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
 	task = ctx->task;
 	pe.value = value;
 
-	if (!task) {
-		cpu_function_call(event->cpu, __perf_event_period, &pe);
-		return 0;
-	}
-
-retry:
-	if (!task_function_call(task, __perf_event_period, &pe))
+	if (!remote_call_or_ctx_lock(event, __perf_event_period, &pe,
+				     PERF_EVENT_STATE_NONE))
 		return 0;
 
-	raw_spin_lock_irq(&ctx->lock);
-	if (ctx->is_active) {
-		raw_spin_unlock_irq(&ctx->lock);
-		task = ctx->task;
-		goto retry;
-	}
-
 	__perf_event_period(&pe);
 	raw_spin_unlock_irq(&ctx->lock);
 
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1283215

On Thu, Dec 03, 2015 at 12:32:37PM +0200, Alexander Shishkin wrote:
> +static int
> +remote_call_or_ctx_lock(struct perf_event *event, remote_function_f func,
> +			void *info, enum perf_event_active_state retry_state)
> +{
> +	struct perf_event_context *ctx = event->ctx;
> +	struct task_struct *task = ctx->task;
> +	unsigned int retry = 1;
> +
> +	if (!task) {
> +		/*
> +		 * Per cpu events are removed via an smp call. The removal can
> +		 * fail if the CPU is currently offline, but in that case we
> +		 * already called __perf_remove_from_context from
> +		 * perf_event_exit_cpu.
> +		 */
> +		cpu_function_call(event->cpu, func, info);
> +		return 0;
> +	}
> +
> +	raw_spin_lock_irq(&ctx->lock);
> +	do {
> +		/*
> +		 * Reload the task pointer, it might have been changed by
> +		 * a concurrent perf_event_context_sched_out().
> +		 */
> +		task = ctx->task;
> +
> +		/*
> +		 * If the context is inactive, we don't need a cross call to
> +		 * fiddle with the event so long as the ctx::lock is held.
> +		 */
> +		if (!ctx->is_active)
> +			break;
> +
> +		raw_spin_unlock_irq(&ctx->lock);
> +
> +		if (!task_function_call(task, func, info))
> +			return 0;
> +
> +		raw_spin_lock_irq(&ctx->lock);
> +
> +		if (retry_state <= PERF_EVENT_STATE_ACTIVE)
> +			retry = event->state == retry_state;
> +	} while (retry);

OK, so the retry_state thing is clever, but either I'm too tired or its
not quite right. Nor do I think its actually required.

/me frobs...

Hmm, I cannot seem to convince myself the current code is correct to
begin with.

In any case, consider the below (on top of my previous collapse patch).
The two 'hard' cases are perf_event_{dis,en}able(), those appear to play
silly games with event->state.

So starting with perf_event_disable(); we don't strictly need to test
for event->state == ACTIVE, ctx->is_active is enough. If the event is
not scheduled while the ctx is, __perf_event_disable() still does the
right thing.  Its a little less efficient to IPI in that case, over-all
simpler.

For perf_event_enable(); the same goes, but I think that's actually
broken in its current form. The current condition is: ctx->is_active &&
event->state == OFF, that means it doesn't do anything when !ctx->active
&& event->state == OFF. This is wrong, it should still mark the event
INACTIVE in that case, otherwise we'll still not try and schedule the
event once the context becomes active again.


--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1766,6 +1766,20 @@ int __perf_event_disable(void *info)
 	return 0;
 }
 
+void ___perf_event_disable(void *info)
+{
+	struct perf_event *event = info;
+
+	/*
+	 * 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;
+	}
+}
+
 /*
  * Disable a event.
  *
@@ -1782,43 +1796,16 @@ int __perf_event_disable(void *info)
 static void _perf_event_disable(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
-	struct task_struct *task = ctx->task;
-
-	if (!task) {
-		/*
-		 * Disable the event on the cpu that it's on
-		 */
-		cpu_function_call(event->cpu, __perf_event_disable, event);
-		return;
-	}
-
-retry:
-	if (!task_function_call(task, __perf_event_disable, event))
-		return;
 
 	raw_spin_lock_irq(&ctx->lock);
-	/*
-	 * If the event is still active, we need to retry the cross-call.
-	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+	if (event->state <= PERF_EVENT_STATE_OFF) {
 		raw_spin_unlock_irq(&ctx->lock);
-		/*
-		 * Reload the task pointer, it might have been changed by
-		 * a concurrent perf_event_context_sched_out().
-		 */
-		task = ctx->task;
-		goto retry;
-	}
-
-	/*
-	 * 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;
+		return;
 	}
 	raw_spin_unlock_irq(&ctx->lock);
+
+	event_function_call(event, __perf_event_disable,
+			    ___perf_event_disable, event);
 }
 
 /*
@@ -2269,6 +2256,11 @@ static int __perf_event_enable(void *inf
 	return 0;
 }
 
+void ___perf_event_enable(void *info)
+{
+	__perf_event_mark_enabled((struct perf_event *)info);
+}
+
 /*
  * Enable a event.
  *
@@ -2281,58 +2273,26 @@ static int __perf_event_enable(void *inf
 static void _perf_event_enable(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
-	struct task_struct *task = ctx->task;
 
-	if (!task) {
-		/*
-		 * Enable the event on the cpu that it's on
-		 */
-		cpu_function_call(event->cpu, __perf_event_enable, event);
+	raw_spin_lock_irq(&ctx->lock);
+	if (event->state >= PERF_EVENT_STATE_INACTIVE) {
+		raw_spin_unlock_irq(&ctx->lock);
 		return;
 	}
 
-	raw_spin_lock_irq(&ctx->lock);
-	if (event->state >= PERF_EVENT_STATE_INACTIVE)
-		goto out;
-
 	/*
 	 * If the event is in error state, clear that first.
-	 * That way, if we see the event in error state below, we
-	 * know that it has gone back into error state, as distinct
-	 * from the task having been scheduled away before the
-	 * cross-call arrived.
+	 *
+	 * That way, if we see the event in error state below, we know that it
+	 * has gone back into error state, as distinct from the task having
+	 * been scheduled away before the cross-call arrived.
 	 */
 	if (event->state == PERF_EVENT_STATE_ERROR)
 		event->state = PERF_EVENT_STATE_OFF;
-
-retry:
-	if (!ctx->is_active) {
-		__perf_event_mark_enabled(event);
-		goto out;
-	}
-
 	raw_spin_unlock_irq(&ctx->lock);
 
-	if (!task_function_call(task, __perf_event_enable, event))
-		return;
-
-	raw_spin_lock_irq(&ctx->lock);
-
-	/*
-	 * If the context is active and the event is still off,
-	 * we need to retry the cross-call.
-	 */
-	if (ctx->is_active && event->state == PERF_EVENT_STATE_OFF) {
-		/*
-		 * task could have been flipped by a concurrent
-		 * perf_event_context_sched_out()
-		 */
-		task = ctx->task;
-		goto retry;
-	}
-
-out:
-	raw_spin_unlock_irq(&ctx->lock);
+	event_function_call(event, __perf_event_enable,
+			    ___perf_event_enable, event);
 }
 
 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286677

Peter Zijlstra <peterz@infradead.org> writes:

> OK, so the retry_state thing is clever, but either I'm too tired or its
> not quite right. Nor do I think its actually required.
>
> /me frobs...
>
> Hmm, I cannot seem to convince myself the current code is correct to
> begin with.
>
> In any case, consider the below (on top of my previous collapse patch).
> The two 'hard' cases are perf_event_{dis,en}able(), those appear to play
> silly games with event->state.
>
> So starting with perf_event_disable(); we don't strictly need to test
> for event->state == ACTIVE, ctx->is_active is enough. If the event is
> not scheduled while the ctx is, __perf_event_disable() still does the
> right thing.  Its a little less efficient to IPI in that case, over-all
> simpler.
>
> For perf_event_enable(); the same goes, but I think that's actually
> broken in its current form. The current condition is: ctx->is_active &&
> event->state == OFF, that means it doesn't do anything when !ctx->active
> && event->state == OFF. This is wrong, it should still mark the event
> INACTIVE in that case, otherwise we'll still not try and schedule the
> event once the context becomes active again.

Yes, this does look more logically correct.

>
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1766,6 +1766,20 @@ int __perf_event_disable(void *info)
>  	return 0;
>  }
>  
> +void ___perf_event_disable(void *info)

Only maybe change these to __perf_event_disable_locked() or something
visually distinctive from the 'active' callback?

FWIW,

Reviewed-by: Alexander Shishkin <alexander.shishkin@intel.com>

Thanks,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286690

On Tue, Dec 08, 2015 at 06:42:01PM +0200, Alexander Shishkin wrote:
> > +void ___perf_event_disable(void *info)
> 
> Only maybe change these to __perf_event_disable_locked() or something
> visually distinctive from the 'active' callback?

Yeah, I ran out of naming-foo and punted. I'll give it another go
tomorrow.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web