Re: [PATCH] bpf: Tweak BPF jump table optimizations for objtool compatibility

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH] bpf: Tweak BPF jump table optimizations for objtool compatibility
Date: Tue, 5 May 2020 10:43:00 -0700	[thread overview]
Message-ID: <20200505174300.gech3wr5v6kkho35@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20200502192105.xp2osi5z354rh4sm@treble>

On Sat, May 02, 2020 at 02:21:05PM -0500, Josh Poimboeuf wrote:
> 
> Ideally we would get rid of that label and just change all the 'goto
> select_insn' to 'goto *jumptable[insn->code]'.  That allows objtool to
> follow the code in both retpoline and non-retpoline cases.  It also
> simplifies the code flow and (IMO) makes it easier for GCC to find
> optimizations.

No. It's the opposite. It's not simplifying the code. It pessimizes
compilers.

> 
> However, for the RETPOLINE=y case, that simplification actually would
> cause GCC to grow the function text size by 40%.  

It pessimizes and causes text increase, since the version of gcc
you're testing with cannot combine indirect gotos back into direct.

> I thought we were in
> agreement that significant text growth would be universally bad,
> presumably because of i-cache locality/pressure issues.  

No. As I explained before the extra code could give performance
increase depending on how branch predictor is designed in HW.

> Or, if you want to minimize the patch's impact on other arches, and keep
> the current patch the way it is (with bug fixed and changed patch
> description), that's fine too.  I can change the patch description
> accordingly.
> 
> Or if you want me to measure the performance impact of the +40% code
> growth, and *then* decide what to do, that's also fine.  But you'd need
> to tell me what tests to run.

I'd like to minimize the risk and avoid code churn,
so how about we step back and debug it first?
Which version of gcc are you using and what .config?
I've tried:
Linux version 5.7.0-rc2 (gcc version 10.0.1 20200505 (prerelease) (GCC)
CONFIG_UNWINDER_ORC=y
# CONFIG_RETPOLINE is not set

and objtool didn't complain.
I would like to reproduce it first before making any changes.

Also since objtool cannot follow the optimizations compiler is doing
how about admit the design failure and teach objtool to build ORC
(and whatever else it needs to build) based on dwarf for the functions where
it cannot understand the assembly code ?
Otherwise objtool will forever be playing whackamole with compilers.