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

[Alexander Shishkin]

I think I caught one, below.

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