LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Daniel Bristot de Oliveira <bristot@redhat.com>,
	linux-kernel@vger.kernel.org, williams@redhat.com,
	daniel@bristot.me, Ingo Molnar <mingo@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Matthias Kaehlcke <mka@chromium.org>,
	"Joel Fernandes (Google)" <joel@joelfernandes.org>,
	Frederic Weisbecker <frederic@kernel.org>,
	Yangtao Li <tiny.windzz@gmail.com>,
	Tommaso Cucinotta <tommaso.cucinotta@santannapisa.it>
Subject: Re: [RFC 2/3] preempt_tracer: Disable IRQ while starting/stopping due to a preempt_counter change
Date: Wed, 29 May 2019 08:39:30 -0400	[thread overview]
Message-ID: <20190529083930.5541130e@oasis.local.home> (raw)
In-Reply-To: <20190529102038.GO2623@hirez.programming.kicks-ass.net>

On Wed, 29 May 2019 12:20:38 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, May 29, 2019 at 11:40:34AM +0200, Daniel Bristot de Oliveira wrote:
> > On 29/05/2019 10:33, Peter Zijlstra wrote:  
> > > On Tue, May 28, 2019 at 05:16:23PM +0200, Daniel Bristot de Oliveira wrote:  
> > >> The preempt_disable/enable tracepoint only traces in the disable <-> enable
> > >> case, which is correct. But think about this case:
> > >>
> > >> ---------------------------- %< ------------------------------
> > >> 	THREAD					IRQ
> > >> 	   |					 |
> > >> preempt_disable() {
> > >>     __preempt_count_add(1)  
> > >> 	------->	    smp_apic_timer_interrupt() {  
> > >> 				preempt_disable()
> > >> 				    do not trace (preempt count >= 1)
> > >> 				    ....
> > >> 				preempt_enable()
> > >> 				    do not trace (preempt count >= 1)
> > >> 			    }
> > >>     trace_preempt_disable();
> > >> }  
> > >> ---------------------------- >% ------------------------------  
> > >>
> > >> The tracepoint will be skipped.  
> > > 
> > > .... for the IRQ. But IRQs are not preemptible anyway, so what the
> > > problem?  
> > 
> > 
> > right, they are.
> > 
> > exposing my problem in a more specific way:
> > 
> > To show in a model that an event always takes place with preemption disabled,
> > but not necessarily with IRQs disabled, it is worth having the preemption
> > disable events separated from IRQ disable ones.
> > 
> > The main reason is that, although IRQs disabled postpone the execution of the
> > scheduler, it is more pessimistic, as it also delays IRQs. So the more precise
> > the model is, the less pessimistic the analysis will be.  
> 
> I'm not sure I follow, IRQs disabled fully implies !preemptible. I don't
> see how the model would be more pessimistic than reality if it were to
> use this knowledge.
> 
> Any !0 preempt_count(), which very much includes (Hard)IRQ and SoftIRQ
> counts, means non-preemptible.

I believe I see what Daniel is talking about, but I hate the proposed
solution ;-)

First, if you care about real times that the CPU can't preempt
(preempt_count != 0 or interrupts disabled), then you want the
preempt_irqsoff tracer. The preempt_tracer is more academic where it
just shows you when we disable preemption via the counter. But even
with the preempt_irqsoff tracer you may not get the full length of time
due to the above explained race.

	__preempt_count_add(1) <-- CPU now prevents preemption

	<interrupt>
		---->
			trace_hardirqs_off() <-- Start preempt disable timer
			handler();
			trace_hardirqs_on() <-- Stop preempt disable timer (Diff A)
		<----
	trace_preempt_disable() <-- Start preempt disable timer

	[..]

	trace_preempt_enable() <-- Stop preempt disable timer (Diff B)

	__preempt_count_sub(1) <-- CPU re-enables preemption

The preempt_irqsoff tracer will break this into two parts: Diff A and
Diff B, when in reality, the total time the CPU prevented preemption
was A + B.

Note, we can have the same race if an interrupt came in just after
calling trace_preempt_enable() and before the __preempt_count_sub().

What I would recommend is adding a flag to the task_struct that gets
set before the __preempt_count_add() and cleared by the tracing
function. If an interrupt goes off during this time, it will start the
total time to record, and not end it on the trace_hardirqs_on() part.
Now since we set this flag before disabling preemption, what if we get
preempted before calling __preempt_count_add()?. Simple, have a hook in
the scheduler (just connect to the sched_switch tracepoint) that checks
that flag, and if it is set, it ends the preempt disable recording
time. Also on scheduling that task back in, if that flag is set, start
the preempt disable timer.

It may get a bit more complex, but we can contain that complexity
within the tracing code, and we don't have to do added irq disabling.

-- Steve

  reply	other threads:[~2019-05-29 12:39 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-28 15:16 [RFC 0/3] preempt_tracer: Fix preempt_disable tracepoint Daniel Bristot de Oliveira
2019-05-28 15:16 ` [RFC 1/3] softirq: Use preempt_latency_stop/start to trace preemption Daniel Bristot de Oliveira
2019-05-29  8:29   ` Peter Zijlstra
2019-05-29  9:30   ` Joel Fernandes
2019-05-29 12:22     ` Steven Rostedt
2019-06-04 10:39       ` Daniel Bristot de Oliveira
2019-06-04 12:01         ` Steven Rostedt
2019-05-28 15:16 ` [RFC 2/3] preempt_tracer: Disable IRQ while starting/stopping due to a preempt_counter change Daniel Bristot de Oliveira
2019-05-29  8:33   ` Peter Zijlstra
2019-05-29  9:40     ` Daniel Bristot de Oliveira
2019-05-29 10:20       ` Peter Zijlstra
2019-05-29 12:39         ` Steven Rostedt [this message]
2019-05-29 13:19           ` Peter Zijlstra
2019-05-29 13:29             ` Peter Zijlstra
2019-05-29 13:42             ` Steven Rostedt
2019-05-29 13:49               ` Peter Zijlstra
2019-05-29 13:58                 ` Steven Rostedt
2019-05-29 13:51         ` Daniel Bristot de Oliveira
2019-05-29 18:21           ` Peter Zijlstra
2019-06-04 10:20             ` Daniel Bristot de Oliveira
2019-05-31  7:47       ` Joel Fernandes
2019-06-04 10:12         ` Daniel Bristot de Oliveira
2019-06-04 15:01           ` Steven Rostedt
2019-06-05 15:16             ` Joel Fernandes
2019-05-28 15:16 ` [RFC 3/3] preempt_tracer: Use a percpu variable to control traceble calls Daniel Bristot de Oliveira
2019-05-29  8:41   ` Peter Zijlstra
2019-05-29  9:48     ` Daniel Bristot de Oliveira

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190529083930.5541130e@oasis.local.home \
    --to=rostedt@goodmis.org \
    --cc=bristot@redhat.com \
    --cc=daniel@bristot.me \
    --cc=frederic@kernel.org \
    --cc=joel@joelfernandes.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=mka@chromium.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=tiny.windzz@gmail.com \
    --cc=tommaso.cucinotta@santannapisa.it \
    --cc=williams@redhat.com \
    --subject='Re: [RFC 2/3] preempt_tracer: Disable IRQ while starting/stopping due to a preempt_counter change' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).