Re: [PATCH 15/18] tracing: Add string type for dynamic strings in function based events

[Namhyung Kim]

On Fri, Feb 02, 2018 at 06:05:13PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> 
> Add a "string" type that will create a dynamic length string for the
> event, this is the same as the __string() field in normal TRACE_EVENTS.
> 
> [ missing 'static' found by Fengguang Wu's kbuild test robot ]
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
>  Documentation/trace/function-based-events.rst |  19 ++-
>  kernel/trace/trace_event_ftrace.c             | 183 +++++++++++++++++++++++---
>  2 files changed, 181 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/trace/function-based-events.rst b/Documentation/trace/function-based-events.rst
> index 99ae77cd59e6..6c643ea749e7 100644
> --- a/Documentation/trace/function-based-events.rst
> +++ b/Documentation/trace/function-based-events.rst
> @@ -99,7 +99,7 @@ as follows:
>           's8' | 's16' | 's32' | 's64' |
>           'x8' | 'x16' | 'x32' | 'x64' |
>           'char' | 'short' | 'int' | 'long' | 'size_t' |
> -	 'symbol'
> +	 'symbol' | 'string'
>  
>   FIELD := <name> | <name> INDEX | <name> OFFSET | <name> OFFSET INDEX
>  
> @@ -342,3 +342,20 @@ the format "%s". If a nul is found, the output will stop. Use another type
>        bash-1470  [003] ...2   980.678715: path_openat->link_path_walk(name=/lib64/ld-linux-x86-64.so.2)
>        bash-1470  [003] ...2   980.678721: path_openat->link_path_walk(name=ld-2.24.so)
>        bash-1470  [003] ...2   980.678978: path_lookupat->link_path_walk(name=/etc/ld.so.preload)
> +
> +
> +Dynamic strings
> +===============
> +
> +Static strings are fine, but they can waste a lot of memory in the ring buffer.
> +The above allocated 64 bytes for a character array, but most of the output was
> +less than 20 characters. Not wanting to truncate strings or waste space on
> +the ring buffer, the dynamic string can help.
> +
> +Use the "string" type for strings that have a large range in size. The max
> +size that will be recorded is 512 bytes. If a string is larger than that, then
> +it will be truncated.
> +
> + # echo 'link_path_walk(string name)' > function_events
> +
> +Gives the same result as above, but does not waste buffer space.
> diff --git a/kernel/trace/trace_event_ftrace.c b/kernel/trace/trace_event_ftrace.c
> index dd24b840329d..273c5838a8e2 100644
> --- a/kernel/trace/trace_event_ftrace.c
> +++ b/kernel/trace/trace_event_ftrace.c
> @@ -39,6 +39,7 @@ struct func_event {
>  	struct func_arg			*last_arg;
>  	int				arg_cnt;
>  	int				arg_offset;
> +	int				has_strings;
>  };
>  
>  struct func_file {
> @@ -83,6 +84,8 @@ typedef u32 x32;
>  typedef u16 x16;
>  typedef u8 x8;
>  typedef void * symbol;
> +/* 2 byte offset, 2 byte length */
> +typedef u32 string;
>  
>  #define TYPE_TUPLE(type)			\
>  	{ #type, sizeof(type), is_signed_type(type) }
> @@ -105,7 +108,8 @@ typedef void * symbol;
>  	TYPE_TUPLE(u8),				\
>  	TYPE_TUPLE(s8),				\
>  	TYPE_TUPLE(x8),				\
> -	TYPE_TUPLE(symbol)
> +	TYPE_TUPLE(symbol),			\
> +	TYPE_TUPLE(string)
>  
>  static struct func_type {
>  	char		*name;
> @@ -124,6 +128,16 @@ enum {
>  	FUNC_TYPE_MAX
>  };
>  
> +#define MAX_STR		512
> +
> +/* Two contexts, normal and NMI, hence the " * 2" */
> +struct func_string {
> +	char		buf[MAX_STR * 2];
> +};
> +
> +static struct func_string __percpu *str_buffer;
> +static int nr_strings;

What protects it?

Thanks,
Namhyung


> +
>  /**
>   * arch_get_func_args - retrieve function arguments via pt_regs
>   * @regs: The registers at the moment the function is called
> @@ -163,6 +177,23 @@ int __weak arch_get_func_args(struct pt_regs *regs,
>  	return 0;
>  }
>  
> +static void free_arg(struct func_arg *arg)
> +{
> +	list_del(&arg->list);
> +	if (arg->func_type == FUNC_TYPE_string) {
> +		nr_strings--;
> +		if (WARN_ON(nr_strings < 0))
> +			nr_strings = 0;
> +		if (!nr_strings) {
> +			free_percpu(str_buffer);
> +			str_buffer = NULL;
> +		}
> +	}
> +	kfree(arg->name);
> +	kfree(arg->type);
> +	kfree(arg);
> +}
> +
>  static void free_func_event(struct func_event *func_event)
>  {
>  	struct func_arg *arg, *n;
> @@ -171,10 +202,7 @@ static void free_func_event(struct func_event *func_event)
>  		return;
>  
>  	list_for_each_entry_safe(arg, n, &func_event->args, list) {
> -		list_del(&arg->list);
> -		kfree(arg->name);
> -		kfree(arg->type);
> -		kfree(arg);
> +		free_arg(arg);
>  	}
>  	ftrace_free_filter(&func_event->ops);
>  	kfree(func_event->call.print_fmt);
> @@ -255,6 +283,17 @@ static int add_arg(struct func_event *fevent, int ftype, int unsign)
>  	list_add_tail(&arg->list, &fevent->args);
>  	fevent->last_arg = arg;
>  
> +	if (ftype == FUNC_TYPE_string) {
> +		fevent->has_strings++;
> +		nr_strings++;
> +		if (nr_strings == 1) {
> +			str_buffer = alloc_percpu(struct func_string);
> +			if (!str_buffer) {
> +				free_arg(arg);
> +				return -ENOMEM;
> +			}
> +		}
> +	}
>  	return 0;
>  }