On Mon, Sep 23, 2019 at 09:55:27PM -0400, Andrea Arcangeli wrote: > This commit I reverted adds literally 3 inlines called by 3 functions, > in a very fast path, how many bytes of .text difference did you expect > by dropping some call/ret from a very fast path when you asked me to > test it? I mean it's just a couple of insn each. > > I thought the question was if gcc was already inlining without the > hint or not or if it actually grew in size in case I got it wrong and > there were many callers and it was better off not inline, so now I > don't get what was the point of this test if with the result that > confirms it's needed, the patch should be dropped. > > It's possible that this patch may not be relevant anymore with the > rename in place of the vmx/svm functions, but if this patch is to be > dropped with the optimal result, then I recommend you to go ahead and > submit a patch to drop __always_inline from the whole kernel because > if it's not good to use it here in a extreme fast path like > handle_external_interrupt and handle_halt, then I don't know what > __always_inline is good for anywhere else in the kernel. Thinking more at this I don't think the result of the size check was nearly enough to come to any conclusion. The only probably conclusion one can take is that if the size didn't change it was a fail, because there would be high probability that it wouldn't be beneficial because it was a noop (even that wouldn't be 100% certain). One needs to look at why it changed to take any conclusion, and the reason it got smaller is that all dynamic tracing for ftrace was dropped, the functions were already inlined fine in the RETPOLINE case. Those are tiny functions so it looks a positive thing to make them as small as possible, there are already proper tracepoints in kvm_exit/kvm_entry for bpf tracing all all KVM events so there's no need of the fentry in those tiny functions with just an instruction that are only ever compiled as static because of the pointer to function. This however also means it helps the CONFIG_RETPOLINE=n case and it doesn't help at all the CONFIG_RETPOLINE=y case, so it's fully orthogonal to the patchset and can be splitted off but I think it's worth it. Thanks, Andrea
