Re: [PATCH 4/4] tracing: Allow boot instances to have snapshot buffers

[Ross Zwisler <zwisler@google.com>]

On Wed, Jan 11, 2023 at 09:56:40AM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add to ftrace_boot_snapshot, "=<instance>" name, where the instance will
> get a snapshot buffer, and will take a snapshot at the end of boot (which
> will save the boot traces).
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  .../admin-guide/kernel-parameters.txt         |  9 +++
>  kernel/trace/trace.c                          | 77 +++++++++++++++++--
>  2 files changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7a7f41652719..f4e87b17427f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1532,6 +1532,15 @@
>  			boot up that is likely to be overridden by user space
>  			start up functionality.
>  
> +			Optionally, the snapshot can also be defined for a tracing
> +			instance that was created by the trace_instance= command
> +			line parameter.
> +
> +			trace_instance=foo,sched_switch ftrace_boot_snapshot=foo
> +
> +			The above will cause the "foo" tracing instance to trigger
> +			a snapshot at the end of boot up.
> +
>  	ftrace_dump_on_oops[=orig_cpu]
>  			[FTRACE] will dump the trace buffers on oops.
>  			If no parameter is passed, ftrace will dump
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 3cb9bbc0f076..d445789dc247 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -192,6 +192,9 @@ static bool snapshot_at_boot;
>  static char boot_instance_info[COMMAND_LINE_SIZE] __initdata;
>  static int boot_instance_index;
>  
> +static char boot_snapshot_info[COMMAND_LINE_SIZE] __initdata;

For x86 machines at least COMMAND_LINE_SIZE is pretty big (2048), so between
boot_instance_info and boot_snapshot_info we are using an entire 4k of memory.
It seems unlikely that any user would need a string this long for these
options.  Should we trim this down to something smaller?

> +static int boot_snapshot_index;
> +
>  static int __init set_cmdline_ftrace(char *str)
>  {
>  	strlcpy(bootup_tracer_buf, str, MAX_TRACER_SIZE);
> @@ -228,9 +231,22 @@ __setup("traceoff_on_warning", stop_trace_on_warning);
>  
>  static int __init boot_alloc_snapshot(char *str)
>  {
> -	allocate_snapshot = true;
> -	/* We also need the main ring buffer expanded */
> -	ring_buffer_expanded = true;
> +	char *slot = boot_snapshot_info + boot_snapshot_index;
> +	int left = COMMAND_LINE_SIZE - boot_snapshot_index;

sizeof(boot_snapshot_info) is a bit safer than COMMAND_LINE_SIZE so they don't
get out of sync, plus we may also want to shrink it a bit as mentioned above.

Just two nits, other than that you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>