From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934506AbdIYKPi (ORCPT ); Mon, 25 Sep 2017 06:15:38 -0400 Received: from smtprelay0039.hostedemail.com ([216.40.44.39]:48848 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933052AbdIYKPg (ORCPT ); Mon, 25 Sep 2017 06:15:36 -0400 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:1183:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1534:1543:1593:1594:1605:1711:1730:1747:1777:1792:1981:2194:2199:2376:2393:2553:2559:2562:2692:2897:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4250:4605:5007:6261:6755:7875:7903:10004:10400:10848:10967:11026:11232:11233:11473:11658:11914:12043:12219:12294:12438:12555:12663:12740:12760:12895:13255:13439:14096:14097:14181:14659:14721:21080:21324:21433:21451:21611:21627:30054:30074:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: alley74_4db3c124abf51 X-Filterd-Recvd-Size: 4687 Date: Mon, 25 Sep 2017 06:15:28 -0400 From: Steven Rostedt To: Peter Zijlstra Cc: Joel Fernandes , LKML , kernel-team@android.com Subject: Re: [PATCH v6 2/2] tracing: Add support for preempt and irq enable/disable events Message-ID: <20170925061528.09082957@vmware.local.home> In-Reply-To: <20170925083432.jvaewlsrb46wjmjj@hirez.programming.kicks-ass.net> References: <20170922015024.16123-1-joelaf@google.com> <20170922015024.16123-3-joelaf@google.com> <20170922090229.rdicci6emtyffqdn@hirez.programming.kicks-ass.net> <20170925083432.jvaewlsrb46wjmjj@hirez.programming.kicks-ass.net> X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; 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 Mon, 25 Sep 2017 10:34:32 +0200 Peter Zijlstra wrote: > > > > grep-1041 [002] d..1 80.363455: preempt_disable: > > caller=_raw_spin_lock_irqsave+0x1d/0x40 > > parent=add_wait_queue+0x15/0x50 > > I suppose that sort-of makes sense for the preempt-tracer, but its a > weird thing for a generic tracepoint. I still find it very useful, even as a tracepoint. > > > >> void start_critical_timings(void) > > >> { > > >> + if (this_cpu_read(tracing_preempt_cpu)) > > >> + trace_preempt_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > >> + > > >> + if (this_cpu_read(tracing_irq_cpu)) > > >> + trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > >> + > > >> start_critical_timings_tracer(); > > >> } > > >> EXPORT_SYMBOL_GPL(start_critical_timings); > > >> > > >> void stop_critical_timings(void) > > >> { > > >> + if (this_cpu_read(tracing_preempt_cpu)) > > >> + trace_preempt_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > >> + > > >> + if (this_cpu_read(tracing_irq_cpu)) > > >> + trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > >> + > > >> stop_critical_timings_tracer(); > > >> } > > >> EXPORT_SYMBOL_GPL(stop_critical_timings); > > > > > > And I feel these yield somewhat odd semantics, does that need explaining > > > somewhere? > > > > Maybe I can add a comment here, if you prefer that. When you meant > > semantics, do you mean 'critical' vs 'atomic' thing or do you mean the > > semantics/context of how this function is supposed to be used? > > I would add the comment to the tracepoint definition. > > On semantics, the whole stop/start excludes a fair bunch of walltime > from our measurement, I feel that needs to be called out and enumerated > (when time goes missing and why). > > Given that the idle thread runs with preempt-off I understand its > purpose from the POV from the preempt-tracer, but its 'weird' behaviour > if you're looking at it from a pure tracepoint pov. You mean you want to trace all calls to preempt and irq off even if preempt and irqs are already off? > > > >> void trace_preempt_off(unsigned long a0, unsigned long a1) > > >> { > > >> + if (this_cpu_read(tracing_preempt_cpu)) > > >> + return; > > >> + > > >> + this_cpu_write(tracing_preempt_cpu, 1); > > >> + > > >> + trace_preempt_disable_rcuidle(a0, a1); > > >> tracer_preempt_off(a0, a1); > > >> } > > >> #endif > > > > > > And here you assume things like double on / double off don't happen, > > > which might well be so, but does seem somewhat fragile. > > > > > > > We are handling the cases where these functions might be called twice, > > but we are only interested in the first time they're called. I caught > > a dead lock happen when I didn't add such protection to > > trace_hardirqs_off so I added to these to the trace_hardirqs* and > > trace_preempt* ones as well to just to be extra safe and keep it > > consistent. Hope I understood your concern correctly, if not please > > let me know, thanks. > > Under what conditions where they called twice? That seems like something > that should not happen to begin with. Esp the one I left quoted above, > disabling when its already disabled sounds like fail. So please provide > more details on the scenario you're working around. I'm a little confused by this too. -- Steve