| Message ID | 20241014123136.3890807-1-metin.kaya@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | trace-cmd reset: Add option to preserve specific events | expand |
On Mon, 14 Oct 2024 13:31:32 +0100 Metin Kaya <metin.kaya@arm.com> wrote: > Changes since v1: > - Update man page for -k command line parameter of "trace-cmd reset". > - Add tab completion support for "trace-cmd reset". Thanks, I'll go ahead and apply this. But can you do me a favor, and add a test case to utest/tracecmd-utest.c that tests this. It should create a each of the types, start tracing, do the reset, and make sure that the -k keeps what is expected. Doesn't need to be too fancy. I want to add test cases to all new features, as I'm way behind in testing current features :-p You don't need to send another version of this series. Just a standalone patch. -- Steve
On 14/10/2024 10:59 pm, Steven Rostedt wrote: > On Mon, 14 Oct 2024 13:31:32 +0100 > Metin Kaya <metin.kaya@arm.com> wrote: > >> Changes since v1: >> - Update man page for -k command line parameter of "trace-cmd reset". >> - Add tab completion support for "trace-cmd reset". > > Thanks, I'll go ahead and apply this. But can you do me a favor, and add a > test case to utest/tracecmd-utest.c that tests this. It should create a > each of the types, start tracing, do the reset, and make sure that the -k > keeps what is expected. Doesn't need to be too fancy. I wrote the patch below, but noticed I'm unable to preserve uprobes during reset. I verified trace-cmd tries to prevent TRACEFS_DYNEVENT_UPROBE from being destroyed, but I cannot see them after running "trace-cmd reset -k uprobe" command. The same problem exists for uretprobe, too. I confirmed utest uses correct trace-cmd executable. Any ideas? -->8-- commit 01d0822dc5e22a34155b0a70d56c67fa023ccacc Author: Metin Kaya <metin.kaya@arm.com> Date: Tue Oct 15 09:34:49 2024 +0100 trace-cmd utest: Add test cases for trace-cmd reset Implement test cases for dynamic event types kprobe, kretprobe, uprobe, uretprobe and eprobe. Verify resetting synthetic events by inserting a hook to test_trace_sqlhist_hist(). While we are in the neighborhood, fix typo "ceated". Link: https://lore.kernel.org/linux-trace-devel/20241014175905.03bec85a@gandalf.local.home Signed-off-by: Metin Kaya <metin.kaya@arm.com> diff --git a/utest/tracecmd-utest.c b/utest/tracecmd-utest.c index 164f5da1..c1a12965 100644 --- a/utest/tracecmd-utest.c +++ b/utest/tracecmd-utest.c @@ -352,6 +352,11 @@ static void test_trace_sqlhist_hist(void) sleep(1); ret = grep_match(SYNTH_EVENT ":", "show", NULL); CU_TEST(ret == 0); + /* Ensure synthetic events remain untouched after "trace-cmd reset -k synth". */ + ret = run_trace("reset", "-k", "synth", NULL); + CU_TEST(ret == 0); + ret = grep_match(SYNTH_EVENT, "stat", NULL); + CU_TEST(ret == 0); tracefs_instance_reset(NULL); } @@ -598,6 +603,159 @@ static void test_trace_library_read_back(void) tracecmd_close(handle); } +static void test_trace_reset_kprobe(void) +{ + int ret; + FILE *fp; + + /* Create a simple kprobe for do_sys_open */ + fp = fopen("/sys/kernel/tracing/kprobe_events", "w"); + CU_TEST(fp != NULL); + fprintf(fp, "p do_sys_open"); + fclose(fp); + + /* Ensure the kprobe is listed in "trace-cmd stat" output. */ + ret = grep_match("p:kprobes/p_do_sys_open_0 do_sys_open", "stat", NULL); + CU_TEST(ret == 0); + + /* Issue "trace-cmd reset", but keep kprobes. */ + ret = run_trace("reset", "-k", "kprobe", NULL); + CU_TEST(ret == 0); + + /* Verify the kprobe's existence after reset. */ + ret = grep_match("p:kprobes/p_do_sys_open_0 do_sys_open", "stat", NULL); + CU_TEST(ret == 0); +} + +static void test_trace_reset_kretprobe(void) +{ + int ret; + FILE *fp; + + /* Create a simple kretprobe for do_sys_open */ + fp = fopen("/sys/kernel/tracing/kprobe_events", "w"); + CU_TEST(fp != NULL); + fprintf(fp, "r do_sys_open"); + fclose(fp); + + /* Ensure the kretprobe is listed in "trace-cmd stat" output. */ + ret = grep_match("r128:kprobes/r_do_sys_open_0 do_sys_open", "stat", NULL); + CU_TEST(ret == 0); + + /* Issue "trace-cmd reset", but keep kretprobes. */ + ret = run_trace("reset", "-k", "kretprobe", NULL); + CU_TEST(ret == 0); + + /* Verify the kretprobe's existence after reset. */ + ret = grep_match("r128:kprobes/r_do_sys_open_0 do_sys_open", "stat", NULL); + CU_TEST(ret == 0); +} + +static void test_trace_reset_uprobe(void) +{ + int ret; + FILE *fp; + + /* Create a simple uprobe for do_sys_open */ + fp = fopen("/sys/kernel/tracing/uprobe_events", "w"); + CU_TEST(fp != NULL); + fprintf(fp, "p /bin/bash:0x4245c0"); + fclose(fp); + + /* Ensure the uprobe is listed in "trace-cmd stat" output. */ + ret = grep_match("p:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0", "stat", NULL); + CU_TEST(ret == 0); + + /* Issue "trace-cmd reset", but keep uprobes. */ + ret = run_trace("reset", "-k", "uprobe", NULL); + CU_TEST(ret == 0); + + /* Verify the uprobe's existence after reset. */ + ret = grep_match("p:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0", "stat", NULL); + CU_TEST(ret == 0); // TODO: FAILS! +} + +static void test_trace_reset_uretprobe(void) +{ + int ret; + FILE *fp; + + /* Create a simple uretprobe for do_sys_open */ + fp = fopen("/sys/kernel/tracing/uprobe_events", "w"); + CU_TEST(fp != NULL); + fprintf(fp, "r /bin/bash:0x4245c0"); + fclose(fp); + + /* Ensure the uretprobe is listed in "trace-cmd stat" output. */ + ret = grep_match("r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0", "stat", NULL); + CU_TEST(ret == 0); + + /* Issue "trace-cmd reset", but keep uretprobes. */ + ret = run_trace("reset", "-k", "uretprobe", NULL); + CU_TEST(ret == 0); + + /* Verify the uretprobe's existence after reset. */ + ret = grep_match("r:uprobes/p_bash_0x4245c0 /bin/bash:0x00000000004245c0", "stat", NULL); + CU_TEST(ret == 0); // TODO: FAILS! +} + +static void test_trace_reset_eprobe(void) +{ + int ret; + bool matched = false; + size_t l = 0; + ssize_t n; + char *buf = NULL; + struct tracefs_dynevent *deprobe; + FILE *fp; + + deprobe = tracefs_eprobe_alloc(NULL, "sopen_in", "syscalls", "sys_enter_openat", NULL); + CU_TEST(deprobe != NULL); + + ret = tracefs_dynevent_create(deprobe); + CU_TEST(ret == 0); + + /* Issue "trace-cmd reset", but keep eprobes. */ + ret = run_trace("reset", "-k", "eprobe", NULL); + CU_TEST(ret == 0); + + /* Verify the eprobe's existence after reset. */ + fp = fopen("/sys/kernel/tracing/dynamic_events", "r"); + CU_TEST(fp != NULL); + + while ((n = getline(&buf, &l, fp)) != -1) { + if (!strcmp(buf, "e:eprobes/sopen_in syscalls.sys_enter_openat\n")) { + matched = true; + break; + } + } + free(buf); + + fclose(fp); + + CU_TEST(matched == true); + + ret = tracefs_dynevent_destroy(deprobe, false); + CU_TEST(ret == 0); + + tracefs_dynevent_free(deprobe); +} + +static void test_trace_reset(void) +{ + int ret; + + test_trace_reset_kprobe(); + test_trace_reset_kretprobe(); + test_trace_reset_uprobe(); + test_trace_reset_uretprobe(); + test_trace_reset_eprobe(); + + /* Destroy all dynamic events. */ + ret = run_trace("reset", NULL); + CU_TEST(ret == 0); +} + static int test_suite_destroy(void) { unlink(TRACECMD_FILE); @@ -639,7 +797,7 @@ void test_tracecmd_lib(void) suite = CU_add_suite(TRACECMD_SUITE, test_suite_init, test_suite_destroy); if (suite == NULL) { - fprintf(stderr, "Suite \"%s\" cannot be ceated\n", TRACECMD_SUITE); + fprintf(stderr, "Suite \"%s\" cannot be created\n", TRACECMD_SUITE); return; } CU_add_test(suite, "Simple record and report", @@ -656,4 +814,5 @@ void test_tracecmd_lib(void) test_trace_library_read_back); CU_add_test(suite, "Test max length", test_trace_record_max); + CU_add_test(suite, "Simple reset", test_trace_reset); } IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Tue, 15 Oct 2024 14:30:58 +0100 Metin Kaya <metin.kaya@arm.com> wrote: > On 14/10/2024 10:59 pm, Steven Rostedt wrote: > > On Mon, 14 Oct 2024 13:31:32 +0100 > > Metin Kaya <metin.kaya@arm.com> wrote: > > > >> Changes since v1: > >> - Update man page for -k command line parameter of "trace-cmd reset". > >> - Add tab completion support for "trace-cmd reset". > > > > Thanks, I'll go ahead and apply this. But can you do me a favor, and add a > > test case to utest/tracecmd-utest.c that tests this. It should create a > > each of the types, start tracing, do the reset, and make sure that the -k > > keeps what is expected. Doesn't need to be too fancy. > > I wrote the patch below, but noticed I'm unable to preserve uprobes > during reset. I verified trace-cmd tries to prevent > TRACEFS_DYNEVENT_UPROBE from being destroyed, but I cannot see them > after running "trace-cmd reset -k uprobe" command. > The same problem exists for uretprobe, too. > I confirmed utest uses correct trace-cmd executable. > > Any ideas? I'll check it out, but your patch below is all messed up. Can you send it properly? Thanks, -- Steve > > -->8-- > > commit 01d0822dc5e22a34155b0a70d56c67fa023ccacc > Author: Metin Kaya <metin.kaya@arm.com> > Date: Tue Oct 15 09:34:49 2024 +0100 > > trace-cmd utest: Add test cases for trace-cmd reset > > Implement test cases for dynamic event types kprobe, kretprobe, uprobe, > uretprobe and eprobe. > > Verify resetting synthetic events by inserting a hook to > test_trace_sqlhist_hist(). > > While we are in the neighborhood, fix typo "ceated". > > Link: > https://lore.kernel.org/linux-trace-devel/20241014175905.03bec85a@gandalf.local.home > > Signed-off-by: Metin Kaya <metin.kaya@arm.com> > > diff --git a/utest/tracecmd-utest.c b/utest/tracecmd-utest.c > index 164f5da1..c1a12965 100644 > --- a/utest/tracecmd-utest.c > +++ b/utest/tracecmd-utest.c > @@ -352,6 +352,11 @@ static void test_trace_sqlhist_hist(void) > sleep(1); > ret = grep_match(SYNTH_EVENT ":", "show", NULL); > CU_TEST(ret == 0); > + /* Ensure synthetic events remain untouched after "trace-cmd reset -k > synth". */ > + ret = run_trace("reset", "-k", "synth", NULL); > + CU_TEST(ret == 0); > + ret = grep_match(SYNTH_EVENT, "stat", NULL); > + CU_TEST(ret == 0); > > tracefs_instance_reset(NULL); > } > @@ -598,6 +603,159 @@ static void test_trace_library_read_back(void) > tracecmd_close(handle); > } > > +static void test_trace_reset_kprobe(void) > +{ > + int ret; > + FILE *fp; > + > + /* Create a simple kprobe for do_sys_open */ > + fp = fopen("/sys/kernel/tracing/kprobe_events", "w"); > + CU_TEST(fp != NULL); > + fprintf(fp, "p do_sys_open"); > + fclose(fp); > + > + /* Ensure the kprobe is listed in "trace-cmd stat" output. */ > + ret = grep_match("p:kprobes/p_do_sys_open_0 do_sys_open", "stat", NULL); > + CU_TEST(ret == 0); > + > + /* Issue "trace-cmd reset", but keep kprobes. */ > + ret = run_trace("reset", "-k", "kprobe", NULL); > + CU_TEST(ret == 0); > + > + /* Verify the kprobe's existence after reset. */ > + ret = grep_match("p:kprobes/p_do_sys_open_0 do_sys_open", "stat", NULL); > + CU_TEST(ret == 0); > +} > + > +static void test_trace_reset_kretprobe(void) > +{ > + int ret; > + FILE *fp; > + > + /* Create a simple kretprobe for do_sys_open */ > + fp = fopen("/sys/kernel/tracing/kprobe_events", "w"); > + CU_TEST(fp != NULL); > + fprintf(fp, "r do_sys_open"); > + fclose(fp); > + > + /* Ensure the kretprobe is listed in "trace-cmd stat" output. */ > + ret = grep_match("r128:kprobes/r_do_sys_open_0 do_sys_open", "stat", > NULL); > + CU_TEST(ret == 0); > + > + /* Issue "trace-cmd reset", but keep kretprobes. */ > + ret = run_trace("reset", "-k", "kretprobe", NULL); > + CU_TEST(ret == 0); > + > + /* Verify the kretprobe's existence after reset. */ > + ret = grep_match("r128:kprobes/r_do_sys_open_0 do_sys_open", "stat", > NULL); > + CU_TEST(ret == 0); > +} > + > +static void test_trace_reset_uprobe(void) > +{ > + int ret; > + FILE *fp; > + > + /* Create a simple uprobe for do_sys_open */ > + fp = fopen("/sys/kernel/tracing/uprobe_events", "w"); > + CU_TEST(fp != NULL); > + fprintf(fp, "p /bin/bash:0x4245c0"); > + fclose(fp); > + > + /* Ensure the uprobe is listed in "trace-cmd stat" output. */ > + ret = grep_match("p:uprobes/p_bash_0x4245c0 > /bin/bash:0x00000000004245c0", "stat", NULL); > + CU_TEST(ret == 0); > + > + /* Issue "trace-cmd reset", but keep uprobes. */ > + ret = run_trace("reset", "-k", "uprobe", NULL); > + CU_TEST(ret == 0); > + > + /* Verify the uprobe's existence after reset. */ > + ret = grep_match("p:uprobes/p_bash_0x4245c0 > /bin/bash:0x00000000004245c0", "stat", NULL); > + CU_TEST(ret == 0); // TODO: FAILS! > +} > + > +static void test_trace_reset_uretprobe(void) > +{ > + int ret; > + FILE *fp; > + > + /* Create a simple uretprobe for do_sys_open */ > + fp = fopen("/sys/kernel/tracing/uprobe_events", "w"); > + CU_TEST(fp != NULL); > + fprintf(fp, "r /bin/bash:0x4245c0"); > + fclose(fp); > + > + /* Ensure the uretprobe is listed in "trace-cmd stat" output. */ > + ret = grep_match("r:uprobes/p_bash_0x4245c0 > /bin/bash:0x00000000004245c0", "stat", NULL); > + CU_TEST(ret == 0); > + > + /* Issue "trace-cmd reset", but keep uretprobes. */ > + ret = run_trace("reset", "-k", "uretprobe", NULL); > + CU_TEST(ret == 0); > + > + /* Verify the uretprobe's existence after reset. */ > + ret = grep_match("r:uprobes/p_bash_0x4245c0 > /bin/bash:0x00000000004245c0", "stat", NULL); > + CU_TEST(ret == 0); // TODO: FAILS! > +} > + > +static void test_trace_reset_eprobe(void) > +{ > + int ret; > + bool matched = false; > + size_t l = 0; > + ssize_t n; > + char *buf = NULL; > + struct tracefs_dynevent *deprobe; > + FILE *fp; > + > + deprobe = tracefs_eprobe_alloc(NULL, "sopen_in", "syscalls", > "sys_enter_openat", NULL); > + CU_TEST(deprobe != NULL); > + > + ret = tracefs_dynevent_create(deprobe); > + CU_TEST(ret == 0); > + > + /* Issue "trace-cmd reset", but keep eprobes. */ > + ret = run_trace("reset", "-k", "eprobe", NULL); > + CU_TEST(ret == 0); > + > + /* Verify the eprobe's existence after reset. */ > + fp = fopen("/sys/kernel/tracing/dynamic_events", "r"); > + CU_TEST(fp != NULL); > + > + while ((n = getline(&buf, &l, fp)) != -1) { > + if (!strcmp(buf, "e:eprobes/sopen_in syscalls.sys_enter_openat\n")) { > + matched = true; > + break; > + } > + } > + free(buf); > + > + fclose(fp); > + > + CU_TEST(matched == true); > + > + ret = tracefs_dynevent_destroy(deprobe, false); > + CU_TEST(ret == 0); > + > + tracefs_dynevent_free(deprobe); > +} > + > +static void test_trace_reset(void) > +{ > + int ret; > + > + test_trace_reset_kprobe(); > + test_trace_reset_kretprobe(); > + test_trace_reset_uprobe(); > + test_trace_reset_uretprobe(); > + test_trace_reset_eprobe(); > + > + /* Destroy all dynamic events. */ > + ret = run_trace("reset", NULL); > + CU_TEST(ret == 0); > +} > + > static int test_suite_destroy(void) > { > unlink(TRACECMD_FILE); > @@ -639,7 +797,7 @@ void test_tracecmd_lib(void) > > suite = CU_add_suite(TRACECMD_SUITE, test_suite_init, > test_suite_destroy); > if (suite == NULL) { > - fprintf(stderr, "Suite \"%s\" cannot be ceated\n", TRACECMD_SUITE); > + fprintf(stderr, "Suite \"%s\" cannot be created\n", TRACECMD_SUITE); > return; > } > CU_add_test(suite, "Simple record and report", > @@ -656,4 +814,5 @@ void test_tracecmd_lib(void) > test_trace_library_read_back); > CU_add_test(suite, "Test max length", > test_trace_record_max); > + CU_add_test(suite, "Simple reset", test_trace_reset); > } > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Tue, 15 Oct 2024 14:30:58 +0100 Metin Kaya <metin.kaya@arm.com> wrote: > On 14/10/2024 10:59 pm, Steven Rostedt wrote: > > On Mon, 14 Oct 2024 13:31:32 +0100 > > Metin Kaya <metin.kaya@arm.com> wrote: > > > >> Changes since v1: > >> - Update man page for -k command line parameter of "trace-cmd reset". > >> - Add tab completion support for "trace-cmd reset". > > > > Thanks, I'll go ahead and apply this. But can you do me a favor, and add a > > test case to utest/tracecmd-utest.c that tests this. It should create a > > each of the types, start tracing, do the reset, and make sure that the -k > > keeps what is expected. Doesn't need to be too fancy. > > I wrote the patch below, but noticed I'm unable to preserve uprobes > during reset. I verified trace-cmd tries to prevent > TRACEFS_DYNEVENT_UPROBE from being destroyed, but I cannot see them > after running "trace-cmd reset -k uprobe" command. > The same problem exists for uretprobe, too. > I confirmed utest uses correct trace-cmd executable. > > Any ideas? Ug, that's because uprobes are defined in dynamic_events with a "p:", just like kprobes. :-p And the tracefs dynamic event parser flags uprobes as kprobes. Thus, if we are deleting kprobes, we will delete uprobes too. I need to fix that. We can't change the kernel as it's ABI unfortunately, but there's other ways libtracefs can differentiate the two. -- Steve
We may want to preserve some of the dynamic events (e.g., kprobes) during a "trace-cmd reset" in some cases as discussed at [1]. Thus, introduce a new command line option to "trace-cmd reset" to permit sustaining specified events across reset. To implement the necessary support, address a corner case in command line option parsing handler in "trace-cmd reset" first. Because all dynamic events gets destroyed even if the user passes an invalid parameter to "trace-cmd reset". Here is an example usage with the new command line option (-k): # trace-cmd reset -h trace-cmd version 3.3.1 (841c96c66b529d52bfce9e84a533fa904ba954ed) usage: trace-cmd reset [-b size][-B buf][-k event][-a][-d][-t] Disables the tracer (may reset trace file) Used in conjunction with start -b change the kernel buffer size (in kilobytes per CPU) -d delete the previous specified instance -B reset the given buffer instance (may specify multiple -B) -a reset all instances (except top one) -t reset the top level instance (useful with -B or -a) -k keep dynamic event during reset (can be specified multiple times). Valid values are: 'kprobe', 'kretprobe', 'uprobe', 'uretprobe', 'eprobe', 'synth' and 'all'. # Add kprobe and kretprobe for sys_open(): $ echo 'p do_sys_open' > /sys/kernel/tracing/kprobe_events $ echo 1 > /sys/kernel/tracing/events/kprobes/p_do_sys_open_0/enable $ echo 'r do_sys_open' >> /sys/kernel/tracing/kprobe_events $ echo 1 > /sys/kernel/tracing/events/kprobes/r_do_sys_open_0/enable $ cat /sys/kernel/tracing/kprobe_events p:kprobes/p_do_sys_open_0 do_sys_open r128:kprobes/r_do_sys_open_0 do_sys_open # Issue reset, but keep kprobes and kretprobes ('-k all' would keep # all existing dynamic events): $ trace-cmd reset -k kprobe -k kretprobe $ cat /sys/kernel/tracing/kprobe_events p:kprobes/p_do_sys_open_0 do_sys_open r128:kprobes/r_do_sys_open_0 do_sys_open # Issue reset, but this time only keep kretprobes: $ trace-cmd reset -k kretprobe $ cat /sys/kernel/tracing/kprobe_events r128:kprobes/r_do_sys_open_0 do_sys_open # Don't preserve any dynamic event: $ trace-cmd reset $ cat /sys/kernel/tracing/kprobe_events $ Changes since v1: - Update man page for -k command line parameter of "trace-cmd reset". - Add tab completion support for "trace-cmd reset". [1] https://bugzilla.kernel.org/show_bug.cgi?id=219302 Metin Kaya (4): trace-cmd reset: Bail out immediately if user provides an invalid option trace-cmd reset: Add option to preserve specific dynamic events trace-cmd reset: Update man page for -k option trace-cmd reset: Add bash tab completion for -B and -k Documentation/trace-cmd/trace-cmd-reset.1.txt | 12 +++- tracecmd/trace-cmd.bash | 31 ++++++++++ tracecmd/trace-record.c | 62 +++++++++++++++++-- tracecmd/trace-usage.c | 6 +- 4 files changed, 103 insertions(+), 8 deletions(-)