Re: [PATCH 07/18 v3] tracing: Add TRACE_DEFINE_ENUM() macro to map enums to their values

[Namhyung Kim <namhyung@kernel.org>]

Hi Steve,

On Thu, Apr 02, 2015 at 09:38:09PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> Several tracepoints use the helper functions __print_symbolic() or
> __print_flags() and pass in enums that do the mapping between the
> binary data stored and the value to print. This works well for reading
> the ASCII trace files, but when the data is read via userspace tools
> such as perf and trace-cmd, the conversion of the binary value to a
> human string format is lost if an enum is used, as userspace does not
> have access to what the ENUM is.
> 
> For example, the tracepoint trace_tlb_flush() has:
> 
>  __print_symbolic(REC->reason,
>     { TLB_FLUSH_ON_TASK_SWITCH, "flush on task switch" },
>     { TLB_REMOTE_SHOOTDOWN, "remote shootdown" },
>     { TLB_LOCAL_SHOOTDOWN, "local shootdown" },
>     { TLB_LOCAL_MM_SHOOTDOWN, "local mm shootdown" })
> 
> Which maps the enum values to the strings they represent. But perf and
> trace-cmd do no know what value TLB_LOCAL_MM_SHOOTDOWN is, and would
> not be able to map it.
> 
> With TRACE_DEFINE_ENUM(), developers can place these in the event header
> files and ftrace will convert the enums to their values:
> 
> By adding:
> 
>  TRACE_DEFINE_ENUM(TLB_FLUSH_ON_TASK_SWITCH);
>  TRACE_DEFINE_ENUM(TLB_REMOTE_SHOOTDOWN);
>  TRACE_DEFINE_ENUM(TLB_LOCAL_SHOOTDOWN);
>  TRACE_DEFINE_ENUM(TLB_LOCAL_MM_SHOOTDOWN);
> 
>  $ cat /sys/kernel/debug/tracing/events/tlb/tlb_flush/format
> [...]
>  __print_symbolic(REC->reason,
>     { 0, "flush on task switch" },
>     { 1, "remote shootdown" },
>     { 2, "local shootdown" },
>     { 3, "local mm shootdown" })
> 
> The above is what userspace expects to see, and tools do not need to
> be modified to parse them.
> 
> Cc: Guilherme Cox <cox@computer.org>
> Cc: Tony Luck <tony.luck@gmail.com>
> Cc: Xie XiuQi <xiexiuqi@huawei.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---

[SNIP]
> +static void update_event_printk(struct ftrace_event_call *call,
> +				struct trace_enum_map *map)
> +{
> +	char *ptr;
> +	int quote = 0;
> +	int len = strlen(map->enum_string);
> +
> +	for (ptr = call->print_fmt; *ptr; ptr++) {
> +		if (*ptr == '\\') {
> +			ptr++;
> +			/* paranoid */
> +			if (!*ptr)
> +				break;
> +			continue;
> +		}
> +		if (*ptr == '"') {
> +			quote ^= 1;
> +			continue;
> +		}
> +		if (quote)
> +			continue;
> +		if (isdigit(*ptr)) {
> +			/* skip numbers */
> +			do {
> +				ptr++;
> +				/* Check for alpha chars like ULL */
> +			} while (isalnum(*ptr));
> +			/*
> +			 * A number must have some kind of delimiter after
> +			 * it, and we can ignore that too.
> +			 */
> +			continue;
> +		}
> +		if (isalpha(*ptr) || *ptr == '_') {
> +			if (strncmp(map->enum_string, ptr, len) == 0 &&
> +			    !isalnum(ptr[len]) && ptr[len] != '_') {
> +				ptr = enum_replace(ptr, map, len);
> +				/* Hmm, enum string smaller than value */
> +				if (WARN_ON_ONCE(!ptr))
> +					return;
> +				/*
> +				 * No need to decrement here, as enum_replace()
> +				 * returns the pointer to the character passed
> +				 * the enum, and two enums can not be placed
> +				 * back to back without something in between.
> +				 * We can skip that something in between.
> +				 */
> +				continue;

Maybe I'm becoming a bit paranoid, what I worried was like this:

  ENUM1\"ENUM2\"

In this case, it skips the backslash and makes quotation effective..


> +			}
> +		skip_more:
> +			do {
> +				ptr++;
> +			} while (isalnum(*ptr) || *ptr == '_');
> +			/*
> +			 * If what comes after this variable is a '.' or
> +			 * '->' then we can continue to ignore that string.
> +			 */
> +			if (*ptr == '.' || (ptr[0] == '-' && ptr[1] == '>')) {
> +				ptr += *ptr == '.' ? 1 : 2;
> +				goto skip_more;
> +			}
> +			/*
> +			 * Once again, we can skip the delimiter that came
> +			 * after the string.
> +			 */
> +			continue;
> +		}
> +	}
> +}
> +
> +void trace_event_enum_update(struct trace_enum_map **map, int len)
> +{
> +	struct ftrace_event_call *call, *p;
> +	const char *last_system = NULL;
> +	int last_i;
> +	int i;
> +
> +	down_write(&trace_event_sem);
> +	list_for_each_entry_safe(call, p, &ftrace_events, list) {
> +		/* events are usually grouped together with systems */
> +		if (!last_system || call->class->system != last_system) {

I think simply checking "call->class->system != last_system" would work.

Thanks,
Namhyung


> +			last_i = 0;
> +			last_system = call->class->system;
> +		}
> +
> +		for (i = last_i; i < len; i++) {
> +			if (call->class->system == map[i]->system) {
> +				/* Save the first system if need be */
> +				if (!last_i)
> +					last_i = i;
> +				update_event_printk(call, map[i]);
> +			}
> +		}
> +	}
> +	up_write(&trace_event_sem);
> +}
> +
>  static struct ftrace_event_file *
>  trace_create_new_event(struct ftrace_event_call *call,
>  		       struct trace_array *tr)
> -- 
> 2.1.4
> 
>