On Tue, Mar 16, 2021 at 05:22PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 10, 2021 at 11:41:34AM +0100, Marco Elver wrote:
> > Adds bit perf_event_attr::remove_on_exec, to support removing an event
> > from a task on exec.
> >
> > This option supports the case where an event is supposed to be
> > process-wide only, and should not propagate beyond exec, to limit
> > monitoring to the original process image only.
> >
> > Signed-off-by: Marco Elver <el...@google.com>
>
> > +/*
> > + * Removes all events from the current task that have been marked
> > + * remove-on-exec, and feeds their values back to parent events.
> > + */
> > +static void perf_event_remove_on_exec(void)
> > +{
> > + int ctxn;
> > +
> > + for_each_task_context_nr(ctxn) {
> > + struct perf_event_context *ctx;
> > + struct perf_event *event, *next;
> > +
> > + ctx = perf_pin_task_context(current, ctxn);
> > + if (!ctx)
> > + continue;
> > + mutex_lock(&ctx->mutex);
> > +
> > + list_for_each_entry_safe(event, next, &ctx->event_list,
> > event_entry) {
> > + if (!event->attr.remove_on_exec)
> > + continue;
> > +
> > + if (!is_kernel_event(event))
> > + perf_remove_from_owner(event);
> > + perf_remove_from_context(event, DETACH_GROUP);
>
> There's a comment on this in perf_event_exit_event(), if this task
> happens to have the original event, then DETACH_GROUP will destroy the
> grouping.
>
> I think this wants to be:
>
> perf_remove_from_text(event,
> child_event->parent ?
> DETACH_GROUP : 0);
>
> or something.
>
> > + /*
> > + * Remove the event and feed back its values to the
> > + * parent event.
> > + */
> > + perf_event_exit_event(event, ctx, current);
>
> Oooh, and here we call it... but it will do list_del_even() /
> perf_group_detach() *again*.
>
> So the problem is that perf_event_exit_task_context() doesn't use
> remove_from_context(), but instead does task_ctx_sched_out() and then
> relies on the events not being active.
>
> Whereas above you *DO* use remote_from_context(), but then
> perf_event_exit_event() will try and remove it more.
AFAIK, we want to deallocate the events and not just remove them, so
doing what perf_event_exit_event() is the right way forward? Or did you
have something else in mind?
I'm still trying to make sense of the zoo of synchronisation mechanisms
at play here. No matter what I try, it seems I get stuck on the fact
that I can't cleanly "pause" the context to remove the events (warnings
in event_function()).
This is what I've been playing with to understand:
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 450ea9415ed7..c585cef284a0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4195,6 +4195,88 @@ static void perf_event_enable_on_exec(int ctxn)
put_ctx(clone_ctx);
}
+static void perf_remove_from_owner(struct perf_event *event);
+static void perf_event_exit_event(struct perf_event *child_event,
+ struct perf_event_context *child_ctx,
+ struct task_struct *child);
+
+/*
+ * Removes all events from the current task that have been marked
+ * remove-on-exec, and feeds their values back to parent events.
+ */
+static void perf_event_remove_on_exec(void)
+{
+ struct perf_event *event, *next;
+ int ctxn;
+
+ /***************** BROKEN BROKEN BROKEN *****************/
+
+ for_each_task_context_nr(ctxn) {
+ struct perf_event_context *ctx;
+ bool removed = false;
+
+ ctx = perf_pin_task_context(current, ctxn);
+ if (!ctx)
+ continue;
+ mutex_lock(&ctx->mutex);
+
+ raw_spin_lock_irq(&ctx->lock);
+ /*
+ * WIP: Ok, we will unschedule the context, _and_ tell everyone
+ * still trying to use that it's dead... even though it isn't.
+ *
+ * This can't be right...
+ */
+ task_ctx_sched_out(__get_cpu_context(ctx), ctx, EVENT_ALL);
+ RCU_INIT_POINTER(current->perf_event_ctxp[ctxn], NULL);
+ WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
This code here is obviously bogus, because it removes the context from
the task: we might still need it since this task is not dead yet.
What's the right way to pause the context to remove the events from it?
+ raw_spin_unlock_irq(&ctx->lock);
+
+ list_for_each_entry_safe(event, next, &ctx->event_list,
event_entry) {
+ if (!event->attr.remove_on_exec)
+ continue;
+ removed = true;
+
+ if (!is_kernel_event(event))
+ perf_remove_from_owner(event);
+
+ /*
+ * WIP: Want to free the event and feed back its values
+ * to the parent (if any) ...
+ */
+ perf_event_exit_event(event, ctx, current);
+ }
+
... need to schedule context back in here?
+
+ mutex_unlock(&ctx->mutex);
+ perf_unpin_context(ctx);
+ put_ctx(ctx);
+ }
+}
+
struct perf_read_data {
struct perf_event *event;
bool group;
@@ -7553,6 +7635,8 @@ void perf_event_exec(void)
true);
}
rcu_read_unlock();
+
+ perf_event_remove_on_exec();
}
Thanks,
-- Marco
