* [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' @ 2016-02-26 13:14 Taeung Song 2016-02-26 13:57 ` [RFC] " Arnaldo Carvalho de Melo 0 siblings, 1 reply; 10+ messages in thread From: Taeung Song @ 2016-02-26 13:14 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Namhyung Kim, Taeung Song, Thomas Gleixner, Lai Jiangshan There is a problem about duplicated variable name i.e. # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format name: sys_enter_io_getevents ID: 739 format: field:unsigned short common_type; offset:0; size:2; signed:0; field:unsigned char common_flags; offset:2; size:1; signed:0; field:unsigned char common_preempt_count; offset:3; size:1; signed:0; field:int common_pid;offset:4;size:4;signed:1; field:int nr; offset:8; size:4; signed:1; field:aio_context_t ctx_id; offset:16; size:8; signed:0; field:long min_nr; offset:24; size:8; signed:0; field:long nr; offset:32; size:8; signed:0; field:struct io_event * events; offset:40; size:8; signed:0; field:struct timespec * timeout; offset:48; size:8; signed:0; print fmt: "ctx_id: 0x%08lx, min_nr: 0x%08lx, nr: 0x%08lx, events: 0x%08lx, timeout: 0x%08lx", ((unsigned long)(REC->ctx_id)), ((unsigned long)(REC->min_nr)), ((unsigned long)(REC->nr)), ((unsigned long)(REC->events)), ((unsigned long)(REC->timeout)) As above 'int nr;' and 'long nr;' variables have duplicated name so problems are occurred in perf-script i.e. # perf record -e syscalls:* # perf script -g python # perf script -s perf-script.py File "perf-script.py", line 8694 def syscalls__sys_enter_io_getevents(event_name, context, common_cpu, SyntaxError: duplicate argument 'nr' in function definition Error running python script perf-script.py As above, problems about duplicated argument occurred. Not only sys_enter_io_getevent() but also sys_enter_io_submit() have relevevance to this problem. So rename a variable 'nr' to '__syscall_nr' for system call number in print_syscall_enter() and etc. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Lai Jiangshan <jiangshanlai@gmail.com> Signed-off-by: Taeung Song <treeze.taeung@gmail.com> --- kernel/trace/trace.h | 4 ++-- kernel/trace/trace_syscalls.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index 8414fa4..98b3c66 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -88,13 +88,13 @@ enum trace_type { */ struct syscall_trace_enter { struct trace_entry ent; - int nr; + int __syscall_nr; unsigned long args[]; }; struct syscall_trace_exit { struct trace_entry ent; - int nr; + int __syscall_nr; long ret; }; diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c index 0655afb..90bb468 100644 --- a/kernel/trace/trace_syscalls.c +++ b/kernel/trace/trace_syscalls.c @@ -118,7 +118,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags, int i, syscall; trace = (typeof(trace))ent; - syscall = trace->nr; + syscall = trace->__syscall_nr; entry = syscall_nr_to_meta(syscall); if (!entry) @@ -164,7 +164,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags, struct syscall_metadata *entry; trace = (typeof(trace))ent; - syscall = trace->nr; + syscall = trace->__syscall_nr; entry = syscall_nr_to_meta(syscall); if (!entry) { @@ -261,7 +261,7 @@ static int __init syscall_enter_define_fields(struct trace_event_call *call) int i; int offset = offsetof(typeof(trace), args); - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); + ret = trace_define_field(call, SYSCALL_FIELD(int, __syscall_nr), FILTER_OTHER); if (ret) return ret; @@ -281,7 +281,7 @@ static int __init syscall_exit_define_fields(struct trace_event_call *call) struct syscall_trace_exit trace; int ret; - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); + ret = trace_define_field(call, SYSCALL_FIELD(int, __syscall_nr), FILTER_OTHER); if (ret) return ret; @@ -332,7 +332,7 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id) return; entry = ring_buffer_event_data(event); - entry->nr = syscall_nr; + entry->__syscall_nr = syscall_nr; syscall_get_arguments(current, regs, 0, sys_data->nb_args, entry->args); event_trigger_unlock_commit(trace_file, buffer, event, entry, @@ -378,7 +378,7 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret) return; entry = ring_buffer_event_data(event); - entry->nr = syscall_nr; + entry->__syscall_nr = syscall_nr; entry->ret = syscall_get_return_value(current, regs); event_trigger_unlock_commit(trace_file, buffer, event, entry, @@ -579,7 +579,7 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id) if (!rec) return; - rec->nr = syscall_nr; + rec->__syscall_nr = syscall_nr; syscall_get_arguments(current, regs, 0, sys_data->nb_args, (unsigned long *)&rec->args); perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, head, NULL); @@ -652,7 +652,7 @@ static void perf_syscall_exit(void *ignore, struct pt_regs *regs, long ret) if (!rec) return; - rec->nr = syscall_nr; + rec->__syscall_nr = syscall_nr; rec->ret = syscall_get_return_value(current, regs); perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, head, NULL); } -- 2.5.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-26 13:14 [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Taeung Song @ 2016-02-26 13:57 ` Arnaldo Carvalho de Melo 2016-02-26 18:23 ` Steven Rostedt 2016-02-27 15:13 ` [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Peter Zijlstra 0 siblings, 2 replies; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-02-26 13:57 UTC (permalink / raw) To: Steven Rostedt, Peter Zijlstra, Ingo Molnar Cc: Taeung Song, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan Em Fri, Feb 26, 2016 at 10:14:06PM +0900, Taeung Song escreveu: > There is a problem about duplicated variable name i.e. > > # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format > name: sys_enter_io_getevents > ID: 739 > format: Steven, what do you think? Should we break this ABI while disambiguating the 'nr' field, using '__syscall_nr' in an attempt to use a name that is unlikely to be used by a real syscall argument name? If we stand by published ABIs, we should keep it written in stone and state that the first 'nr' means '__syscall_nr' while keeping it as-is, the change for 'perf trace' in that case is to do nothing, it work as-is, we have just to fix the python binding to do that rename. Perhaps we can live with that, to avoid having three different cases: !nr, nr and __syscall_nr. Ingo, Peter, have you guys followed this case? Summary: Some tracepoint have multiple fields with the same name, 'nr', the first one is a unique syscall ID, the other is a syscall argument: [root@jouet ~]# cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format name: sys_enter_io_getevents ID: 747 format: field:unsigned short common_type; offset:0; size:2; signed:0; field:unsigned char common_flags; offset:2; size:1; signed:0; field:unsigned char common_preempt_count; offset:3; size:1; signed:0; field:int common_pid; offset:4; size:4; signed:1; field:int nr; offset:8; size:4; signed:1; field:aio_context_t ctx_id; offset:16; size:8; signed:0; field:long min_nr; offset:24; size:8; signed:0; field:long nr; offset:32; size:8; signed:0; field:struct io_event * events; offset:40; size:8; signed:0; field:struct timespec * timeout; offset:48; size:8; signed:0; print fmt: "ctx_id: 0x%08lx, min_nr: 0x%08lx, nr: 0x%08lx, events: 0x%08lx, timeout: 0x%08lx", ((unsigned long)(REC->ctx_id)), ((unsigned long)(REC->min_nr)), ((unsigned long)(REC->nr)), ((unsigned long)(REC->events)), ((unsigned long)(REC->timeout)) [root@jouet ~]# - Arnaldo > field:unsigned short common_type; offset:0; size:2; signed:0; > field:unsigned char common_flags; offset:2; size:1; signed:0; > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > field:int common_pid;offset:4;size:4;signed:1; > field:int nr; offset:8; size:4; signed:1; > field:aio_context_t ctx_id; offset:16; size:8; signed:0; > field:long min_nr; offset:24; size:8; signed:0; > field:long nr; offset:32; size:8; signed:0; > field:struct io_event * events; offset:40; size:8; signed:0; > field:struct timespec * timeout; offset:48; size:8; signed:0; > > print fmt: "ctx_id: 0x%08lx, min_nr: 0x%08lx, nr: 0x%08lx, > events: 0x%08lx, timeout: 0x%08lx", ((unsigned long)(REC->ctx_id)), > ((unsigned long)(REC->min_nr)), ((unsigned long)(REC->nr)), > ((unsigned long)(REC->events)), ((unsigned long)(REC->timeout)) > > As above 'int nr;' and 'long nr;' variables have > duplicated name so problems are occurred in perf-script i.e. > > # perf record -e syscalls:* > # perf script -g python > # perf script -s perf-script.py > File "perf-script.py", line 8694 > def syscalls__sys_enter_io_getevents(event_name, context, common_cpu, > SyntaxError: duplicate argument 'nr' in function definition > Error running python script perf-script.py > > As above, problems about duplicated argument occurred. > Not only sys_enter_io_getevent() but also sys_enter_io_submit() > have relevevance to this problem. > > So rename a variable 'nr' to '__syscall_nr' for system call number > in print_syscall_enter() and etc. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Lai Jiangshan <jiangshanlai@gmail.com> > Signed-off-by: Taeung Song <treeze.taeung@gmail.com> > --- > kernel/trace/trace.h | 4 ++-- > kernel/trace/trace_syscalls.c | 16 ++++++++-------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index 8414fa4..98b3c66 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -88,13 +88,13 @@ enum trace_type { > */ > struct syscall_trace_enter { > struct trace_entry ent; > - int nr; > + int __syscall_nr; > unsigned long args[]; > }; > > struct syscall_trace_exit { > struct trace_entry ent; > - int nr; > + int __syscall_nr; > long ret; > }; > > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c > index 0655afb..90bb468 100644 > --- a/kernel/trace/trace_syscalls.c > +++ b/kernel/trace/trace_syscalls.c > @@ -118,7 +118,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags, > int i, syscall; > > trace = (typeof(trace))ent; > - syscall = trace->nr; > + syscall = trace->__syscall_nr; > entry = syscall_nr_to_meta(syscall); > > if (!entry) > @@ -164,7 +164,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags, > struct syscall_metadata *entry; > > trace = (typeof(trace))ent; > - syscall = trace->nr; > + syscall = trace->__syscall_nr; > entry = syscall_nr_to_meta(syscall); > > if (!entry) { > @@ -261,7 +261,7 @@ static int __init syscall_enter_define_fields(struct trace_event_call *call) > int i; > int offset = offsetof(typeof(trace), args); > > - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); > + ret = trace_define_field(call, SYSCALL_FIELD(int, __syscall_nr), FILTER_OTHER); > if (ret) > return ret; > > @@ -281,7 +281,7 @@ static int __init syscall_exit_define_fields(struct trace_event_call *call) > struct syscall_trace_exit trace; > int ret; > > - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); > + ret = trace_define_field(call, SYSCALL_FIELD(int, __syscall_nr), FILTER_OTHER); > if (ret) > return ret; > > @@ -332,7 +332,7 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id) > return; > > entry = ring_buffer_event_data(event); > - entry->nr = syscall_nr; > + entry->__syscall_nr = syscall_nr; > syscall_get_arguments(current, regs, 0, sys_data->nb_args, entry->args); > > event_trigger_unlock_commit(trace_file, buffer, event, entry, > @@ -378,7 +378,7 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret) > return; > > entry = ring_buffer_event_data(event); > - entry->nr = syscall_nr; > + entry->__syscall_nr = syscall_nr; > entry->ret = syscall_get_return_value(current, regs); > > event_trigger_unlock_commit(trace_file, buffer, event, entry, > @@ -579,7 +579,7 @@ static void perf_syscall_enter(void *ignore, struct pt_regs *regs, long id) > if (!rec) > return; > > - rec->nr = syscall_nr; > + rec->__syscall_nr = syscall_nr; > syscall_get_arguments(current, regs, 0, sys_data->nb_args, > (unsigned long *)&rec->args); > perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, head, NULL); > @@ -652,7 +652,7 @@ static void perf_syscall_exit(void *ignore, struct pt_regs *regs, long ret) > if (!rec) > return; > > - rec->nr = syscall_nr; > + rec->__syscall_nr = syscall_nr; > rec->ret = syscall_get_return_value(current, regs); > perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, head, NULL); > } > -- > 2.5.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-26 13:57 ` [RFC] " Arnaldo Carvalho de Melo @ 2016-02-26 18:23 ` Steven Rostedt 2016-02-26 19:03 ` Arnaldo Carvalho de Melo ` (2 more replies) 2016-02-27 15:13 ` [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Peter Zijlstra 1 sibling, 3 replies; 10+ messages in thread From: Steven Rostedt @ 2016-02-26 18:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra, Ingo Molnar, Taeung Song, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan On Fri, 26 Feb 2016 10:57:13 -0300 Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > Em Fri, Feb 26, 2016 at 10:14:06PM +0900, Taeung Song escreveu: > > There is a problem about duplicated variable name i.e. > > > > # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format > > name: sys_enter_io_getevents > > ID: 739 > > format: > > Steven, what do you think? > > Should we break this ABI while disambiguating the 'nr' field, using > '__syscall_nr' in an attempt to use a name that is unlikely to be used > by a real syscall argument name? > > If we stand by published ABIs, we should keep it written in stone and > state that the first 'nr' means '__syscall_nr' while keeping it as-is, > the change for 'perf trace' in that case is to do nothing, it work > as-is, we have just to fix the python binding to do that rename. ABIs only matter if they break something, and people complain. Linus has been somewhat accepting of us fixing those tools that break and we push out the fixes. If an ABI breaks in the forest and nobody is around to complain about it, did it really break? I would say, lets make the change and fix perf. If people complain, we send them the fixes for their tools. If they need the distros to have the fixes, then let the change be reverted, and we wait till the distros have the update (this may take a few years), then re-submit. This worked for me to get rid of padding that was in every trace event. The change was reverted, I fixed the tools that broke, waited till all the major distros had the updates. And resubmitted the change. Linus took it. > > Perhaps we can live with that, to avoid having three different cases: > !nr, nr and __syscall_nr. We could, do this as well. Want me to add something to event-parse? > > Ingo, Peter, have you guys followed this case? > > Summary: Some tracepoint have multiple fields with the same name, 'nr', > the first one is a unique syscall ID, the other is a syscall > argument: > > [root@jouet ~]# cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format > name: sys_enter_io_getevents > ID: 747 > format: > field:unsigned short common_type; offset:0; size:2; signed:0; > field:unsigned char common_flags; offset:2; size:1; signed:0; > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > field:int common_pid; offset:4; size:4; signed:1; > > field:int nr; offset:8; size:4; signed:1; > field:aio_context_t ctx_id; offset:16; size:8; signed:0; > field:long min_nr; offset:24; size:8; signed:0; > field:long nr; offset:32; size:8; signed:0; > field:struct io_event * events; offset:40; size:8; signed:0; > field:struct timespec * timeout; offset:48; size:8; signed:0; > > print fmt: "ctx_id: 0x%08lx, min_nr: 0x%08lx, nr: 0x%08lx, events: 0x%08lx, timeout: 0x%08lx", ((unsigned long)(REC->ctx_id)), ((unsigned long)(REC->min_nr)), ((unsigned long)(REC->nr)), ((unsigned long)(REC->events)), ((unsigned long)(REC->timeout)) > [root@jouet ~]# > BTW, here's a less intrusive change, because honestly, I hate the kernel structure having underscores in the name. This could be signed off by Taeung Song and myself. -- Steve diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c index 0655afbea83f..d1663083d903 100644 --- a/kernel/trace/trace_syscalls.c +++ b/kernel/trace/trace_syscalls.c @@ -186,11 +186,11 @@ print_syscall_exit(struct trace_iterator *iter, int flags, extern char *__bad_type_size(void); -#define SYSCALL_FIELD(type, name) \ - sizeof(type) != sizeof(trace.name) ? \ +#define SYSCALL_FIELD(type, field, name) \ + sizeof(type) != sizeof(trace.field) ? \ __bad_type_size() : \ - #type, #name, offsetof(typeof(trace), name), \ - sizeof(trace.name), is_signed_type(type) + #type, #name, offsetof(typeof(trace), field), \ + sizeof(trace.field), is_signed_type(type) static int __init __set_enter_print_fmt(struct syscall_metadata *entry, char *buf, int len) @@ -261,7 +261,8 @@ static int __init syscall_enter_define_fields(struct trace_event_call *call) int i; int offset = offsetof(typeof(trace), args); - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); + ret = trace_define_field(call, SYSCALL_FIELD(int, nr, __syscall_nr), + FILTER_OTHER); if (ret) return ret; @@ -281,11 +282,12 @@ static int __init syscall_exit_define_fields(struct trace_event_call *call) struct syscall_trace_exit trace; int ret; - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); + ret = trace_define_field(call, SYSCALL_FIELD(int, nr, __syscall_nr), + FILTER_OTHER); if (ret) return ret; - ret = trace_define_field(call, SYSCALL_FIELD(long, ret), + ret = trace_define_field(call, SYSCALL_FIELD(long, ret, ret), FILTER_OTHER); return ret; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-26 18:23 ` Steven Rostedt @ 2016-02-26 19:03 ` Arnaldo Carvalho de Melo 2016-02-27 16:10 ` Taeung Song 2016-03-05 8:13 ` [tip:perf/core] tracing/syscalls: Rename "/format" tracepoint field name "nr" to "__syscall_nr: tip-bot for Taeung Song 2 siblings, 0 replies; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-02-26 19:03 UTC (permalink / raw) To: Steven Rostedt Cc: Peter Zijlstra, Ingo Molnar, Taeung Song, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan Em Fri, Feb 26, 2016 at 01:23:01PM -0500, Steven Rostedt escreveu: > On Fri, 26 Feb 2016 10:57:13 -0300 Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > If we stand by published ABIs, we should keep it written in stone and > > state that the first 'nr' means '__syscall_nr' while keeping it as-is, > > the change for 'perf trace' in that case is to do nothing, it work > > as-is, we have just to fix the python binding to do that rename. <SNIP> > > Perhaps we can live with that, to avoid having three different cases: > > !nr, nr and __syscall_nr. > > We could, do this as well. Want me to add something to event-parse? You mean do nothing in the kernel? Suits me fine, the fix would be just on the tools/perf/ code, where it generates the python skeletons, I can do that, as I guess Taeung is sleep at this point :-) - Arnaldo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-26 18:23 ` Steven Rostedt 2016-02-26 19:03 ` Arnaldo Carvalho de Melo @ 2016-02-27 16:10 ` Taeung Song 2016-02-27 18:34 ` Steven Rostedt 2016-03-05 8:13 ` [tip:perf/core] tracing/syscalls: Rename "/format" tracepoint field name "nr" to "__syscall_nr: tip-bot for Taeung Song 2 siblings, 1 reply; 10+ messages in thread From: Taeung Song @ 2016-02-27 16:10 UTC (permalink / raw) To: Steven Rostedt Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan Hi, Steven On 02/27/2016 03:23 AM, Steven Rostedt wrote: > On Fri, 26 Feb 2016 10:57:13 -0300 > Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > >> Em Fri, Feb 26, 2016 at 10:14:06PM +0900, Taeung Song escreveu: >>> There is a problem about duplicated variable name i.e. >>> >>> # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format >>> name: sys_enter_io_getevents >>> ID: 739 >>> format: >> >> Steven, what do you think? >> >> Should we break this ABI while disambiguating the 'nr' field, using >> '__syscall_nr' in an attempt to use a name that is unlikely to be used >> by a real syscall argument name? >> >> If we stand by published ABIs, we should keep it written in stone and >> state that the first 'nr' means '__syscall_nr' while keeping it as-is, >> the change for 'perf trace' in that case is to do nothing, it work >> as-is, we have just to fix the python binding to do that rename. > > ABIs only matter if they break something, and people complain. Linus > has been somewhat accepting of us fixing those tools that break and we > push out the fixes. If an ABI breaks in the forest and nobody is around > to complain about it, did it really break? > > I would say, lets make the change and fix perf. If people complain, we > send them the fixes for their tools. If they need the distros to have > the fixes, then let the change be reverted, and we wait till the > distros have the update (this may take a few years), then re-submit. > > This worked for me to get rid of padding that was in every trace event. > The change was reverted, I fixed the tools that broke, waited till all > the major distros had the updates. And resubmitted the change. Linus > took it. > > >> >> Perhaps we can live with that, to avoid having three different cases: >> !nr, nr and __syscall_nr. > > We could, do this as well. Want me to add something to event-parse? > >> >> Ingo, Peter, have you guys followed this case? >> >> Summary: Some tracepoint have multiple fields with the same name, 'nr', >> the first one is a unique syscall ID, the other is a syscall >> argument: >> >> [root@jouet ~]# cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format >> name: sys_enter_io_getevents >> ID: 747 >> format: >> field:unsigned short common_type; offset:0; size:2; signed:0; >> field:unsigned char common_flags; offset:2; size:1; signed:0; >> field:unsigned char common_preempt_count; offset:3; size:1; signed:0; >> field:int common_pid; offset:4; size:4; signed:1; >> >> field:int nr; offset:8; size:4; signed:1; >> field:aio_context_t ctx_id; offset:16; size:8; signed:0; >> field:long min_nr; offset:24; size:8; signed:0; >> field:long nr; offset:32; size:8; signed:0; >> field:struct io_event * events; offset:40; size:8; signed:0; >> field:struct timespec * timeout; offset:48; size:8; signed:0; >> >> print fmt: "ctx_id: 0x%08lx, min_nr: 0x%08lx, nr: 0x%08lx, events: 0x%08lx, timeout: 0x%08lx", ((unsigned long)(REC->ctx_id)), ((unsigned long)(REC->min_nr)), ((unsigned long)(REC->nr)), ((unsigned long)(REC->events)), ((unsigned long)(REC->timeout)) >> [root@jouet ~]# >> > > BTW, here's a less intrusive change, because honestly, I hate the > kernel structure having underscores in the name. > > This could be signed off by Taeung Song and myself. > > -- Steve > > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c > index 0655afbea83f..d1663083d903 100644 > --- a/kernel/trace/trace_syscalls.c > +++ b/kernel/trace/trace_syscalls.c > @@ -186,11 +186,11 @@ print_syscall_exit(struct trace_iterator *iter, int flags, > > extern char *__bad_type_size(void); > > -#define SYSCALL_FIELD(type, name) \ > - sizeof(type) != sizeof(trace.name) ? \ > +#define SYSCALL_FIELD(type, field, name) \ > + sizeof(type) != sizeof(trace.field) ? \ > __bad_type_size() : \ > - #type, #name, offsetof(typeof(trace), name), \ > - sizeof(trace.name), is_signed_type(type) > + #type, #name, offsetof(typeof(trace), field), \ > + sizeof(trace.field), is_signed_type(type) > > static int __init > __set_enter_print_fmt(struct syscall_metadata *entry, char *buf, int len) > @@ -261,7 +261,8 @@ static int __init syscall_enter_define_fields(struct trace_event_call *call) > int i; > int offset = offsetof(typeof(trace), args); > > - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); > + ret = trace_define_field(call, SYSCALL_FIELD(int, nr, __syscall_nr), > + FILTER_OTHER); > if (ret) > return ret; > > @@ -281,11 +282,12 @@ static int __init syscall_exit_define_fields(struct trace_event_call *call) > struct syscall_trace_exit trace; > int ret; > > - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); > + ret = trace_define_field(call, SYSCALL_FIELD(int, nr, __syscall_nr), > + FILTER_OTHER); > if (ret) > return ret; > > - ret = trace_define_field(call, SYSCALL_FIELD(long, ret), > + ret = trace_define_field(call, SYSCALL_FIELD(long, ret, ret), > FILTER_OTHER); > > return ret; > Would you mean to avoid struct syscall_trace_enter or _exit has '__syscall_nr' variable ? So not including a portion of this patch([PATCH v3 1/2] tracing/syscalls: ...) diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index 8414fa4..98b3c66 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -88,13 +88,13 @@ enum trace_type { */ struct syscall_trace_enter { struct trace_entry ent; - int nr; + int __syscall_nr; unsigned long args[]; }; struct syscall_trace_exit { struct trace_entry ent; - int nr; + int __syscall_nr; long ret; }; I got it :-) In conclusion, output of format (/sys/kernel/debug/tracing/events/syscalls/*/format) has __syscall_nr for syscall number but the kernel structure 'syscall_trace_enter' and 'syscall_trace_exit' have not __syscall_nr variable. Is it right ? I'll resend modified patch after testing new patch you suggest soon. Thanks, Taeung ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-27 16:10 ` Taeung Song @ 2016-02-27 18:34 ` Steven Rostedt 2016-02-28 7:43 ` Taeung Song 0 siblings, 1 reply; 10+ messages in thread From: Steven Rostedt @ 2016-02-27 18:34 UTC (permalink / raw) To: Taeung Song Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan On Sun, 28 Feb 2016 01:10:19 +0900 Taeung Song <treeze.taeung@gmail.com> wrote: > I got it :-) > In conclusion, output of format > (/sys/kernel/debug/tracing/events/syscalls/*/format) > has __syscall_nr for syscall number but the kernel structure > 'syscall_trace_enter' and 'syscall_trace_exit' have not __syscall_nr > variable. > Is it right ? Correct. Just change the output of the format file. Not the data structures in the kernel. Thanks, -- Steve ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-27 18:34 ` Steven Rostedt @ 2016-02-28 7:43 ` Taeung Song 0 siblings, 0 replies; 10+ messages in thread From: Taeung Song @ 2016-02-28 7:43 UTC (permalink / raw) To: Steven Rostedt Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan On 02/28/2016 03:34 AM, Steven Rostedt wrote: > On Sun, 28 Feb 2016 01:10:19 +0900 > Taeung Song <treeze.taeung@gmail.com> wrote: > >> I got it :-) >> In conclusion, output of format >> (/sys/kernel/debug/tracing/events/syscalls/*/format) >> has __syscall_nr for syscall number but the kernel structure >> 'syscall_trace_enter' and 'syscall_trace_exit' have not __syscall_nr >> variable. >> Is it right ? > > Correct. Just change the output of the format file. Not the data > structures in the kernel. > > Thanks, > > -- Steve Thank you !! :-) Taeung ^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip:perf/core] tracing/syscalls: Rename "/format" tracepoint field name "nr" to "__syscall_nr: 2016-02-26 18:23 ` Steven Rostedt 2016-02-26 19:03 ` Arnaldo Carvalho de Melo 2016-02-27 16:10 ` Taeung Song @ 2016-03-05 8:13 ` tip-bot for Taeung Song 2 siblings, 0 replies; 10+ messages in thread From: tip-bot for Taeung Song @ 2016-03-05 8:13 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, rostedt, acme, peterz, namhyung, jolsa, mingo, jiangshanlai, linux-kernel, hpa, treeze.taeung Commit-ID: 026842d148b920dc28f0499ede4950dcb098d4d5 Gitweb: http://git.kernel.org/tip/026842d148b920dc28f0499ede4950dcb098d4d5 Author: Taeung Song <treeze.taeung@gmail.com> AuthorDate: Fri, 26 Feb 2016 13:23:01 -0500 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Mon, 29 Feb 2016 11:34:53 -0300 tracing/syscalls: Rename "/format" tracepoint field name "nr" to "__syscall_nr: Some tracepoint have multiple fields with the same name, "nr", the first one is a unique syscall ID, the other is a syscall argument: # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_io_getevents/format name: sys_enter_io_getevents ID: 747 format: field:unsigned short common_type; offset:0; size:2; signed:0; field:unsigned char common_flags; offset:2; size:1; signed:0; field:unsigned char common_preempt_count; offset:3; size:1; signed:0; field:int common_pid; offset:4; size:4; signed:1; field:int nr; offset:8; size:4; signed:1; field:aio_context_t ctx_id; offset:16; size:8; signed:0; field:long min_nr; offset:24; size:8; signed:0; field:long nr; offset:32; size:8; signed:0; field:struct io_event * events; offset:40; size:8; signed:0; field:struct timespec * timeout; offset:48; size:8; signed:0; print fmt: "ctx_id: 0x%08lx, min_nr: 0x%08lx, nr: 0x%08lx, events: 0x%08lx, timeout: 0x%08lx", ((unsigned long)(REC->ctx_id)), ((unsigned long)(REC->min_nr)), ((unsigned long)(REC->nr)), ((unsigned long)(REC->events)), ((unsigned long)(REC->timeout)) # Fix it by renaming the "/format" common tracepoint field "nr" to "__syscall_nr". Signed-off-by: Taeung Song <treeze.taeung@gmail.com> [ Do not rename the struct member, just the '/format' field name ] Signed-off-by: Steven Rostedt <rostedt@goodmis.org> Acked-by: Peter Zijlstra <peterz@infradead.org> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Lai Jiangshan <jiangshanlai@gmail.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/20160226132301.3ae065a4@gandalf.local.home Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- kernel/trace/trace_syscalls.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c index 0655afb..d166308 100644 --- a/kernel/trace/trace_syscalls.c +++ b/kernel/trace/trace_syscalls.c @@ -186,11 +186,11 @@ print_syscall_exit(struct trace_iterator *iter, int flags, extern char *__bad_type_size(void); -#define SYSCALL_FIELD(type, name) \ - sizeof(type) != sizeof(trace.name) ? \ +#define SYSCALL_FIELD(type, field, name) \ + sizeof(type) != sizeof(trace.field) ? \ __bad_type_size() : \ - #type, #name, offsetof(typeof(trace), name), \ - sizeof(trace.name), is_signed_type(type) + #type, #name, offsetof(typeof(trace), field), \ + sizeof(trace.field), is_signed_type(type) static int __init __set_enter_print_fmt(struct syscall_metadata *entry, char *buf, int len) @@ -261,7 +261,8 @@ static int __init syscall_enter_define_fields(struct trace_event_call *call) int i; int offset = offsetof(typeof(trace), args); - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); + ret = trace_define_field(call, SYSCALL_FIELD(int, nr, __syscall_nr), + FILTER_OTHER); if (ret) return ret; @@ -281,11 +282,12 @@ static int __init syscall_exit_define_fields(struct trace_event_call *call) struct syscall_trace_exit trace; int ret; - ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER); + ret = trace_define_field(call, SYSCALL_FIELD(int, nr, __syscall_nr), + FILTER_OTHER); if (ret) return ret; - ret = trace_define_field(call, SYSCALL_FIELD(long, ret), + ret = trace_define_field(call, SYSCALL_FIELD(long, ret, ret), FILTER_OTHER); return ret; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-26 13:57 ` [RFC] " Arnaldo Carvalho de Melo 2016-02-26 18:23 ` Steven Rostedt @ 2016-02-27 15:13 ` Peter Zijlstra 2016-02-29 14:21 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 10+ messages in thread From: Peter Zijlstra @ 2016-02-27 15:13 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Steven Rostedt, Ingo Molnar, Taeung Song, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan On Fri, Feb 26, 2016 at 10:57:13AM -0300, Arnaldo Carvalho de Melo wrote: > Ingo, Peter, have you guys followed this case? > > Summary: Some tracepoint have multiple fields with the same name, 'nr', > the first one is a unique syscall ID, the other is a syscall > argument: I'm all for pushing the limits on tracepoint ABI. I'm all for making it less rigid. So if we can get away with changing this under the promise of helping fixup fallout, I'm all for it. That means we can always just change tracepoints, and they're not really ABI at all. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' 2016-02-27 15:13 ` [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Peter Zijlstra @ 2016-02-29 14:21 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-02-29 14:21 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Taeung Song, linux-kernel, Jiri Olsa, Namhyung Kim, Thomas Gleixner, Lai Jiangshan Em Sat, Feb 27, 2016 at 04:13:57PM +0100, Peter Zijlstra escreveu: > On Fri, Feb 26, 2016 at 10:57:13AM -0300, Arnaldo Carvalho de Melo wrote: > > > Ingo, Peter, have you guys followed this case? > > > > Summary: Some tracepoint have multiple fields with the same name, 'nr', > > the first one is a unique syscall ID, the other is a syscall > > argument: > > I'm all for pushing the limits on tracepoint ABI. I'm all for making it > less rigid. So if we can get away with changing this under the promise > of helping fixup fallout, I'm all for it. > > That means we can always just change tracepoints, and they're not really > ABI at all. I'll take that as an Acked-by, and will apply Taeung/Steven's patch for the kernel + the builtin-trace.c change, Ok everybody? - Arnaldo ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-05 8:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-26 13:14 [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Taeung Song 2016-02-26 13:57 ` [RFC] " Arnaldo Carvalho de Melo 2016-02-26 18:23 ` Steven Rostedt 2016-02-26 19:03 ` Arnaldo Carvalho de Melo 2016-02-27 16:10 ` Taeung Song 2016-02-27 18:34 ` Steven Rostedt 2016-02-28 7:43 ` Taeung Song 2016-03-05 8:13 ` [tip:perf/core] tracing/syscalls: Rename "/format" tracepoint field name "nr" to "__syscall_nr: tip-bot for Taeung Song 2016-02-27 15:13 ` [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Peter Zijlstra 2016-02-29 14:21 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.