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

Namhyung Kim
2018-02-09T03:15:47Z

On Fri, Feb 02, 2018 at 06:05:13PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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@goodmis.org>
> ---
> 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;
> }