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

Ravi Bangoria
2022-08-23T04:57:28Z

On 22-Aug-22 10:22 PM, Peter Zijlstra wrote:
> On Tue, Aug 02, 2022 at 11:41:42AM +0530, Ravi Bangoria wrote:
>
>>> @@ -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?
>
> I think so too; in fact the code I have still has it, perhaps it was
> there write before this patch?
>
>>> -
>>> 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?
>
> Idem.
>
> Perhaps it is best if we look at the end result of all these patches
> combined and then I'll fold the lot if we're in agreement and then we
> can forget about these intermediate steps.

Let me accumulate all these changes, rebase to v6.0-rc2 and send RFC v3.

Thanks,
Ravi