| Message ID | 1573123866-348262-2-git-send-email-vincent.donnefort@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] trace-cmd: Enable kptr_restrict | expand |
On Thu, 7 Nov 2019 10:51:06 +0000 vincent.donnefort@arm.com wrote: > From: Vincent Donnefort <vincent.donnefort@arm.com> > > The tracing file saved_cmdlines_size allows setting the number of entries > that saved_cmdlines will contain. The latter is then dumped into the > trace.dat file to map PIDs with comm. The default value is 128. > > Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com> > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index 7260d27..a4e10a4 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -229,6 +229,7 @@ struct common_record_context { > int topt; > int do_child; > int run_command; > + int saved_cmdlines_size; > }; > > static void add_reset_file(const char *file, const char *val, int prio) > @@ -1810,6 +1811,39 @@ static void set_options(void) > } > } > > +static void set_saved_cmdlines_size(struct common_record_context *ctx) > +{ > + char *path, *str; > + int fd, len, ret; > + > + if (!ctx->saved_cmdlines_size) > + return; > + > + path = tracecmd_get_tracing_file("saved_cmdlines_size"); > + if (!path) > + goto err; > + > + reset_save_file(path, RESET_DEFAULT_PRIO); > + > + fd = open(path, O_WRONLY); > + tracecmd_put_tracing_file(path); > + if (fd < 0) > + goto err; > + > + len = asprintf(&str, "%d", ctx->saved_cmdlines_size); > + if (len < 0) > + die("%s couldn't allocate memory", __func__); > + > + if (write(fd, str, len) > 0) > + ret = 0; > + > + close(fd); > + free(str); > +err: > + if (ret) > + warning("Couldn't set saved_cmdlines_size"); > +} > + > static int trace_check_file_exists(struct buffer_instance *instance, char *file) > { > struct stat st; > @@ -5480,7 +5514,7 @@ static void parse_record_options(int argc, > if (IS_EXTRACT(ctx)) > opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT"; > else > - opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q"; > + opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK"; > c = getopt_long (argc-1, argv+1, opts, long_options, &option_index); > if (c == -1) > break; > @@ -5741,6 +5775,9 @@ static void parse_record_options(int argc, > case 'k': > keep = 1; > break; > + case 'K': > + ctx->saved_cmdlines_size = atoi(optarg); > + break; Perhaps we should make this a long variable --cmdline-size(?), and not yet give it a short option. > case 'i': > ignore_event_not_found = 1; > break; > @@ -5990,6 +6027,7 @@ static void record_trace(int argc, char **argv, > enable_events(instance); > } > > + set_saved_cmdlines_size(ctx); > set_buffer_size(); > update_plugins(type); > set_options(); > diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c > index b5788f7..ddfb480 100644 > --- a/tracecmd/trace-usage.c > +++ b/tracecmd/trace-usage.c > @@ -47,6 +47,7 @@ static struct usage_help usage_help[] = { > " -b change kernel buffersize (in kilobytes per CPU)\n" > " -B create sub buffer and following events will be enabled here\n" > " -k do not reset the buffers after tracing.\n" > + " -K change kernel saved_cmdlines_size\n" We would also want to update the man pages too. Thanks! -- Steve > " -i do not fail if an event is not found\n" > " -q print no output to the screen\n" > " --quiet print no output to the screen\n"
On Mon, Nov 11, 2019 at 05:52:03PM -0500, Steven Rostedt wrote: > On Thu, 7 Nov 2019 10:51:06 +0000 > vincent.donnefort@arm.com wrote: > > > From: Vincent Donnefort <vincent.donnefort@arm.com> > > > > The tracing file saved_cmdlines_size allows setting the number of entries > > that saved_cmdlines will contain. The latter is then dumped into the > > trace.dat file to map PIDs with comm. The default value is 128. > > > > Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com> > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index 7260d27..a4e10a4 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > @@ -229,6 +229,7 @@ struct common_record_context { > > int topt; > > int do_child; > > int run_command; > > + int saved_cmdlines_size; > > }; > > > > static void add_reset_file(const char *file, const char *val, int prio) > > @@ -1810,6 +1811,39 @@ static void set_options(void) > > } > > } > > > > +static void set_saved_cmdlines_size(struct common_record_context *ctx) > > +{ > > + char *path, *str; > > + int fd, len, ret; > > + > > + if (!ctx->saved_cmdlines_size) > > + return; > > + > > + path = tracecmd_get_tracing_file("saved_cmdlines_size"); > > + if (!path) > > + goto err; > > + > > + reset_save_file(path, RESET_DEFAULT_PRIO); > > + > > + fd = open(path, O_WRONLY); > > + tracecmd_put_tracing_file(path); > > + if (fd < 0) > > + goto err; > > + > > + len = asprintf(&str, "%d", ctx->saved_cmdlines_size); > > + if (len < 0) > > + die("%s couldn't allocate memory", __func__); > > + > > + if (write(fd, str, len) > 0) > > + ret = 0; > > + > > + close(fd); > > + free(str); > > +err: > > + if (ret) > > + warning("Couldn't set saved_cmdlines_size"); > > +} > > + > > static int trace_check_file_exists(struct buffer_instance *instance, char *file) > > { > > struct stat st; > > @@ -5480,7 +5514,7 @@ static void parse_record_options(int argc, > > if (IS_EXTRACT(ctx)) > > opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT"; > > else > > - opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q"; > > + opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK"; > > c = getopt_long (argc-1, argv+1, opts, long_options, &option_index); > > if (c == -1) > > break; > > @@ -5741,6 +5775,9 @@ static void parse_record_options(int argc, > > case 'k': > > keep = 1; > > break; > > + case 'K': > > + ctx->saved_cmdlines_size = atoi(optarg); > > + break; > > Perhaps we should make this a long variable --cmdline-size(?), and not > yet give it a short option. Hi Steve, Thank you for your review. I also thought that we could avoid having a new option and try to write /proc/sys/kernel/threads-max into saved_cmdlines_size, before recording the trace. Do you think that would be a more suitable solution? -- Vincent > > > case 'i': > > ignore_event_not_found = 1; > > break; > > @@ -5990,6 +6027,7 @@ static void record_trace(int argc, char **argv, > > enable_events(instance); > > } > > > > + set_saved_cmdlines_size(ctx); > > set_buffer_size(); > > update_plugins(type); > > set_options(); > > diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c > > index b5788f7..ddfb480 100644 > > --- a/tracecmd/trace-usage.c > > +++ b/tracecmd/trace-usage.c > > @@ -47,6 +47,7 @@ static struct usage_help usage_help[] = { > > " -b change kernel buffersize (in kilobytes per CPU)\n" > > " -B create sub buffer and following events will be enabled here\n" > > " -k do not reset the buffers after tracing.\n" > > + " -K change kernel saved_cmdlines_size\n" > > We would also want to update the man pages too. > > Thanks! > > -- Steve > > > " -i do not fail if an event is not found\n" > > " -q print no output to the screen\n" > > " --quiet print no output to the screen\n" > 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, 12 Nov 2019 12:03:15 +0000 Vincent Donnefort <vincent.donnefort@arm.com> wrote: > Hi Steve, > > Thank you for your review. I also thought that we could avoid having a new > option and try to write /proc/sys/kernel/threads-max into saved_cmdlines_size, > before recording the trace. Do you think that would be a more suitable > solution? That's an interesting approach. I still think that having a parameter would be good as well, but then use the threads-max approach if one is not supplied. -- Steve
On Tue, Nov 12, 2019 at 09:40:37AM -0500, Steven Rostedt wrote: > On Tue, 12 Nov 2019 12:03:15 +0000 > Vincent Donnefort <vincent.donnefort@arm.com> wrote: > > > Hi Steve, > > > > Thank you for your review. I also thought that we could avoid having a new > > option and try to write /proc/sys/kernel/threads-max into saved_cmdlines_size, > > before recording the trace. Do you think that would be a more suitable > > solution? > > That's an interesting approach. I still think that having a parameter > would be good as well, but then use the threads-max approach if one is > not supplied. > > -- Steve This was not a good idea in the end. We can't rely on the sysctl entries threads-max or pid_max, as the pid/comm map is static and the maximum is defined by PID_MAX_DEFAULT. I do not see any way of getting this value from user-space. We could though, as an alternative, use a hard-coded value of 32768 for --cmdlines-size. Without being perfect, this should still be true for most of the systems. PID_MAX_DEFAULT depends on CONFIG_BASE_SMALL and hasn't changed since Linux started using Git. -- Vincent 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, 12 Nov 2019 19:01:02 +0000 Vincent Donnefort <vincent.donnefort@arm.com> wrote: > This was not a good idea in the end. We can't rely on the sysctl entries > threads-max or pid_max, as the pid/comm map is static and the maximum is > defined by PID_MAX_DEFAULT. I do not see any way of getting this value from > user-space. > > We could though, as an alternative, use a hard-coded value of 32768 for > --cmdlines-size. Without being perfect, this should still be true for most of > the systems. PID_MAX_DEFAULT depends on CONFIG_BASE_SMALL and hasn't changed > since Linux started using Git. But that does take up a bit of memory in the kernel, which isn't probably needed. What we could do, is periodically record the contents of the file, and update the map at the end of the record. Perhaps I should even make it so that we can just get notifications when it does get updated. But that would require a kernel change. -- Steve
On Tue, Nov 12, 2019 at 02:09:20PM -0500, Steven Rostedt wrote: > On Tue, 12 Nov 2019 19:01:02 +0000 > Vincent Donnefort <vincent.donnefort@arm.com> wrote: > > > This was not a good idea in the end. We can't rely on the sysctl entries > > threads-max or pid_max, as the pid/comm map is static and the maximum is > > defined by PID_MAX_DEFAULT. I do not see any way of getting this value from > > user-space. > > > > We could though, as an alternative, use a hard-coded value of 32768 for > > --cmdlines-size. Without being perfect, this should still be true for most of > > the systems. PID_MAX_DEFAULT depends on CONFIG_BASE_SMALL and hasn't changed > > since Linux started using Git. > > But that does take up a bit of memory in the kernel, which isn't > probably needed. What we could do, is periodically record the contents > of the file, and update the map at the end of the record. > > Perhaps I should even make it so that we can just get notifications > when it does get updated. But that would require a kernel change. > > -- Steve Hi Steve, I then pushed an updated version covering your previous comments, small issues and introducing the --cmdlines-size option without any default value. Aside, apology for the disclaimer notice into the previous emails. This should hopefully be gone by now.
On Wed, 13 Nov 2019 12:27:35 +0000 Vincent Donnefort <vincent.donnefort@arm.com> wrote: > Hi Steve, > > I then pushed an updated version covering your previous comments, small issues > and introducing the --cmdlines-size option without any default value. I actually had applied both your patches and pushed them up. I reverted the second one to apply this updated version. > > Aside, apology for the disclaimer notice into the previous emails. This should > hopefully be gone by now. No problem. Thanks! -- Steve
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index 7260d27..a4e10a4 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -229,6 +229,7 @@ struct common_record_context { int topt; int do_child; int run_command; + int saved_cmdlines_size; }; static void add_reset_file(const char *file, const char *val, int prio) @@ -1810,6 +1811,39 @@ static void set_options(void) } } +static void set_saved_cmdlines_size(struct common_record_context *ctx) +{ + char *path, *str; + int fd, len, ret; + + if (!ctx->saved_cmdlines_size) + return; + + path = tracecmd_get_tracing_file("saved_cmdlines_size"); + if (!path) + goto err; + + reset_save_file(path, RESET_DEFAULT_PRIO); + + fd = open(path, O_WRONLY); + tracecmd_put_tracing_file(path); + if (fd < 0) + goto err; + + len = asprintf(&str, "%d", ctx->saved_cmdlines_size); + if (len < 0) + die("%s couldn't allocate memory", __func__); + + if (write(fd, str, len) > 0) + ret = 0; + + close(fd); + free(str); +err: + if (ret) + warning("Couldn't set saved_cmdlines_size"); +} + static int trace_check_file_exists(struct buffer_instance *instance, char *file) { struct stat st; @@ -5480,7 +5514,7 @@ static void parse_record_options(int argc, if (IS_EXTRACT(ctx)) opts = "+haf:Fp:co:O:sr:g:l:n:P:N:tb:B:ksiT"; else - opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:ksSiTm:M:H:q"; + opts = "+hae:f:FA:p:cC:dDGo:O:s:r:vg:l:n:P:N:tb:R:B:kK:sSiTm:M:H:qK"; c = getopt_long (argc-1, argv+1, opts, long_options, &option_index); if (c == -1) break; @@ -5741,6 +5775,9 @@ static void parse_record_options(int argc, case 'k': keep = 1; break; + case 'K': + ctx->saved_cmdlines_size = atoi(optarg); + break; case 'i': ignore_event_not_found = 1; break; @@ -5990,6 +6027,7 @@ static void record_trace(int argc, char **argv, enable_events(instance); } + set_saved_cmdlines_size(ctx); set_buffer_size(); update_plugins(type); set_options(); diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c index b5788f7..ddfb480 100644 --- a/tracecmd/trace-usage.c +++ b/tracecmd/trace-usage.c @@ -47,6 +47,7 @@ static struct usage_help usage_help[] = { " -b change kernel buffersize (in kilobytes per CPU)\n" " -B create sub buffer and following events will be enabled here\n" " -k do not reset the buffers after tracing.\n" + " -K change kernel saved_cmdlines_size\n" " -i do not fail if an event is not found\n" " -q print no output to the screen\n" " --quiet print no output to the screen\n"