| Message ID | 20210310104139.679618-9-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for synchronous signals on perf events | expand |
On Wed, Mar 10, 2021 at 11:41AM +0100, Marco Elver wrote: > Add kselftest to test that remove_on_exec removes inherited events from > child tasks. > > Signed-off-by: Marco Elver <elver@google.com> To make compatible with more recent libc, we'll need to fixup the tests with the below. Also, I've seen that tools/perf/tests exists, however it seems to be primarily about perf-tool related tests. Is this correct? I'd propose to keep these purely kernel ABI related tests separate, and that way we can also make use of the kselftests framework which will also integrate into various CI systems such as kernelci.org. Thanks, -- Marco ------ >8 ------ diff --git a/tools/testing/selftests/perf_events/remove_on_exec.c b/tools/testing/selftests/perf_events/remove_on_exec.c index e176b3a74d55..f89d0cfdb81e 100644 --- a/tools/testing/selftests/perf_events/remove_on_exec.c +++ b/tools/testing/selftests/perf_events/remove_on_exec.c @@ -13,6 +13,11 @@ #define __have_siginfo_t 1 #define __have_sigval_t 1 #define __have_sigevent_t 1 +#define __siginfo_t_defined +#define __sigval_t_defined +#define __sigevent_t_defined +#define _BITS_SIGINFO_CONSTS_H 1 +#define _BITS_SIGEVENT_CONSTS_H 1 #include <linux/perf_event.h> #include <pthread.h> diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c b/tools/testing/selftests/perf_events/sigtrap_threads.c index 7ebb9bb34c2e..b9a7d4b64b3c 100644 --- a/tools/testing/selftests/perf_events/sigtrap_threads.c +++ b/tools/testing/selftests/perf_events/sigtrap_threads.c @@ -13,6 +13,11 @@ #define __have_siginfo_t 1 #define __have_sigval_t 1 #define __have_sigevent_t 1 +#define __siginfo_t_defined +#define __sigval_t_defined +#define __sigevent_t_defined +#define _BITS_SIGINFO_CONSTS_H 1 +#define _BITS_SIGEVENT_CONSTS_H 1 #include <linux/hw_breakpoint.h> #include <linux/perf_event.h>
On Mon, Mar 22, 2021 at 02:24:40PM +0100, Marco Elver wrote: > To make compatible with more recent libc, we'll need to fixup the tests > with the below. OK, that reprodiced things here, thanks! The below seems to not explode instantly.... it still has the alternative version in as well (and I think it might even work too, but the one I left in seems simpler). --- kernel/events/core.c | 154 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 111 insertions(+), 43 deletions(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index a7220e8c447e..8c0f905cc017 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -2167,8 +2172,9 @@ static void perf_group_detach(struct perf_event *event) * If this is a sibling, remove it from its group. */ if (leader != event) { + leader->nr_siblings--; list_del_init(&event->sibling_list); - event->group_leader->nr_siblings--; + event->group_leader = event; goto out; } @@ -2182,8 +2188,9 @@ static void perf_group_detach(struct perf_event *event) if (sibling->event_caps & PERF_EV_CAP_SIBLING) perf_remove_sibling_event(sibling); - sibling->group_leader = sibling; + leader->nr_siblings--; list_del_init(&sibling->sibling_list); + sibling->group_leader = sibling; /* Inherit group flags from the previous leader */ sibling->group_caps = event->group_caps; @@ -2360,10 +2367,19 @@ __perf_remove_from_context(struct perf_event *event, static void perf_remove_from_context(struct perf_event *event, unsigned long flags) { struct perf_event_context *ctx = event->ctx; + bool remove; lockdep_assert_held(&ctx->mutex); - event_function_call(event, __perf_remove_from_context, (void *)flags); + /* + * There is concurrency vs remove_on_exec(). + */ + raw_spin_lock_irq(&ctx->lock); + remove = (event->attach_state & PERF_ATTACH_CONTEXT); + raw_spin_unlock_irq(&ctx->lock); + + if (remove) + event_function_call(event, __perf_remove_from_context, (void *)flags); /* * The above event_function_call() can NO-OP when it hits @@ -4232,41 +4248,92 @@ static void perf_event_enable_on_exec(int ctxn) 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); + struct task_struct *child, + bool removed); /* * 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) +static void perf_event_remove_on_exec(int ctxn) { - int ctxn; + struct perf_event_context *ctx, *clone_ctx = NULL; + struct perf_event *event, *next; + LIST_HEAD(free_list); + unsigned long flags; + bool modified = false; - 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) + return; - ctx = perf_pin_task_context(current, ctxn); - if (!ctx) + mutex_lock(&ctx->mutex); + + if (WARN_ON_ONCE(ctx->task != current)) + goto unlock; + + list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) { + if (!event->attr.remove_on_exec) 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); - if (!is_kernel_event(event)) - perf_remove_from_owner(event); - perf_remove_from_context(event, DETACH_GROUP); - /* - * Remove the event and feed back its values to the - * parent event. - */ - perf_event_exit_event(event, ctx, current); - } - mutex_unlock(&ctx->mutex); - put_ctx(ctx); + modified = true; + + perf_remove_from_context(event, !!event->parent * DETACH_GROUP); + perf_event_exit_event(event, ctx, current, true); + } + + raw_spin_lock_irqsave(&ctx->lock, flags); + if (modified) + clone_ctx = unclone_ctx(ctx); + --ctx->pin_count; + raw_spin_unlock_irqrestore(&ctx->lock, flags); + +#if 0 + struct perf_cpu_context *cpuctx; + + if (!modified) { + perf_unpin_context(ctx); + goto unlock; + } + + local_irq_save(flags); + cpuctx = __get_cpu_context(ctx); + perf_ctx_lock(cpuctx, ctx); + task_ctx_sched_out(cpuctx, ctx, EVENT_ALL); + + list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) { + if (!event->attr.remove_on_exec) + continue; + + if (event->parent) + perf_group_detach(event); + list_del_event(event, ctx); + + list_add(&event->active_list, &free_list); + } + + ctx_resched(cpuctx, ctx, EVENT_ALL); + + clone_ctx = unclone_ctx(ctx); + --ctx->pin_count; + perf_ctx_unlock(cpuctx, ctx); + local_irq_restore(flags); + + list_for_each_entry_safe(event, next, &free_list, active_entry) { + list_del(&event->active_entry); + perf_event_exit_event(event, ctx, current, true); } +#endif + +unlock: + mutex_unlock(&ctx->mutex); + + put_ctx(ctx); + if (clone_ctx) + put_ctx(clone_ctx); } struct perf_read_data { @@ -7615,20 +7682,18 @@ void perf_event_exec(void) struct perf_event_context *ctx; int ctxn; - rcu_read_lock(); for_each_task_context_nr(ctxn) { - ctx = current->perf_event_ctxp[ctxn]; - if (!ctx) - continue; - perf_event_enable_on_exec(ctxn); + perf_event_remove_on_exec(ctxn); - perf_iterate_ctx(ctx, perf_event_addr_filters_exec, NULL, - true); + rcu_read_lock(); + ctx = rcu_dereference(current->perf_event_ctxp[ctxn]); + if (ctx) { + perf_iterate_ctx(ctx, perf_event_addr_filters_exec, + NULL, true); + } + rcu_read_unlock(); } - rcu_read_unlock(); - - perf_event_remove_on_exec(); } struct remote_output { @@ -12509,7 +12574,7 @@ static void sync_child_event(struct perf_event *child_event, static void perf_event_exit_event(struct perf_event *child_event, struct perf_event_context *child_ctx, - struct task_struct *child) + struct task_struct *child, bool removed) { struct perf_event *parent_event = child_event->parent; @@ -12526,12 +12591,15 @@ perf_event_exit_event(struct perf_event *child_event, * and being thorough is better. */ raw_spin_lock_irq(&child_ctx->lock); - WARN_ON_ONCE(child_ctx->is_active); + if (!removed) { + WARN_ON_ONCE(child_ctx->is_active); - if (parent_event) - perf_group_detach(child_event); - list_del_event(child_event, child_ctx); - perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */ + if (parent_event) + perf_group_detach(child_event); + list_del_event(child_event, child_ctx); + } + if (child_event->state >= PERF_EVENT_STATE_EXIT) + perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */ raw_spin_unlock_irq(&child_ctx->lock); /* @@ -12617,7 +12685,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn) perf_event_task(child, child_ctx, 0); list_for_each_entry_safe(child_event, next, &child_ctx->event_list, event_entry) - perf_event_exit_event(child_event, child_ctx, child); + perf_event_exit_event(child_event, child_ctx, child, false); mutex_unlock(&child_ctx->mutex);
On Mon, Mar 22, 2021 at 6:24 AM Marco Elver <elver@google.com> wrote: > > On Wed, Mar 10, 2021 at 11:41AM +0100, Marco Elver wrote: > > Add kselftest to test that remove_on_exec removes inherited events from > > child tasks. > > > > Signed-off-by: Marco Elver <elver@google.com> > > To make compatible with more recent libc, we'll need to fixup the tests > with the below. > > Also, I've seen that tools/perf/tests exists, however it seems to be > primarily about perf-tool related tests. Is this correct? > > I'd propose to keep these purely kernel ABI related tests separate, and > that way we can also make use of the kselftests framework which will > also integrate into various CI systems such as kernelci.org. Perhaps there is a way to have both? Having the perf tool spot an errant kernel feels like a feature. There are also tools/lib/perf/tests and Vince Weaver's tests [1]. It is possible to run standalone tests from within perf test by having them be executed by a shell test. Thanks, Ian [1] https://github.com/deater/perf_event_tests > Thanks, > -- Marco > > ------ >8 ------ > > diff --git a/tools/testing/selftests/perf_events/remove_on_exec.c b/tools/testing/selftests/perf_events/remove_on_exec.c > index e176b3a74d55..f89d0cfdb81e 100644 > --- a/tools/testing/selftests/perf_events/remove_on_exec.c > +++ b/tools/testing/selftests/perf_events/remove_on_exec.c > @@ -13,6 +13,11 @@ > #define __have_siginfo_t 1 > #define __have_sigval_t 1 > #define __have_sigevent_t 1 > +#define __siginfo_t_defined > +#define __sigval_t_defined > +#define __sigevent_t_defined > +#define _BITS_SIGINFO_CONSTS_H 1 > +#define _BITS_SIGEVENT_CONSTS_H 1 > > #include <linux/perf_event.h> > #include <pthread.h> > diff --git a/tools/testing/selftests/perf_events/sigtrap_threads.c b/tools/testing/selftests/perf_events/sigtrap_threads.c > index 7ebb9bb34c2e..b9a7d4b64b3c 100644 > --- a/tools/testing/selftests/perf_events/sigtrap_threads.c > +++ b/tools/testing/selftests/perf_events/sigtrap_threads.c > @@ -13,6 +13,11 @@ > #define __have_siginfo_t 1 > #define __have_sigval_t 1 > #define __have_sigevent_t 1 > +#define __siginfo_t_defined > +#define __sigval_t_defined > +#define __sigevent_t_defined > +#define _BITS_SIGINFO_CONSTS_H 1 > +#define _BITS_SIGEVENT_CONSTS_H 1 > > #include <linux/hw_breakpoint.h> > #include <linux/perf_event.h>
On Tue, 23 Mar 2021 at 04:10, Ian Rogers <irogers@google.com> wrote: > On Mon, Mar 22, 2021 at 6:24 AM Marco Elver <elver@google.com> wrote: > > On Wed, Mar 10, 2021 at 11:41AM +0100, Marco Elver wrote: > > > Add kselftest to test that remove_on_exec removes inherited events from > > > child tasks. > > > > > > Signed-off-by: Marco Elver <elver@google.com> > > > > To make compatible with more recent libc, we'll need to fixup the tests > > with the below. > > > > Also, I've seen that tools/perf/tests exists, however it seems to be > > primarily about perf-tool related tests. Is this correct? > > > > I'd propose to keep these purely kernel ABI related tests separate, and > > that way we can also make use of the kselftests framework which will > > also integrate into various CI systems such as kernelci.org. > > Perhaps there is a way to have both? Having the perf tool spot an > errant kernel feels like a feature. There are also > tools/lib/perf/tests and Vince Weaver's tests [1]. It is possible to > run standalone tests from within perf test by having them be executed > by a shell test. Thanks for the pointers. Sure, I'd support more additional tests. But I had another look and it seems the tests in tools/{perf,lib/perf}/tests do focus on perf-tool or the library respectively, so adding kernel ABI tests there feels wrong. (If perf-tool somehow finds use for sigtrap, or remove_on_exec, then having a perf-tool specific test for those would make sense again.) The tests at [1] do seem relevant, and its test strategy seems more extensive, including testing older kernels. Unfortunately it is out-of-tree, but that's probably because it was started before kselftest came into existence. But there are probably things that [1] contains that are not appropriate in-tree. It's all a bit confusing. Going forward, if you insist on tests being also added to [1], we can perhaps mirror some of the kselftest tests there. There's also a logistical problem with the tests added here, because the tests require an up-to-date siginfo_t, and they use the kernel's <asm/siginfo.h> with some trickery. Until libc's siginfo_t is updated, it probably doesn't make sense to add these tests to [1]. The other question is, would it be possible to also copy some of the tests in [1] and convert to kselftest, so that they live in-tree and are tested regularly (CI, ...)? Because I'd much prefer in-tree tests with little boilerplate, that are structured with parsable output; in the kernel we have the kselftest framework for tests with a user space component, and KUnit for pure in-kernel tests. Thanks, -- Marco > Thanks, > Ian > > [1] https://github.com/deater/perf_event_tests [...]
On Mon, Mar 22, 2021 at 05:42PM +0100, Peter Zijlstra wrote: > On Mon, Mar 22, 2021 at 02:24:40PM +0100, Marco Elver wrote: > > To make compatible with more recent libc, we'll need to fixup the tests > > with the below. > > OK, that reprodiced things here, thanks! > > The below seems to not explode instantly.... it still has the > alternative version in as well (and I think it might even work too, but > the one I left in seems simpler). Thanks! Unfortunately neither version worked if I tortured it a little with this: for x in {1..1000}; do ( tools/testing/selftests/perf_events/remove_on_exec & ); done Which resulted in the 2 warnings: WARNING: CPU: 1 PID: 795 at kernel/events/core.c:242 event_function+0xf3/0x100 WARNING: CPU: 1 PID: 795 at kernel/events/core.c:247 event_function+0xef/0x100 with efs->func==__perf_event_enable. I believe it's sufficient to add mutex_lock(&parent_event->child_mutex); list_del_init(&event->child_list); mutex_unlock(&parent_event->child_mutex); right before removing from context. With the version I have now (below for completeness), extended torture with the above test results in no more warnings and the test also passes. I'd be happy to send a non-RFC v3 with all that squashed in. I'd need your Signed-off-by for the diff you sent to proceed (and add your Co-developed-by). Thanks, -- Marco ------ >8 ------ diff --git a/kernel/events/core.c b/kernel/events/core.c index aa47e111435e..cea7c88fe131 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -2165,8 +2165,9 @@ static void perf_group_detach(struct perf_event *event) * If this is a sibling, remove it from its group. */ if (leader != event) { + leader->nr_siblings--; list_del_init(&event->sibling_list); - event->group_leader->nr_siblings--; + event->group_leader = event; goto out; } @@ -2180,8 +2181,9 @@ static void perf_group_detach(struct perf_event *event) if (sibling->event_caps & PERF_EV_CAP_SIBLING) perf_remove_sibling_event(sibling); - sibling->group_leader = sibling; + leader->nr_siblings--; list_del_init(&sibling->sibling_list); + sibling->group_leader = sibling; /* Inherit group flags from the previous leader */ sibling->group_caps = event->group_caps; @@ -2358,10 +2360,19 @@ __perf_remove_from_context(struct perf_event *event, static void perf_remove_from_context(struct perf_event *event, unsigned long flags) { struct perf_event_context *ctx = event->ctx; + bool remove; lockdep_assert_held(&ctx->mutex); - event_function_call(event, __perf_remove_from_context, (void *)flags); + /* + * There is concurrency vs remove_on_exec(). + */ + raw_spin_lock_irq(&ctx->lock); + remove = (event->attach_state & PERF_ATTACH_CONTEXT); + raw_spin_unlock_irq(&ctx->lock); + + if (remove) + event_function_call(event, __perf_remove_from_context, (void *)flags); /* * The above event_function_call() can NO-OP when it hits @@ -4198,41 +4209,68 @@ static void perf_event_enable_on_exec(int ctxn) 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); + struct task_struct *child, + bool removed); /* * 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) +static void perf_event_remove_on_exec(int ctxn) { - int ctxn; + struct perf_event_context *ctx, *clone_ctx = NULL; + struct perf_event *event, *next; + LIST_HEAD(free_list); + unsigned long flags; + bool modified = false; - 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) + return; - ctx = perf_pin_task_context(current, ctxn); - if (!ctx) + mutex_lock(&ctx->mutex); + + if (WARN_ON_ONCE(ctx->task != current)) + goto unlock; + + list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) { + struct perf_event *parent_event = event->parent; + + if (!event->attr.remove_on_exec) 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); - if (!is_kernel_event(event)) - perf_remove_from_owner(event); - perf_remove_from_context(event, DETACH_GROUP); + modified = true; + + if (parent_event) { /* - * Remove the event and feed back its values to the - * parent event. + * Remove event from parent, to avoid race where the + * parent concurrently iterates through its children to + * enable, disable, or otherwise modify an event. */ - perf_event_exit_event(event, ctx, current); + mutex_lock(&parent_event->child_mutex); + list_del_init(&event->child_list); + mutex_unlock(&parent_event->child_mutex); } - mutex_unlock(&ctx->mutex); - put_ctx(ctx); + + perf_remove_from_context(event, !!event->parent * DETACH_GROUP); + perf_event_exit_event(event, ctx, current, true); } + + raw_spin_lock_irqsave(&ctx->lock, flags); + if (modified) + clone_ctx = unclone_ctx(ctx); + --ctx->pin_count; + raw_spin_unlock_irqrestore(&ctx->lock, flags); + +unlock: + mutex_unlock(&ctx->mutex); + + put_ctx(ctx); + if (clone_ctx) + put_ctx(clone_ctx); } struct perf_read_data { @@ -7581,20 +7619,18 @@ void perf_event_exec(void) struct perf_event_context *ctx; int ctxn; - rcu_read_lock(); for_each_task_context_nr(ctxn) { - ctx = current->perf_event_ctxp[ctxn]; - if (!ctx) - continue; - perf_event_enable_on_exec(ctxn); + perf_event_remove_on_exec(ctxn); - perf_iterate_ctx(ctx, perf_event_addr_filters_exec, NULL, - true); + rcu_read_lock(); + ctx = rcu_dereference(current->perf_event_ctxp[ctxn]); + if (ctx) { + perf_iterate_ctx(ctx, perf_event_addr_filters_exec, + NULL, true); + } + rcu_read_unlock(); } - rcu_read_unlock(); - - perf_event_remove_on_exec(); } struct remote_output { @@ -12472,7 +12508,7 @@ static void sync_child_event(struct perf_event *child_event, static void perf_event_exit_event(struct perf_event *child_event, struct perf_event_context *child_ctx, - struct task_struct *child) + struct task_struct *child, bool removed) { struct perf_event *parent_event = child_event->parent; @@ -12489,12 +12525,15 @@ perf_event_exit_event(struct perf_event *child_event, * and being thorough is better. */ raw_spin_lock_irq(&child_ctx->lock); - WARN_ON_ONCE(child_ctx->is_active); + if (!removed) { + WARN_ON_ONCE(child_ctx->is_active); - if (parent_event) - perf_group_detach(child_event); - list_del_event(child_event, child_ctx); - perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */ + if (parent_event) + perf_group_detach(child_event); + list_del_event(child_event, child_ctx); + } + if (child_event->state >= PERF_EVENT_STATE_EXIT) + perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */ raw_spin_unlock_irq(&child_ctx->lock); /* @@ -12580,7 +12619,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn) perf_event_task(child, child_ctx, 0); list_for_each_entry_safe(child_event, next, &child_ctx->event_list, event_entry) - perf_event_exit_event(child_event, child_ctx, child); + perf_event_exit_event(child_event, child_ctx, child, false); mutex_unlock(&child_ctx->mutex);
On Tue, Mar 23, 2021 at 10:52:41AM +0100, Marco Elver wrote: > with efs->func==__perf_event_enable. I believe it's sufficient to add > > mutex_lock(&parent_event->child_mutex); > list_del_init(&event->child_list); > mutex_unlock(&parent_event->child_mutex); > > right before removing from context. With the version I have now (below > for completeness), extended torture with the above test results in no > more warnings and the test also passes. > > + list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) { > + struct perf_event *parent_event = event->parent; > + > + if (!event->attr.remove_on_exec) > continue; > > + if (!is_kernel_event(event)) > + perf_remove_from_owner(event); > > + modified = true; > + > + if (parent_event) { > /* > + * Remove event from parent, to avoid race where the > + * parent concurrently iterates through its children to > + * enable, disable, or otherwise modify an event. > */ > + mutex_lock(&parent_event->child_mutex); > + list_del_init(&event->child_list); > + mutex_unlock(&parent_event->child_mutex); > } ^^^ this, right? But that's something perf_event_exit_event() alread does. So then you're worried about the order of things. > + > + perf_remove_from_context(event, !!event->parent * DETACH_GROUP); > + perf_event_exit_event(event, ctx, current, true); > } perf_event_release_kernel() first does perf_remove_from_context() and then clears the child_list, and that makes sense because if we're there, there's no external access anymore, the filedesc is gone and nobody will be iterating child_list anymore. perf_event_exit_task_context() and perf_event_exit_event() OTOH seem to rely on ctx->task == TOMBSTONE to sabotage event_function_call() such that if anybody is iterating the child_list, it'll NOP out. But here we don't have neither, and thus need to worry about the order vs child_list iteration. I suppose we should stick sync_child_event() in there as well. And at that point there's very little value in still using perf_event_exit_event()... let me see if there's something to be done about that.
On Tue, 23 Mar 2021 at 11:32, Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Mar 23, 2021 at 10:52:41AM +0100, Marco Elver wrote: > > > with efs->func==__perf_event_enable. I believe it's sufficient to add > > > > mutex_lock(&parent_event->child_mutex); > > list_del_init(&event->child_list); > > mutex_unlock(&parent_event->child_mutex); > > > > right before removing from context. With the version I have now (below > > for completeness), extended torture with the above test results in no > > more warnings and the test also passes. > > > > > + list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) { > > + struct perf_event *parent_event = event->parent; > > + > > + if (!event->attr.remove_on_exec) > > continue; > > > > + if (!is_kernel_event(event)) > > + perf_remove_from_owner(event); > > > > + modified = true; > > + > > + if (parent_event) { > > /* > > + * Remove event from parent, to avoid race where the > > + * parent concurrently iterates through its children to > > + * enable, disable, or otherwise modify an event. > > */ > > + mutex_lock(&parent_event->child_mutex); > > + list_del_init(&event->child_list); > > + mutex_unlock(&parent_event->child_mutex); > > } > > ^^^ this, right? > > But that's something perf_event_exit_event() alread does. So then you're > worried about the order of things. Correct. We somehow need to prohibit the parent from doing an event_function_call() while we potentially deactivate the context with perf_remove_from_context(). > > + > > + perf_remove_from_context(event, !!event->parent * DETACH_GROUP); > > + perf_event_exit_event(event, ctx, current, true); > > } > > perf_event_release_kernel() first does perf_remove_from_context() and > then clears the child_list, and that makes sense because if we're there, > there's no external access anymore, the filedesc is gone and nobody will > be iterating child_list anymore. > > perf_event_exit_task_context() and perf_event_exit_event() OTOH seem to > rely on ctx->task == TOMBSTONE to sabotage event_function_call() such > that if anybody is iterating the child_list, it'll NOP out. > > But here we don't have neither, and thus need to worry about the order > vs child_list iteration. > > I suppose we should stick sync_child_event() in there as well. > > And at that point there's very little value in still using > perf_event_exit_event()... let me see if there's something to be done > about that. I don't mind dropping use of perf_event_exit_event() and open coding all of this. That would also avoid modifying perf_event_exit_event(). But I leave it to you what you think is nicest. Thanks, -- Marco
On Tue, Mar 23, 2021 at 11:41AM +0100, Marco Elver wrote: > On Tue, 23 Mar 2021 at 11:32, Peter Zijlstra <peterz@infradead.org> wrote: [...] > > > + if (parent_event) { > > > /* > > > + * Remove event from parent, to avoid race where the > > > + * parent concurrently iterates through its children to > > > + * enable, disable, or otherwise modify an event. > > > */ > > > + mutex_lock(&parent_event->child_mutex); > > > + list_del_init(&event->child_list); > > > + mutex_unlock(&parent_event->child_mutex); > > > } > > > > ^^^ this, right? > > > > But that's something perf_event_exit_event() alread does. So then you're > > worried about the order of things. > > Correct. We somehow need to prohibit the parent from doing an > event_function_call() while we potentially deactivate the context with > perf_remove_from_context(). > > > > + > > > + perf_remove_from_context(event, !!event->parent * DETACH_GROUP); > > > + perf_event_exit_event(event, ctx, current, true); > > > } > > > > perf_event_release_kernel() first does perf_remove_from_context() and > > then clears the child_list, and that makes sense because if we're there, > > there's no external access anymore, the filedesc is gone and nobody will > > be iterating child_list anymore. > > > > perf_event_exit_task_context() and perf_event_exit_event() OTOH seem to > > rely on ctx->task == TOMBSTONE to sabotage event_function_call() such > > that if anybody is iterating the child_list, it'll NOP out. > > > > But here we don't have neither, and thus need to worry about the order > > vs child_list iteration. > > > > I suppose we should stick sync_child_event() in there as well. > > > > And at that point there's very little value in still using > > perf_event_exit_event()... let me see if there's something to be done > > about that. > > I don't mind dropping use of perf_event_exit_event() and open coding > all of this. That would also avoid modifying perf_event_exit_event(). > > But I leave it to you what you think is nicest. I played a bit more with it, and the below would be the version without using perf_event_exit_event(). Perhaps it isn't too bad. Thanks, -- Marco ------ >8 ------ diff --git a/kernel/events/core.c b/kernel/events/core.c index aa47e111435e..288b61820dab 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -2165,8 +2165,9 @@ static void perf_group_detach(struct perf_event *event) * If this is a sibling, remove it from its group. */ if (leader != event) { + leader->nr_siblings--; list_del_init(&event->sibling_list); - event->group_leader->nr_siblings--; + event->group_leader = event; goto out; } @@ -2180,8 +2181,9 @@ static void perf_group_detach(struct perf_event *event) if (sibling->event_caps & PERF_EV_CAP_SIBLING) perf_remove_sibling_event(sibling); - sibling->group_leader = sibling; + leader->nr_siblings--; list_del_init(&sibling->sibling_list); + sibling->group_leader = sibling; /* Inherit group flags from the previous leader */ sibling->group_caps = event->group_caps; @@ -2358,10 +2360,19 @@ __perf_remove_from_context(struct perf_event *event, static void perf_remove_from_context(struct perf_event *event, unsigned long flags) { struct perf_event_context *ctx = event->ctx; + bool remove; lockdep_assert_held(&ctx->mutex); - event_function_call(event, __perf_remove_from_context, (void *)flags); + /* + * There is concurrency vs remove_on_exec(). + */ + raw_spin_lock_irq(&ctx->lock); + remove = (event->attach_state & PERF_ATTACH_CONTEXT); + raw_spin_unlock_irq(&ctx->lock); + + if (remove) + event_function_call(event, __perf_remove_from_context, (void *)flags); /* * The above event_function_call() can NO-OP when it hits @@ -4196,43 +4207,86 @@ static void perf_event_enable_on_exec(int ctxn) } 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); +static void sync_child_event(struct perf_event *child_event, + struct task_struct *child); +static void free_event(struct perf_event *event); /* * 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) +static void perf_event_remove_on_exec(int ctxn) { - int ctxn; + struct perf_event_context *ctx, *clone_ctx = NULL; + struct perf_event *event, *next; + LIST_HEAD(free_list); + unsigned long flags; + bool modified = false; - 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) + return; - ctx = perf_pin_task_context(current, ctxn); - if (!ctx) + mutex_lock(&ctx->mutex); + + if (WARN_ON_ONCE(ctx->task != current)) + goto unlock; + + list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) { + struct perf_event *parent_event = event->parent; + + if (!event->attr.remove_on_exec) 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); + + modified = true; - if (!is_kernel_event(event)) - perf_remove_from_owner(event); - perf_remove_from_context(event, DETACH_GROUP); + if (parent_event) { /* - * Remove the event and feed back its values to the - * parent event. + * Remove event from parent *before* modifying contexts, + * to avoid race where the parent concurrently iterates + * through its children to enable, disable, or otherwise + * modify an event. */ - perf_event_exit_event(event, ctx, current); + + sync_child_event(event, current); + + WARN_ON_ONCE(parent_event->ctx->parent_ctx); + mutex_lock(&parent_event->child_mutex); + list_del_init(&event->child_list); + mutex_unlock(&parent_event->child_mutex); + + perf_event_wakeup(parent_event); + put_event(parent_event); } - mutex_unlock(&ctx->mutex); - put_ctx(ctx); + + perf_remove_from_context(event, !!event->parent * DETACH_GROUP); + + raw_spin_lock_irq(&ctx->lock); + WARN_ON_ONCE(ctx->is_active); + perf_event_set_state(event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */ + raw_spin_unlock_irq(&ctx->lock); + + if (parent_event) + free_event(event); + else + perf_event_wakeup(event); } + + raw_spin_lock_irqsave(&ctx->lock, flags); + if (modified) + clone_ctx = unclone_ctx(ctx); + --ctx->pin_count; + raw_spin_unlock_irqrestore(&ctx->lock, flags); + +unlock: + mutex_unlock(&ctx->mutex); + + put_ctx(ctx); + if (clone_ctx) + put_ctx(clone_ctx); } struct perf_read_data { @@ -7581,20 +7635,18 @@ void perf_event_exec(void) struct perf_event_context *ctx; int ctxn; - rcu_read_lock(); for_each_task_context_nr(ctxn) { - ctx = current->perf_event_ctxp[ctxn]; - if (!ctx) - continue; - perf_event_enable_on_exec(ctxn); + perf_event_remove_on_exec(ctxn); - perf_iterate_ctx(ctx, perf_event_addr_filters_exec, NULL, - true); + rcu_read_lock(); + ctx = rcu_dereference(current->perf_event_ctxp[ctxn]); + if (ctx) { + perf_iterate_ctx(ctx, perf_event_addr_filters_exec, + NULL, true); + } + rcu_read_unlock(); } - rcu_read_unlock(); - - perf_event_remove_on_exec(); } struct remote_output {
On Tue, Mar 23, 2021 at 11:32:03AM +0100, Peter Zijlstra wrote: > And at that point there's very little value in still using > perf_event_exit_event()... let me see if there's something to be done > about that. I ended up with something like the below. Which then simplifies remove_on_exec() to: static void perf_event_remove_on_exec(int ctxn) { struct perf_event_context *ctx, *clone_ctx = NULL; struct perf_event *event, *next; bool modified = false; unsigned long flags; ctx = perf_pin_task_context(current, ctxn); if (!ctx) return; mutex_lock(&ctx->mutex); if (WARN_ON_ONCE(ctx->task != current)) goto unlock; 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); modified = true; perf_event_exit_event(event, ctx); } raw_spin_lock_irqsave(&ctx->lock, flags); if (modified) clone_ctx = unclone_ctx(ctx); --ctx->pin_count; raw_spin_unlock_irqrestore(&ctx->lock, flags); unlock: mutex_unlock(&ctx->mutex); put_ctx(ctx); if (clone_ctx) put_ctx(clone_ctx); } Very lightly tested with that {1..1000} thing. --- Subject: perf: Rework perf_event_exit_event() From: Peter Zijlstra <peterz@infradead.org> Date: Tue Mar 23 15:16:06 CET 2021 Make perf_event_exit_event() more robust, such that we can use it from other contexts. Specifically the up and coming remove_on_exec. For this to work we need to address a few issues. Remove_on_exec will not destroy the entire context, so we cannot rely on TASK_TOMBSTONE to disable event_function_call() and we thus have to use perf_remove_from_context(). When using perf_remove_from_context(), there's two races to consider. The first is against close(), where we can have concurrent tear-down of the event. The second is against child_list iteration, which should not find a half baked event. To address this, teach perf_remove_from_context() to special case !ctx->is_active and about DETACH_CHILD. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- include/linux/perf_event.h | 1 kernel/events/core.c | 144 +++++++++++++++++++++++++-------------------- 2 files changed, 81 insertions(+), 64 deletions(-) --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -607,6 +607,7 @@ struct swevent_hlist { #define PERF_ATTACH_TASK_DATA 0x08 #define PERF_ATTACH_ITRACE 0x10 #define PERF_ATTACH_SCHED_CB 0x20 +#define PERF_ATTACH_CHILD 0x40 struct perf_cgroup; struct perf_buffer; --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -2210,6 +2210,26 @@ static void perf_group_detach(struct per perf_event__header_size(leader); } +static void sync_child_event(struct perf_event *child_event); + +static void perf_child_detach(struct perf_event *event) +{ + struct perf_event *parent_event = event->parent; + + if (!(event->attach_state & PERF_ATTACH_CHILD)) + return; + + event->attach_state &= ~PERF_ATTACH_CHILD; + + if (WARN_ON_ONCE(!parent_event)) + return; + + lockdep_assert_held(&parent_event->child_mutex); + + sync_child_event(event); + list_del_init(&event->child_list); +} + static bool is_orphaned_event(struct perf_event *event) { return event->state == PERF_EVENT_STATE_DEAD; @@ -2317,6 +2337,7 @@ group_sched_out(struct perf_event *group } #define DETACH_GROUP 0x01UL +#define DETACH_CHILD 0x02UL /* * Cross CPU call to remove a performance event @@ -2340,6 +2361,8 @@ __perf_remove_from_context(struct perf_e event_sched_out(event, cpuctx, ctx); if (flags & DETACH_GROUP) perf_group_detach(event); + if (flags & DETACH_CHILD) + perf_child_detach(event); list_del_event(event, ctx); if (!ctx->nr_events && ctx->is_active) { @@ -2368,25 +2391,21 @@ static void perf_remove_from_context(str lockdep_assert_held(&ctx->mutex); - event_function_call(event, __perf_remove_from_context, (void *)flags); - /* - * The above event_function_call() can NO-OP when it hits - * TASK_TOMBSTONE. In that case we must already have been detached - * from the context (by perf_event_exit_event()) but the grouping - * might still be in-tact. - */ - WARN_ON_ONCE(event->attach_state & PERF_ATTACH_CONTEXT); - if ((flags & DETACH_GROUP) && - (event->attach_state & PERF_ATTACH_GROUP)) { - /* - * Since in that case we cannot possibly be scheduled, simply - * detach now. - */ - raw_spin_lock_irq(&ctx->lock); - perf_group_detach(event); + * Because of perf_event_exit_task(), perf_remove_from_context() ought + * to work in the face of TASK_TOMBSTONE, unlike every other + * event_function_call() user. + */ + raw_spin_lock_irq(&ctx->lock); + if (!ctx->is_active) { + __perf_remove_from_context(event, __get_cpu_context(ctx), + ctx, (void *)flags); raw_spin_unlock_irq(&ctx->lock); + return; } + raw_spin_unlock_irq(&ctx->lock); + + event_function_call(event, __perf_remove_from_context, (void *)flags); } /* @@ -12379,14 +12398,17 @@ void perf_pmu_migrate_context(struct pmu } EXPORT_SYMBOL_GPL(perf_pmu_migrate_context); -static void sync_child_event(struct perf_event *child_event, - struct task_struct *child) +static void sync_child_event(struct perf_event *child_event) { struct perf_event *parent_event = child_event->parent; u64 child_val; - if (child_event->attr.inherit_stat) - perf_event_read_event(child_event, child); + if (child_event->attr.inherit_stat) { + struct task_struct *task = child_event->ctx->task; + + if (task) + perf_event_read_event(child_event, task); + } child_val = perf_event_count(child_event); @@ -12401,60 +12423,53 @@ static void sync_child_event(struct perf } static void -perf_event_exit_event(struct perf_event *child_event, - struct perf_event_context *child_ctx, - struct task_struct *child) +perf_event_exit_event(struct perf_event *event, struct perf_event_context *ctx) { - struct perf_event *parent_event = child_event->parent; + struct perf_event *parent_event = event->parent; + unsigned long detach_flags = 0; - /* - * Do not destroy the 'original' grouping; because of the context - * switch optimization the original events could've ended up in a - * random child task. - * - * If we were to destroy the original group, all group related - * operations would cease to function properly after this random - * child dies. - * - * Do destroy all inherited groups, we don't care about those - * and being thorough is better. - */ - raw_spin_lock_irq(&child_ctx->lock); - WARN_ON_ONCE(child_ctx->is_active); + if (parent_event) { + /* + * Do not destroy the 'original' grouping; because of the + * context switch optimization the original events could've + * ended up in a random child task. + * + * If we were to destroy the original group, all group related + * operations would cease to function properly after this + * random child dies. + * + * Do destroy all inherited groups, we don't care about those + * and being thorough is better. + */ + detach_flags = DETACH_GROUP | DETACH_CHILD; + mutex_lock(&parent_event->child_mutex); + } - if (parent_event) - perf_group_detach(child_event); - list_del_event(child_event, child_ctx); - perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */ - raw_spin_unlock_irq(&child_ctx->lock); + perf_remove_from_context(event, detach_flags); + + raw_spin_lock_irq(&ctx->lock); + if (event->state > PERF_EVENT_STATE_EXIT) + perf_event_set_state(event, PERF_EVENT_STATE_EXIT); + raw_spin_unlock_irq(&ctx->lock); /* - * Parent events are governed by their filedesc, retain them. + * Child events can be freed. */ - if (!parent_event) { - perf_event_wakeup(child_event); + if (parent_event) { + mutex_unlock(&parent_event->child_mutex); + /* + * Kick perf_poll() for is_event_hup(); + */ + perf_event_wakeup(parent_event); + free_event(event); + put_event(parent_event); return; } - /* - * Child events can be cleaned up. - */ - - sync_child_event(child_event, child); /* - * Remove this event from the parent's list - */ - WARN_ON_ONCE(parent_event->ctx->parent_ctx); - mutex_lock(&parent_event->child_mutex); - list_del_init(&child_event->child_list); - mutex_unlock(&parent_event->child_mutex); - - /* - * Kick perf_poll() for is_event_hup(). + * Parent events are governed by their filedesc, retain them. */ - perf_event_wakeup(parent_event); - free_event(child_event); - put_event(parent_event); + perf_event_wakeup(event); } static void perf_event_exit_task_context(struct task_struct *child, int ctxn) @@ -12511,7 +12526,7 @@ static void perf_event_exit_task_context perf_event_task(child, child_ctx, 0); list_for_each_entry_safe(child_event, next, &child_ctx->event_list, event_entry) - perf_event_exit_event(child_event, child_ctx, child); + perf_event_exit_event(child_event, child_ctx); mutex_unlock(&child_ctx->mutex); @@ -12771,6 +12786,7 @@ inherit_event(struct perf_event *parent_ */ raw_spin_lock_irqsave(&child_ctx->lock, flags); add_event_to_ctx(child_event, child_ctx); + child_event->attach_state |= PERF_ATTACH_CHILD; raw_spin_unlock_irqrestore(&child_ctx->lock, flags); /*
On Tue, Mar 23, 2021 at 03:45PM +0100, Peter Zijlstra wrote: > On Tue, Mar 23, 2021 at 11:32:03AM +0100, Peter Zijlstra wrote: > > And at that point there's very little value in still using > > perf_event_exit_event()... let me see if there's something to be done > > about that. > > I ended up with something like the below. Which then simplifies > remove_on_exec() to: > [...] > > Very lightly tested with that {1..1000} thing. > > --- > > Subject: perf: Rework perf_event_exit_event() > From: Peter Zijlstra <peterz@infradead.org> > Date: Tue Mar 23 15:16:06 CET 2021 > > Make perf_event_exit_event() more robust, such that we can use it from > other contexts. Specifically the up and coming remove_on_exec. > > For this to work we need to address a few issues. Remove_on_exec will > not destroy the entire context, so we cannot rely on TASK_TOMBSTONE to > disable event_function_call() and we thus have to use > perf_remove_from_context(). > > When using perf_remove_from_context(), there's two races to consider. > The first is against close(), where we can have concurrent tear-down > of the event. The second is against child_list iteration, which should > not find a half baked event. > > To address this, teach perf_remove_from_context() to special case > !ctx->is_active and about DETACH_CHILD. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Very nice, thanks! It seems to all hold up to testing as well. Unless you already have this on some branch somewhere, I'll prepend it to the series for now. I'll test some more and try to get v3 out tomorrow. Thanks, -- Marco
On Tue, Mar 23, 2021 at 04:58:37PM +0100, Marco Elver wrote: > On Tue, Mar 23, 2021 at 03:45PM +0100, Peter Zijlstra wrote: > > On Tue, Mar 23, 2021 at 11:32:03AM +0100, Peter Zijlstra wrote: > > > And at that point there's very little value in still using > > > perf_event_exit_event()... let me see if there's something to be done > > > about that. > > > > I ended up with something like the below. Which then simplifies > > remove_on_exec() to: > > > [...] > > > > Very lightly tested with that {1..1000} thing. > > > > --- > > > > Subject: perf: Rework perf_event_exit_event() > > From: Peter Zijlstra <peterz@infradead.org> > > Date: Tue Mar 23 15:16:06 CET 2021 > > > > Make perf_event_exit_event() more robust, such that we can use it from > > other contexts. Specifically the up and coming remove_on_exec. > > > > For this to work we need to address a few issues. Remove_on_exec will > > not destroy the entire context, so we cannot rely on TASK_TOMBSTONE to > > disable event_function_call() and we thus have to use > > perf_remove_from_context(). > > > > When using perf_remove_from_context(), there's two races to consider. > > The first is against close(), where we can have concurrent tear-down > > of the event. The second is against child_list iteration, which should > > not find a half baked event. > > > > To address this, teach perf_remove_from_context() to special case > > !ctx->is_active and about DETACH_CHILD. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > Very nice, thanks! It seems to all hold up to testing as well. > > Unless you already have this on some branch somewhere, I'll prepend it > to the series for now. I'll test some more and try to get v3 out > tomorrow. I have not queued it, so please keep it in your series so it stays together (and tested). Thanks!
On Tue, Mar 23, 2021 at 10:47AM +0100, Marco Elver wrote: > On Tue, 23 Mar 2021 at 04:10, Ian Rogers <irogers@google.com> wrote: > > On Mon, Mar 22, 2021 at 6:24 AM Marco Elver <elver@google.com> wrote: > > > On Wed, Mar 10, 2021 at 11:41AM +0100, Marco Elver wrote: > > > > Add kselftest to test that remove_on_exec removes inherited events from > > > > child tasks. > > > > > > > > Signed-off-by: Marco Elver <elver@google.com> > > > > > > To make compatible with more recent libc, we'll need to fixup the tests > > > with the below. > > > > > > Also, I've seen that tools/perf/tests exists, however it seems to be > > > primarily about perf-tool related tests. Is this correct? > > > > > > I'd propose to keep these purely kernel ABI related tests separate, and > > > that way we can also make use of the kselftests framework which will > > > also integrate into various CI systems such as kernelci.org. > > > > Perhaps there is a way to have both? Having the perf tool spot an > > errant kernel feels like a feature. There are also > > tools/lib/perf/tests and Vince Weaver's tests [1]. It is possible to > > run standalone tests from within perf test by having them be executed > > by a shell test. > > Thanks for the pointers. Sure, I'd support more additional tests. > > But I had another look and it seems the tests in > tools/{perf,lib/perf}/tests do focus on perf-tool or the library > respectively, so adding kernel ABI tests there feels wrong. (If > perf-tool somehow finds use for sigtrap, or remove_on_exec, then > having a perf-tool specific test for those would make sense again.) Ok, I checked once more, and I did find a few pure kernel ABI tests e.g. in "wp.c". [...] > Because I'd much prefer in-tree tests with little boilerplate, that > are structured with parsable output; in the kernel we have the > kselftest framework for tests with a user space component, and KUnit > for pure in-kernel tests. So let's try to have both... but from what I could tell, the remove_on_exec test just can't be turned into a perf tool built-in test, at least not easily. In perf tool I also can't use the new "si_perf" field yet. I'll add the patch below at the end of the series, so that we can have both. Too many tests probably don't hurt... Thanks, -- Marco ------ >8 ------ commit 6a98611ace59c867aa135f780b1879990180548e Author: Marco Elver <elver@google.com> Date: Tue Mar 23 19:51:12 2021 +0100 perf test: Add basic stress test for sigtrap handling Ports the stress test from tools/testing/selftests/sigtrap_threads.c, and add as a perf tool built-in test. This allows checking the basic sigtrap functionality from within the perf tool. Signed-off-by: Marco Elver <elver@google.com> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build index 650aec19d490..a429c7a02b37 100644 --- a/tools/perf/tests/Build +++ b/tools/perf/tests/Build @@ -64,6 +64,7 @@ perf-y += parse-metric.o perf-y += pe-file-parsing.o perf-y += expand-cgroup.o perf-y += perf-time-to-tsc.o +perf-y += sigtrap.o $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build $(call rule_mkdir) diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c index c4b888f18e9c..28a1cb5eaa77 100644 --- a/tools/perf/tests/builtin-test.c +++ b/tools/perf/tests/builtin-test.c @@ -359,6 +359,11 @@ static struct test generic_tests[] = { .func = test__perf_time_to_tsc, .is_supported = test__tsc_is_supported, }, + { + .desc = "Sigtrap support", + .func = test__sigtrap, + .is_supported = test__wp_is_supported, /* uses wp for test */ + }, { .func = NULL, }, diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c new file mode 100644 index 000000000000..0888a4e02222 --- /dev/null +++ b/tools/perf/tests/sigtrap.c @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Basic stress-test for sigtrap support. + * + * Copyright (C) 2021, Google LLC. + */ + +#include <pthread.h> +#include <signal.h> +#include <stdint.h> +#include <stdlib.h> +#include <string.h> +#include <sys/ioctl.h> +#include <sys/syscall.h> +#include <unistd.h> +#include <linux/hw_breakpoint.h> +#include <linux/kernel.h> +#include "tests.h" +#include "debug.h" +#include "event.h" +#include "cloexec.h" +#include "../perf-sys.h" + +#define NUM_THREADS 5 + +/* Data shared between test body, threads, and signal handler. */ +static struct { + int tids_want_signal; /* Which threads still want a signal. */ + int signal_count; /* Sanity check number of signals received. */ + volatile int iterate_on; /* Variable to set breakpoint on. */ + siginfo_t first_siginfo; /* First observed siginfo_t. */ +} ctx; + +static struct perf_event_attr make_event_attr(void) +{ + struct perf_event_attr attr = { + .type = PERF_TYPE_BREAKPOINT, + .size = sizeof(attr), + .sample_period = 1, + .disabled = 1, + .bp_addr = (long)&ctx.iterate_on, + .bp_type = HW_BREAKPOINT_RW, + .bp_len = HW_BREAKPOINT_LEN_1, + .inherit = 1, /* Children inherit events ... */ + .inherit_thread = 1, /* ... but only cloned with CLONE_THREAD. */ + .remove_on_exec = 1, /* Required by sigtrap. */ + .sigtrap = 1, /* Request synchronous SIGTRAP on event. */ + }; + return attr; +} + +static void +sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __maybe_unused) +{ + if (!__atomic_fetch_add(&ctx.signal_count, 1, __ATOMIC_RELAXED)) + ctx.first_siginfo = *info; + __atomic_fetch_sub(&ctx.tids_want_signal, syscall(SYS_gettid), __ATOMIC_RELAXED); +} + +static void *test_thread(void *arg) +{ + pthread_barrier_t *barrier = (pthread_barrier_t *)arg; + pid_t tid = syscall(SYS_gettid); + int i; + + pthread_barrier_wait(barrier); + + __atomic_fetch_add(&ctx.tids_want_signal, tid, __ATOMIC_RELAXED); + for (i = 0; i < ctx.iterate_on - 1; i++) + __atomic_fetch_add(&ctx.tids_want_signal, tid, __ATOMIC_RELAXED); + + return NULL; +} + +static int run_test_threads(pthread_t *threads, pthread_barrier_t *barrier) +{ + int i; + + pthread_barrier_wait(barrier); + for (i = 0; i < NUM_THREADS; i++) + TEST_ASSERT_EQUAL("pthread_join() failed", pthread_join(threads[i], NULL), 0); + + return 0; +} + +static int run_stress_test(int fd, pthread_t *threads, pthread_barrier_t *barrier) +{ + ctx.iterate_on = 3000; + + TEST_ASSERT_EQUAL("misfired signal?", ctx.signal_count, 0); + TEST_ASSERT_EQUAL("enable failed", ioctl(fd, PERF_EVENT_IOC_ENABLE, 0), 0); + if (run_test_threads(threads, barrier)) + return -1; + TEST_ASSERT_EQUAL("disable failed", ioctl(fd, PERF_EVENT_IOC_DISABLE, 0), 0); + + TEST_ASSERT_EQUAL("unexpected sigtraps", ctx.signal_count, NUM_THREADS * ctx.iterate_on); + TEST_ASSERT_EQUAL("missing signals or incorrectly delivered", ctx.tids_want_signal, 0); + TEST_ASSERT_VAL("unexpected si_addr", ctx.first_siginfo.si_addr == &ctx.iterate_on); + TEST_ASSERT_EQUAL("unexpected si_errno", ctx.first_siginfo.si_errno, PERF_TYPE_BREAKPOINT); +#if 0 /* FIXME: test build and enable when libc's signal.h has si_perf. */ + TEST_ASSERT_VAL("unexpected si_perf", ctx.first_siginfo.si_perf == + ((HW_BREAKPOINT_LEN_1 << 16) | HW_BREAKPOINT_RW)); +#endif + + return 0; +} + +int test__sigtrap(struct test *test __maybe_unused, int subtest __maybe_unused) +{ + struct perf_event_attr attr = make_event_attr(); + struct sigaction action = {}; + struct sigaction oldact; + pthread_t threads[NUM_THREADS]; + pthread_barrier_t barrier; + int i, fd, ret = 0; + + pthread_barrier_init(&barrier, NULL, NUM_THREADS + 1); + + action.sa_flags = SA_SIGINFO | SA_NODEFER; + action.sa_sigaction = sigtrap_handler; + sigemptyset(&action.sa_mask); + if (sigaction(SIGTRAP, &action, &oldact)) { + pr_debug("FAILED sigaction()\n"); + ret = -1; + goto out_sigaction; + } + + + fd = sys_perf_event_open(&attr, 0, -1, -1, perf_event_open_cloexec_flag()); + if (fd < 0) { + pr_debug("FAILED sys_perf_event_open()\n"); + ret = -1; + goto out_sigaction; + } + + /* Spawn threads inheriting perf event. */ + for (i = 0; i < NUM_THREADS; i++) { + if (pthread_create(&threads[i], NULL, test_thread, &barrier)) { + pr_debug("FAILED pthread_create()"); + ret = -1; + goto out_perf_event; + } + } + + ret |= run_stress_test(fd, threads, &barrier); + +out_perf_event: + close(fd); +out_sigaction: + sigaction(SIGTRAP, &oldact, NULL); + pthread_barrier_destroy(&barrier); + return ret; +} diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h index b85f005308a3..c3f2e2ecbfd6 100644 --- a/tools/perf/tests/tests.h +++ b/tools/perf/tests/tests.h @@ -127,6 +127,7 @@ int test__parse_metric(struct test *test, int subtest); int test__pe_file_parsing(struct test *test, int subtest); int test__expand_cgroup_events(struct test *test, int subtest); int test__perf_time_to_tsc(struct test *test, int subtest); +int test__sigtrap(struct test *test, int subtest); bool test__bp_signal_is_supported(void); bool test__bp_account_is_supported(void);
diff --git a/tools/testing/selftests/perf_events/.gitignore b/tools/testing/selftests/perf_events/.gitignore index 4dc43e1bd79c..790c47001e77 100644 --- a/tools/testing/selftests/perf_events/.gitignore +++ b/tools/testing/selftests/perf_events/.gitignore @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only sigtrap_threads +remove_on_exec diff --git a/tools/testing/selftests/perf_events/Makefile b/tools/testing/selftests/perf_events/Makefile index 973a2c39ca83..fcafa5f0d34c 100644 --- a/tools/testing/selftests/perf_events/Makefile +++ b/tools/testing/selftests/perf_events/Makefile @@ -2,5 +2,5 @@ CFLAGS += -Wl,-no-as-needed -Wall -I../../../../usr/include LDFLAGS += -lpthread -TEST_GEN_PROGS := sigtrap_threads +TEST_GEN_PROGS := sigtrap_threads remove_on_exec include ../lib.mk diff --git a/tools/testing/selftests/perf_events/remove_on_exec.c b/tools/testing/selftests/perf_events/remove_on_exec.c new file mode 100644 index 000000000000..e176b3a74d55 --- /dev/null +++ b/tools/testing/selftests/perf_events/remove_on_exec.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Test for remove_on_exec. + * + * Copyright (C) 2021, Google LLC. + */ + +#define _GNU_SOURCE +#include <sys/types.h> + +/* We need the latest siginfo from the kernel repo. */ +#include <asm/siginfo.h> +#define __have_siginfo_t 1 +#define __have_sigval_t 1 +#define __have_sigevent_t 1 + +#include <linux/perf_event.h> +#include <pthread.h> +#include <signal.h> +#include <stdatomic.h> +#include <stdbool.h> +#include <stddef.h> +#include <stdint.h> +#include <stdio.h> +#include <sys/ioctl.h> +#include <sys/syscall.h> +#include <unistd.h> + +#include "../kselftest_harness.h" + +static volatile int signal_count; + +static struct perf_event_attr make_event_attr(void) +{ + struct perf_event_attr attr = { + .type = PERF_TYPE_HARDWARE, + .size = sizeof(attr), + .config = PERF_COUNT_HW_INSTRUCTIONS, + .sample_period = 1000, + .exclude_kernel = 1, + .exclude_hv = 1, + .disabled = 1, + .inherit = 1, + /* + * Children normally retain their inherited event on exec; with + * remove_on_exec, we'll remove their event, but the parent and + * any other non-exec'd children will keep their events. + */ + .remove_on_exec = 1, + .sigtrap = 1, + }; + return attr; +} + +static void sigtrap_handler(int signum, siginfo_t *info, void *ucontext) +{ + if (info->si_code != TRAP_PERF) { + fprintf(stderr, "%s: unexpected si_code %d\n", __func__, info->si_code); + return; + } + + signal_count++; +} + +FIXTURE(remove_on_exec) +{ + struct sigaction oldact; + int fd; +}; + +FIXTURE_SETUP(remove_on_exec) +{ + struct perf_event_attr attr = make_event_attr(); + struct sigaction action = {}; + + signal_count = 0; + + /* Initialize sigtrap handler. */ + action.sa_flags = SA_SIGINFO | SA_NODEFER; + action.sa_sigaction = sigtrap_handler; + sigemptyset(&action.sa_mask); + ASSERT_EQ(sigaction(SIGTRAP, &action, &self->oldact), 0); + + /* Initialize perf event. */ + self->fd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, PERF_FLAG_FD_CLOEXEC); + ASSERT_NE(self->fd, -1); +} + +FIXTURE_TEARDOWN(remove_on_exec) +{ + close(self->fd); + sigaction(SIGTRAP, &self->oldact, NULL); +} + +/* Verify event propagates to fork'd child. */ +TEST_F(remove_on_exec, fork_only) +{ + int status; + pid_t pid = fork(); + + if (pid == 0) { + ASSERT_EQ(signal_count, 0); + ASSERT_EQ(ioctl(self->fd, PERF_EVENT_IOC_ENABLE, 0), 0); + while (!signal_count); + _exit(42); + } + + while (!signal_count); /* Child enables event. */ + EXPECT_EQ(waitpid(pid, &status, 0), pid); + EXPECT_EQ(WEXITSTATUS(status), 42); +} + +/* + * Verify that event does _not_ propagate to fork+exec'd child; event enabled + * after fork+exec. + */ +TEST_F(remove_on_exec, fork_exec_then_enable) +{ + pid_t pid_exec, pid_only_fork; + int pipefd[2]; + int tmp; + + /* + * Non-exec child, to ensure exec does not affect inherited events of + * other children. + */ + pid_only_fork = fork(); + if (pid_only_fork == 0) { + /* Block until parent enables event. */ + while (!signal_count); + _exit(42); + } + + ASSERT_NE(pipe(pipefd), -1); + pid_exec = fork(); + if (pid_exec == 0) { + ASSERT_NE(dup2(pipefd[1], STDOUT_FILENO), -1); + close(pipefd[0]); + execl("/proc/self/exe", "exec_child", NULL); + _exit((perror("exec failed"), 1)); + } + close(pipefd[1]); + + ASSERT_EQ(waitpid(pid_exec, &tmp, WNOHANG), 0); /* Child is running. */ + /* Wait for exec'd child to start spinning. */ + EXPECT_EQ(read(pipefd[0], &tmp, sizeof(int)), sizeof(int)); + EXPECT_EQ(tmp, 42); + close(pipefd[0]); + /* Now we can enable the event, knowing the child is doing work. */ + EXPECT_EQ(ioctl(self->fd, PERF_EVENT_IOC_ENABLE, 0), 0); + /* If the event propagated to the exec'd child, it will exit normally... */ + usleep(100000); /* ... give time for event to trigger (in case of bug). */ + EXPECT_EQ(waitpid(pid_exec, &tmp, WNOHANG), 0); /* Should still be running. */ + EXPECT_EQ(kill(pid_exec, SIGKILL), 0); + + /* Verify removal from child did not affect this task's event. */ + tmp = signal_count; + while (signal_count == tmp); /* Should not hang! */ + /* Nor should it have affected the first child. */ + EXPECT_EQ(waitpid(pid_only_fork, &tmp, 0), pid_only_fork); + EXPECT_EQ(WEXITSTATUS(tmp), 42); +} + +/* + * Verify that event does _not_ propagate to fork+exec'd child; event enabled + * before fork+exec. + */ +TEST_F(remove_on_exec, enable_then_fork_exec) +{ + pid_t pid_exec; + int tmp; + + EXPECT_EQ(ioctl(self->fd, PERF_EVENT_IOC_ENABLE, 0), 0); + + pid_exec = fork(); + if (pid_exec == 0) { + execl("/proc/self/exe", "exec_child", NULL); + _exit((perror("exec failed"), 1)); + } + + /* + * The child may exit abnormally at any time if the event propagated and + * a SIGTRAP is sent before the handler was set up. + */ + usleep(100000); /* ... give time for event to trigger (in case of bug). */ + EXPECT_EQ(waitpid(pid_exec, &tmp, WNOHANG), 0); /* Should still be running. */ + EXPECT_EQ(kill(pid_exec, SIGKILL), 0); + + /* Verify removal from child did not affect this task's event. */ + tmp = signal_count; + while (signal_count == tmp); /* Should not hang! */ +} + +TEST_F(remove_on_exec, exec_stress) +{ + pid_t pids[30]; + int i, tmp; + + for (i = 0; i < sizeof(pids) / sizeof(pids[0]); i++) { + pids[i] = fork(); + if (pids[i] == 0) { + execl("/proc/self/exe", "exec_child", NULL); + _exit((perror("exec failed"), 1)); + } + + /* Some forked with event disabled, rest with enabled. */ + if (i > 10) + EXPECT_EQ(ioctl(self->fd, PERF_EVENT_IOC_ENABLE, 0), 0); + } + + usleep(100000); /* ... give time for event to trigger (in case of bug). */ + + for (i = 0; i < sizeof(pids) / sizeof(pids[0]); i++) { + /* All children should still be running. */ + EXPECT_EQ(waitpid(pids[i], &tmp, WNOHANG), 0); + EXPECT_EQ(kill(pids[i], SIGKILL), 0); + } + + /* Verify event is still alive. */ + tmp = signal_count; + while (signal_count == tmp); +} + +/* For exec'd child. */ +static void exec_child(void) +{ + struct sigaction action = {}; + const int val = 42; + + /* Set up sigtrap handler in case we erroneously receive a trap. */ + action.sa_flags = SA_SIGINFO | SA_NODEFER; + action.sa_sigaction = sigtrap_handler; + sigemptyset(&action.sa_mask); + if (sigaction(SIGTRAP, &action, NULL)) + _exit((perror("sigaction failed"), 1)); + + /* Signal parent that we're starting to spin. */ + if (write(STDOUT_FILENO, &val, sizeof(int)) == -1) + _exit((perror("write failed"), 1)); + + /* Should hang here until killed. */ + while (!signal_count); +} + +#define main test_main +TEST_HARNESS_MAIN +#undef main +int main(int argc, char *argv[]) +{ + if (!strcmp(argv[0], "exec_child")) { + exec_child(); + return 1; + } + + return test_main(argc, argv); +}
Add kselftest to test that remove_on_exec removes inherited events from child tasks. Signed-off-by: Marco Elver <elver@google.com> --- v2: * Add patch to series. --- .../testing/selftests/perf_events/.gitignore | 1 + tools/testing/selftests/perf_events/Makefile | 2 +- .../selftests/perf_events/remove_on_exec.c | 256 ++++++++++++++++++ 3 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/perf_events/remove_on_exec.c