Re: [RFC v2] perf: Rewrite core context handling

[Ravi Bangoria <ravi.bangoria@amd.com>]

> pulling up the ctx->mutex makes things simpler, but also violates the
> locking order vs exec_update_lock.
> 
> Pull that lock up as well...

I'm not able to apply this patch as is but I get the idea. Few
questions below...

> 
> ---
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12254,13 +12254,29 @@ SYSCALL_DEFINE5(perf_event_open,
>  	if (pmu->task_ctx_nr == perf_sw_context)
>  		event->event_caps |= PERF_EV_CAP_SOFTWARE;
>  
> +	if (task) {
> +		err = down_read_interruptible(&task->signal->exec_update_lock);
> +		if (err)
> +			goto err_alloc;
> +
> +		/*
> +		 * We must hold exec_update_lock across this and any potential
> +		 * perf_install_in_context() call for this new event to
> +		 * serialize against exec() altering our credentials (and the
> +		 * perf_event_exit_task() that could imply).
> +		 */
> +		err = -EACCES;
> +		if (!perf_check_permission(&attr, task))
> +			goto err_cred;
> +	}
> +
>  	/*
>  	 * Get the target context (task or percpu):
>  	 */
>  	ctx = find_get_context(task, event);
>  	if (IS_ERR(ctx)) {
>  		err = PTR_ERR(ctx);
> -		goto err_alloc;
> +		goto err_cred;
>  	}
>  
>  	mutex_lock(&ctx->mutex);
> @@ -12358,58 +12374,14 @@ SYSCALL_DEFINE5(perf_event_open,
>  			goto err_context;
>  	}
>  
> -	event_file = anon_inode_getfile("[perf_event]", &perf_fops, event, f_flags);
> -	if (IS_ERR(event_file)) {
> -		err = PTR_ERR(event_file);
> -		event_file = NULL;
> -		goto err_context;
> -	}
> -
> -	if (task) {
> -		err = down_read_interruptible(&task->signal->exec_update_lock);
> -		if (err)
> -			goto err_file;
> -
> -		/*
> -		 * We must hold exec_update_lock across this and any potential
> -		 * perf_install_in_context() call for this new event to
> -		 * serialize against exec() altering our credentials (and the
> -		 * perf_event_exit_task() that could imply).
> -		 */
> -		err = -EACCES;
> -		if (!perf_check_permission(&attr, task))
> -			goto err_cred;
> -	}
> -
> -	if (ctx->task == TASK_TOMBSTONE) {
> -		err = -ESRCH;
> -		goto err_locked;
> -	}

I think we need to keep (ctx->task == TASK_TOMBSTONE) check?

> -
>  	if (!perf_event_validate_size(event)) {
>  		err = -E2BIG;
> -		goto err_locked;
> -	}
> -
> -	if (!task) {
> -		/*
> -		 * Check if the @cpu we're creating an event for is online.
> -		 *
> -		 * We use the perf_cpu_context::ctx::mutex to serialize against
> -		 * the hotplug notifiers. See perf_event_{init,exit}_cpu().
> -		 */
> -		struct perf_cpu_context *cpuctx =
> -			container_of(ctx, struct perf_cpu_context, ctx);
> -
> -		if (!cpuctx->online) {
> -			err = -ENODEV;
> -			goto err_locked;
> -		}
> +		goto err_context;

Why did you remove this hunk? We should confirm whether cpu is online or not
before creating event. No?

Thanks,
Ravi