Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753991AbdIDToj (ORCPT ); Mon, 4 Sep 2017 15:44:39 -0400 Received: from merlin.infradead.org ([205.233.59.134]:47368 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753953AbdIDTog (ORCPT ); Mon, 4 Sep 2017 15:44:36 -0400 Date: Mon, 4 Sep 2017 21:44:26 +0200 From: Peter Zijlstra To: Joel Fernandes Cc: LKML , Steven Rostedt , kernel-team@android.com, Ingo Molnar , Byungchul Park , Tejun Heo Subject: Re: [PATCH 2/2] tracing: Add support for critical section events Message-ID: <20170904194426.GD17526@worktop.programming.kicks-ass.net> References: <20170903085051.6348-1-joelaf@google.com> <20170903085051.6348-3-joelaf@google.com> <20170904075614.bjkkrgyv2dpz7x5v@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.22.1 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2744 Lines: 59 On Mon, Sep 04, 2017 at 12:04:44PM -0700, Joel Fernandes wrote: > Hi Peter, > > On Mon, Sep 4, 2017 at 12:56 AM, Peter Zijlstra wrote: > > On Sun, Sep 03, 2017 at 01:50:51AM -0700, Joel Fernandes wrote: > >> Critical section trace events can be used for tracing the start and > >> end of a critical section which can be used by a trace viewer like > >> systrace to graphically view the start and end of a critical section and > >> correlate them with latencies and scheduling issues. > >> > >> Reason for starting critical section: > >> Either preemption or interrupts were turned off. > >> Reason for ending critical section: > >> Both preemption and interrupts are now turned back on. > > > > Please don't use the name critical section for this. > > I can change the name to something else, but at the moment I can't > think of anything better. Could you suggest a better name? Also btw, > 'critical timings' is the terminology used within the irqsoff tracer > so this is in line with that. So 'critical section' is what some mis-guided people call the locked region of a lock :-) Using it for something else is prone to cause more confusion... I would simply call them what they are: irq_disable,irq_enable preempt_disable,preempt_enable. > > Also IRQ and preempt already have a gazillion trace hooks, why do we need more? > > The goal of the patch is not to add more hooks, but to make it > possible for userspace to use the trace-events mechanism to see these > events, so that these events can be seen along with other trace events > when using event tracing, using a convenient trace event interface > which can be enabled by userspace. But only when you're already building a debug kernel and we already have these irq/preempt trace things in, right? > This makes it possible to visually > see these events as a timeline along with other kernel and userspace > events. See more description in my coverletter [1] and a screenshot of > how this is used [2] by the systrace trace viewer. These patches make > it possible for this. Yeah, for some reason these graphics things don't work for me... And regular traces already have the irq-off and preempt-depth column, which typically is enough. But I suppose I can see the value of allowing explicit events for them. > Also, this work is along the same lines of what we discussed in a > recent conference about adding irqsoff and preemptoff as real trace > events, and then using synthetic events (which can combine start and > end event into a single event) to redo the irqsoff tracer, so in that > sense it is in the right direction. Synthetic events is some time away > though from being merged AFAIK. I have no memories of that, but that sounds OK :-)