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

[Alexei Starovoitov]

On Fri, May 01, 2020 at 02:22:04PM -0500, Josh Poimboeuf wrote:
> On Fri, May 01, 2020 at 12:09:30PM -0700, Alexei Starovoitov wrote:
> > On Thu, Apr 30, 2020 at 02:07:43PM -0500, Josh Poimboeuf wrote:
> > > Objtool decodes instructions and follows all potential code branches
> > > within a function.  But it's not an emulator, so it doesn't track
> > > register values.  For that reason, it usually can't follow
> > > intra-function indirect branches, unless they're using a jump table
> > > which follows a certain format (e.g., GCC switch statement jump tables).
> > > 
> > > In most cases, the generated code for the BPF jump table looks a lot
> > > like a GCC jump table, so objtool can follow it.  However, with
> > > RETPOLINE=n, GCC keeps the jump table address in a register, and then
> > > does 160+ indirect jumps with it.  When objtool encounters the indirect
> > > jumps, it can't tell which jump table is being used (or even whether
> > > they might be sibling calls instead).
> > > 
> > > This was fixed before by disabling an optimization in ___bpf_prog_run(),
> > > using the "optimize" function attribute.  However, that attribute is bad
> > > news.  It doesn't append options to the command-line arguments.  Instead
> > > it starts from a blank slate.  And according to recent GCC documentation
> > > it's not recommended for production use.  So revert the previous fix:
> > > 
> > >   3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> > > 
> > > With that reverted, solve the original problem in a different way by
> > > getting rid of the "goto select_insn" indirection, and instead just goto
> > > the jump table directly.  This simplifies the code a bit and helps GCC
> > > generate saner code for the jump table branches, at least in the
> > > RETPOLINE=n case.
> > > 
> > > But, in the RETPOLINE=y case, this simpler code actually causes GCC to
> > > generate far worse code, ballooning the function text size by +40%.  So
> > > leave that code the way it was.  In fact Alexei prefers to leave *all*
> > > the code the way it was, except where needed by objtool.  So even
> > > non-x86 RETPOLINE=n code will continue to have "goto select_insn".
> > > 
> > > This stuff is crazy voodoo, and far from ideal.  But it works for now.
> > > Eventually, there's a plan to create a compiler plugin for annotating
> > > jump tables.  That will make this a lot less fragile.
> > 
> > I don't like this commit log.
> > Here you're saying that the code recognized by objtool is sane and good
> > whereas well optimized gcc code is somehow voodoo and bad.
> > That is just wrong.
> 
> I have no idea what you're talking about.
> 
> Are you saying that ballooning the function text size by 40% is well
> optimized GCC code?  It seems like a bug to me.  That's the only place I
> said anything bad about GCC code.

It could be a bug, but did you benchmark the speed of interpreter ?
Is it faster or slower with 40% more code ?
Did you benchmark it on other archs ?

> When I said "this stuff is crazy voodoo" I was referring to the patch
> itself.  I agree it's horrible, it's only the best approach we're able
> to come up with at the moment.

please reword it then.