Re: Q: perf_install_in_context/perf_event_enable are racy?

[Oleg Nesterov <oleg@redhat.com>]

On 01/21, Frederic Weisbecker wrote:
>
> +static DEFINE_PER_CPU(int, task_events_schedulable);

Yes, I think this can work. I thought about this too. The only problem,
this doesn't make the whole code more understandable ;)

> @@ -1587,6 +1594,8 @@ void __perf_event_task_sched_in(struct task_struct *task)
>  	struct perf_event_context *ctx;
>  	int ctxn;
>
> +	__get_cpu_var(task_events_schedulable) = 1;
> +
>  	for_each_task_context_nr(ctxn) {
>  		ctx = task->perf_event_ctxp[ctxn];
>  		if (likely(!ctx))

This doesn't look right. We should set task_events_schedulable
_after_ perf_event_context_sched_in(), otherwise we have the similar
race with next.

rq->curr and current_task were already updated. __perf_install_in_context
should not set "cpuctx->task_ctx = next" before perf_event_context_sched_in(),
it does nothing if cpuctx->task_ctx == ctx.

OTOH, if we set task_events_schedulable after for_each_task_context_nr(),
then we have another race with next, but this race is minor. If
find_get_context() + perf_install_in_context() happen in this window,
the new event won't be scheduled until next reschedules itself.

> +	/*
> +	 * Every pending sched switch must finish so that
> +	 * we ensure every pending calls to perf_event_task_sched_in/out are
> +	 * finished. We ensure the next ones will correctly handle the
> +	 * perf_task_events label and then the task_events_schedulable
> +	 * state. So perf_install_in_context() won't install events
> +	 * in the tiny race window between perf_event_task_sched_out()
> +	 * and perf_event_task_sched_in() in the __ARCH_WANT_INTERRUPTS_ON_CTXSW
> +	 * case.
> +	 */
> +	synchronize_sched();

Yes, if perf_task_events was zero before perf_event_alloc(), then it
is possible that task_events_schedulable == 1 while schedule() is in
progress. perf_event_create_kernel_counter() needs this too.



Frederic, All, can't we simplify this?

First, we modify __perf_install_in_context() so that it never tries
to install the event into !is_active context. IOW, it never tries
to set cpuctx->task_ctx = ctx.

Then we add the new trivial helper stop_resched_task(task) which
simply wakeups the stop thread on task_cpu(task), and thus forces
this task to reschedule.

Now,

	static void
	perf_install_in_context(struct perf_event_context *ctx,
				struct perf_event *event,
				int cpu)
	{
		struct task_struct *task = ctx->task;

		event->ctx = ctx;

		if (!task) {
			/*
			 * Per cpu events are installed via an smp call and
			 * the install is always successful.
			 */
			smp_call_function_single(cpu, __perf_install_in_context,
						 event, 1);
			return;
		}

		for (;;) {
			bool done, need_resched = false;

			raw_spin_lock_irq(&ctx->lock);
			done = !list_empty(&event->group_entry);
			if (!done && !ctx->is_active) {
				add_event_to_ctx(event, ctx);
				need_resched = task_running(task);
				done = true;
			}
			raw_spin_unlock_irq(&ctx->lock);

			if (done) {
				if (need_resched)
					stop_resched_task(task);
				break;
			}

			task_oncpu_function_call(task, __perf_install_in_context,
						event);
		}
	}

Yes, stop_resched_task() can't help if this task itself is the stop thread.
But the stop thread shouldn't run for a long time without rescheduling,
otherwise we already have the problems.

Do you all think this makes any sense?

Oleg.