public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: "H. Peter Anvin" <hpa@zytor.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Koki Sanagi <sanagi.koki@jp.fujitsu.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@elte.hu>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	nhorman@tuxdriver.com, scott.a.mcmillan@intel.com,
	laijs@cn.fujitsu.com, LKML <linux-kernel@vger.kernel.org>,
	eric.dumazet@gmail.com, kaneshige.kenji@jp.fujitsu.com,
	David Miller <davem@davemloft.net>,
	izumi.taku@jp.fujitsu.com, kosaki.motohiro@jp.fujitsu.com,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	"Luck, Tony" <tony.luck@intel.com>,
	Jason Baron <jbaron@redhat.com>
Subject: Re: [PATCH] tracing: Cleanup the convoluted softirq tracepoints
Date: Tue, 19 Oct 2010 18:41:27 -0400	[thread overview]
Message-ID: <20101019224126.GD3519@Krystal> (raw)
In-Reply-To: <4CBE122B.9020807@zytor.com>

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 10/19/2010 02:23 PM, Steven Rostedt wrote:
> > 
> > But it seemed that gcc for you inlined the code in the wrong spot.
> > Perhaps it's not a good idea to have the something like h - softirq_vec
> > in the parameter of the tracepoint. Not saying that your change is not
> > worth it. It is, because h - softirq_vec is used by others now too.
> > 
> 
> OK, first of all, there are some serious WTFs here:
> 
> # define JUMP_LABEL_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t"
> 
> A jump instruction is one of the worst possible NOPs.  Why are we doing
> this?

This code is dynamically patched at boot time (and module load time) with a
better nop, just like the function tracer does.

