Messages in this thread Patch in this message | | | Date | Mon, 13 Jun 2022 16:38:40 +0200 | From | Peter Zijlstra <> | Subject | Re: [RFC v2] perf: Rewrite core context handling |
| |
On Mon, Jun 13, 2022 at 04:35:11PM +0200, Peter Zijlstra wrote:
Another one of those lockdep splats:
> @@ -12147,42 +12256,37 @@ SYSCALL_DEFINE5(perf_event_open, > if (pmu->task_ctx_nr == perf_sw_context) > event->event_caps |= PERF_EV_CAP_SOFTWARE; > > - if (group_leader) { > - if (is_software_event(event) && > - !in_software_context(group_leader)) { > - /* > - * If the event is a sw event, but the group_leader > - * is on hw context. > - * > - * Allow the addition of software events to hw > - * groups, this is safe because software events > - * never fail to schedule. > - */ > - pmu = group_leader->ctx->pmu; > - } else if (!is_software_event(event) && > - is_software_event(group_leader) && > - (group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) { > - /* > - * In case the group is a pure software group, and we > - * try to add a hardware event, move the whole group to > - * the hardware context. > - */ > - move_group = 1; > - } > - } > - > /* > * Get the target context (task or percpu): > */ > - ctx = find_get_context(pmu, task, event); > + ctx = find_get_context(task, event); > if (IS_ERR(ctx)) { > err = PTR_ERR(ctx); > goto err_alloc; > } > > - /* > - * Look up the group leader (we will attach this event to it): > - */ > + mutex_lock(&ctx->mutex); > + > + if (ctx->task == TASK_TOMBSTONE) { > + err = -ESRCH; > + 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 = per_cpu_ptr(&cpu_context, event->cpu); > + > + if (!cpuctx->online) { > + err = -ENODEV; > + goto err_locked; > + } > + } > + > if (group_leader) { > err = -EINVAL; >
pulling up the ctx->mutex makes things simpler, but also violates the locking order vs exec_update_lock.
Pull that lock up as well...
--- --- 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; - } - 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; } if (perf_need_aux_event(event) && !perf_get_aux_event(event, group_leader)) { err = -EINVAL; - goto err_locked; + goto err_context; } /* @@ -12418,11 +12390,18 @@ SYSCALL_DEFINE5(perf_event_open, */ if (!exclusive_event_installable(event, ctx)) { err = -EBUSY; - goto err_cred; + goto err_context; } WARN_ON_ONCE(ctx->parent_ctx); + 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; + } + /* * This is the point on no return; we cannot fail hereafter. This is * where we start modifying current state. @@ -12500,17 +12479,15 @@ SYSCALL_DEFINE5(perf_event_open, fd_install(event_fd, event_file); return event_fd; -err_cred: - if (task) - up_read(&task->signal->exec_update_lock); -err_file: - fput(event_file); err_context: /* event->pmu_ctx freed by free_event() */ err_locked: mutex_unlock(&ctx->mutex); perf_unpin_context(ctx); put_ctx(ctx); +err_cred: + if (task) + up_read(&task->signal->exec_update_lock); err_alloc: /* * If event_file is set, the fput() above will have called ->release()
| |