linux-kernel - Re: [PATCH] tracing: Cleanup the convoluted softirq tracepoints

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Oct 2010 14:48:27 -0700
From:	"H. Peter Anvin" <hpa@...or.com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Koki Sanagi <sanagi.koki@...fujitsu.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>,
	Frederic Weisbecker <fweisbec@...il.com>,
	nhorman@...driver.com, scott.a.mcmillan@...el.com,
	laijs@...fujitsu.com, LKML <linux-kernel@...r.kernel.org>,
	eric.dumazet@...il.com, kaneshige.kenji@...fujitsu.com,
	David Miller <davem@...emloft.net>, izumi.taku@...fujitsu.com,
	kosaki.motohiro@...fujitsu.com,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	"Luck, Tony" <tony.luck@...el.com>
Subject: Re: [PATCH] tracing: Cleanup the convoluted softirq tracepoints

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?

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.

	-hpa

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/