From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422886AbcBZSXI (ORCPT ); Fri, 26 Feb 2016 13:23:08 -0500 Received: from smtprelay0178.hostedemail.com ([216.40.44.178]:55362 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1422625AbcBZSXG (ORCPT ); Fri, 26 Feb 2016 13:23:06 -0500 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::,RULES_HIT:41:355:379:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1431:1437:1515:1516:1518:1535:1544:1593:1594:1605:1711:1730:1747:1777:1792:2393:2553:2559:2562:2895:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4117:4250:4321:4605:5007:6261:7875:7903:8603:8660:9163:10004:10848:10967:11026:11232:11658:11914:12043:12296:12438:12517:12519:12555:12740:13138:13148:13161:13229:13230:13231:13255:14096:14097:14659:21080:21212:30029:30045:30054:30070:30080:30083:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: care27_8f0431ce6d74c X-Filterd-Recvd-Size: 6115 Date: Fri, 26 Feb 2016 13:23:01 -0500 From: Steven Rostedt To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra , Ingo Molnar , Taeung Song , linux-kernel@vger.kernel.org, Jiri Olsa , Namhyung Kim , Thomas Gleixner , Lai Jiangshan Subject: Re: [RFC] Re: [PATCH v3 1/2] tracing/syscalls: Rename variable 'nr' to '__syscall_nr' Message-ID: <20160226132301.3ae065a4@gandalf.local.home> In-Reply-To: <20160226135713.GU8720@kernel.org> References: <1456492446-5896-1-git-send-email-treeze.taeung@gmail.com> <20160226135713.GU8720@kernel.org> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.29; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 26 Feb 2016 10:57:13 -0300 Arnaldo Carvalho de Melo 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;