From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751031AbbDFEzf (ORCPT ); Mon, 6 Apr 2015 00:55:35 -0400 Received: from mail-pd0-f181.google.com ([209.85.192.181]:32812 "EHLO mail-pd0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750729AbbDFEzd (ORCPT ); Mon, 6 Apr 2015 00:55:33 -0400 Date: Mon, 6 Apr 2015 13:54:33 +0900 From: Namhyung Kim To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Andrew Morton , Arnaldo Carvalho de Melo , Masami Hiramatsu , Mathieu Desnoyers , Guilherme Cox , Tony Luck , Xie XiuQi Subject: Re: [PATCH 07/18 v3] tracing: Add TRACE_DEFINE_ENUM() macro to map enums to their values Message-ID: <20150406045433.GB15878@danjae.kornet> References: <20150403013802.220157513@goodmis.org> <20150403014123.997385206@goodmis.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150403014123.997385206@goodmis.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Steve, On Thu, Apr 02, 2015 at 09:38:09PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Red Hat)" > > 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 > Cc: Tony Luck > Cc: Xie XiuQi > Signed-off-by: Steven Rostedt > --- [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 > >