> 
> The second thing that I found when implementing static_cpu_has() was
> that it is actually better to encapsulate the asm goto in a small inline
> which returns bool (true/false) -- gcc will happily optimize out the
> variable and only see it as a flow of control thing.  I would be very
> curious if that wouldn't make gcc generate better code in cases like that.
> 
> gcc 4.5.0 has a bug in that there must be a flowthrough case in the asm
> goto (you can't have it unconditionally branch one way or the other), so
> that should be the likely case and accordingly it should be annotated
> likely() so that gcc doesn't reorder.  I suspect in the end one ends up
> with code like this:
> 
> static __always_inline __pure bool __switch_point(...)
> {
> 	asm goto("1: " JUMP_LABEL_INITIAL_NOP
> 		 /* ... patching stuff */
> 		: : : : t_jump);
> 	return false;
> t_jump:
> 	return true;
> }
> 
> #define SWITCH_POINT(x) unlikely(__switch_point(x))
> 
> I *suspect* this will resolve the need for hot/cold labels just fine.

Thanks for the hint! We'll make sure to try it out. Having the ability to force
gcc to put the tracepoint in an unlikely branch is deeply needed here.

I'm a bit curious about the nop vs jump overhead comparison you are referring
to. It is an instruction latency benchmark or throughput benchmark ?

Intel's manual "Intel 64 and IA-32 Architectures Optimization Reference Manual"

http://www.intel.com/Assets/PDF/manual/248966.pdf

Page C-33 (or 577 in the pdf)

"7. Selection of conditional jump instructions should be based on the
    recommendation of section Section 3.4.1, “Branch Prediction Optimization,” to
    improve the predictability of branches. When branches are predicted
    successfully, the latency of jcc is effectively zero."

So it mentions "jcc", but not jmp. Is there any reason for jmp to have a higher
latency than jcc ?

In this manual, the latency of predicted jcc is therefore 0 cycle, and its
throughput is 0.5 cycle/insn.

NOP (page C-29) is stated to have a latency of 0.5 to 1 cycle/insn (depending on
the exact HW), and throughput of 0.5 cycle/insn.

However, I have not found "jmp" explicitly in this listing.

So if we were executing tracepoints in a maze of jumps, we could argue that
instruction throughput is the most important there. However, if we expect the
common case to be surrounded by some non-ALU instructions, latency tends to
become the most important criterion.

But I feel I might be missing something important that distinguish "jcc" from
"jmp".

Thanks,

Mathieu


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

  parent reply	other threads:[~2010-10-19 22:41 UTC|newest]

Thread overview: 93+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-23  9:41 [PATCH v4 0/5] netdev: show a process of packets Koki Sanagi
2010-08-23  9:42 ` [PATCH v4 1/5] irq: add tracepoint to softirq_raise Koki Sanagi
2010-09-03 15:29   ` Frederic Weisbecker
2010-09-03 15:39     ` Steven Rostedt
2010-09-03 15:42       ` Frederic Weisbecker
2010-09-03 15:43     ` Steven Rostedt
2010-09-03 15:50       ` Frederic Weisbecker
2010-09-06  1:46         ` Koki Sanagi
2010-09-08  8:33   ` [tip:perf/core] irq: Add " tip-bot for Lai Jiangshan
2010-09-08 11:25     ` [sparc build bug] " Ingo Molnar
2010-09-08 12:26       ` [PATCH] irq: Fix circular headers dependency Frederic Weisbecker
2010-09-09 19:54         ` [tip:perf/core] " tip-bot for Frederic Weisbecker
2010-10-18  9:44       ` [sparc build bug] Re: [tip:perf/core] irq: Add tracepoint to softirq_raise Peter Zijlstra
2010-10-18 10:11         ` Peter Zijlstra
2010-10-18 10:26           ` Heiko Carstens
2010-10-18 10:48         ` Peter Zijlstra
2010-10-19 10:58           ` Koki Sanagi
2010-10-19 11:25             ` Peter Zijlstra
2010-10-19 13:00             ` [PATCH] tracing: Cleanup the convoluted softirq tracepoints Thomas Gleixner
2010-10-19 13:08               ` Peter Zijlstra
2010-10-19 13:22               ` Mathieu Desnoyers
2010-10-19 13:41                 ` Thomas Gleixner
2010-10-19 13:54                   ` Steven Rostedt
2010-10-19 14:07                     ` Thomas Gleixner
2010-10-19 14:28                       ` Mathieu Desnoyers
2010-10-19 19:49                         ` Thomas Gleixner
2010-10-19 20:55                           ` Steven Rostedt
2010-10-19 21:07                             ` Thomas Gleixner
2010-10-19 21:23                               ` Steven Rostedt
2010-10-19 21:48                                 ` H. Peter Anvin
2010-10-19 22:23                                   ` Steven Rostedt
2010-10-19 22:26                                     ` H. Peter Anvin
2010-10-19 22:27                                     ` Peter Zijlstra
2010-10-19 23:39                                       ` H. Peter Anvin
2010-10-19 23:45                                         ` Steven Rostedt
2010-10-20  0:43                                         ` Jason Baron
2010-10-19 22:41                                   ` Mathieu Desnoyers [this message]
2010-10-19 22:49                                     ` H. Peter Anvin
2010-10-19 23:05                                       ` Steven Rostedt
2010-10-19 23:09                                         ` H. Peter Anvin
2010-10-20 15:27                                         ` Jason Baron
2010-10-20 15:41                                           ` Mathieu Desnoyers
2010-10-25 21:54                                           ` H. Peter Anvin
2010-10-25 22:01                                             ` Mathieu Desnoyers
2010-10-25 22:12                                               ` H. Peter Anvin
2010-10-25 22:19                                                 ` H. Peter Anvin
2010-10-25 22:55                                                 ` Mathieu Desnoyers
2010-10-26  0:39                                                   ` Steven Rostedt
2010-10-26  1:14                                                     ` Mathieu Desnoyers
2010-10-19 22:04                                 ` Thomas Gleixner
2010-10-19 22:33                                   ` Steven Rostedt
2010-10-21 16:18                                     ` Thomas Gleixner
2010-10-21 17:05                                       ` Steven Rostedt
2010-10-21 19:56                                         ` Thomas Gleixner
2010-10-25 22:31                                           ` H. Peter Anvin
2010-10-19 21:45                             ` Thomas Gleixner
2010-10-19 22:14                               ` Steven Rostedt
2010-10-19 21:16                           ` David Daney
2010-10-19 21:32                             ` Jason Baron
2010-10-19 21:38                               ` David Daney
2010-10-19 21:47                             ` Steven Rostedt
2010-10-19 21:28                           ` Jason Baron
2010-10-19 21:55                             ` Thomas Gleixner
2010-10-19 22:17                               ` Thomas Gleixner
2010-10-20  1:36                                 ` Steven Rostedt
2010-10-20  1:52                                   ` Jason Baron
2010-10-25 22:32                                     ` H. Peter Anvin
2010-10-19 22:38                               ` Jason Baron
2010-10-19 22:44                                 ` H. Peter Anvin
2010-10-19 22:56                                   ` Steven Rostedt
2010-10-19 22:57                                     ` H. Peter Anvin
2010-10-19 14:46                       ` Steven Rostedt
2010-10-19 14:00                   ` Mathieu Desnoyers
2010-10-21 14:52               ` [tip:perf/core] " tip-bot for Thomas Gleixner
2010-08-23  9:43 ` [PATCH v4 2/5] napi: convert trace_napi_poll to TRACE_EVENT Koki Sanagi
2010-08-24  3:52   ` David Miller
2010-09-08  8:34   ` [tip:perf/core] napi: Convert " tip-bot for Neil Horman
2010-08-23  9:45 ` [PATCH v4 3/5] netdev: add tracepoints to netdev layer Koki Sanagi
2010-08-24  3:53   ` David Miller
2010-09-08  8:34   ` [tip:perf/core] netdev: Add " tip-bot for Koki Sanagi
2010-08-23  9:46 ` [PATCH v4 4/5] skb: add tracepoints to freeing skb Koki Sanagi
2010-08-24  3:53   ` David Miller
2010-09-08  8:35   ` [tip:perf/core] skb: Add " tip-bot for Koki Sanagi
2010-08-23  9:47 ` [PATCH v4 5/5] perf:add a script shows a process of packet Koki Sanagi
2010-08-24  3:53   ` David Miller
2010-09-07 16:57   ` Frederic Weisbecker
2010-09-08  8:35   ` [tip:perf/core] perf: Add a script to show packets processing tip-bot for Koki Sanagi
2010-08-30 23:50 ` [PATCH v4 0/5] netdev: show a process of packets Steven Rostedt
2010-09-03  2:10   ` Koki Sanagi
2010-09-03  2:17     ` David Miller
2010-09-03  2:55       ` Koki Sanagi
2010-09-03  4:46         ` Frederic Weisbecker
2010-09-03  5:12           ` Koki Sanagi

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=20101019224126.GD3519@Krystal \
    --to=mathieu.desnoyers@efficios.com \
    --cc=davem@davemloft.net \
    --cc=eric.dumazet@gmail.com \
    --cc=fweisbec@gmail.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=hpa@zytor.com \
    --cc=izumi.taku@jp.fujitsu.com \
    --cc=jbaron@redhat.com \
    --cc=kaneshige.kenji@jp.fujitsu.com \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=laijs@cn.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=nhorman@tuxdriver.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=sanagi.koki@jp.fujitsu.com \
    --cc=scott.a.mcmillan@intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox