Re: [PATCH v2 0/4] Static calls

[Edward Cree <ecree@solarflare.com>]

On 12/12/18 18:14, Nadav Amit wrote:
> Second, (2i) is not very intuitive for me. Using the out-of-line static
> calls seems to me as less performant than the inline (potentially, I didn’t
> check).
>
> Anyhow, the use of out-of-line static calls seems to me as
> counter-intuitive. I think (didn’t measure) that it may add more overhead
> than it saves due to the additional call, ret, and so on
AIUI the outline version uses a tail-call (i.e. jmpq *target) rather than an
 additional call and ret.  So I wouldn't expect it to be too expensive.
More to the point, it seems like it's easier to get right than the inline
 version, and if we get the inline version working later we can introduce it
 without any API change, much as Josh's existing patches have both versions
 behind a Kconfig switch.

> I tried to avoid reading to
> compared target from memory and therefore used an immediate. This should
> prevent data cache misses and even when the data is available is faster by
> one cycle. But it requires the patching of both the “cmp %target-reg, imm”
> and “call rel-target” to be patched “atomically”. So the static-calls
> mechanism wouldn’t be sufficient.
The approach I took to deal with that (since though I'm doing a read from
 memory, it's key->func in .data rather than the jmp immediate in .text) was
 to have another static_call (though a plain static_key could also be used)
 to 'skip' the fast-path while it's actually being patched.  Then, since all
 my callers were under the rcu_read_lock, I just needed to synchronize_rcu()
 after switching off the fast-path to make sure no threads were still in it.
I'm not sure how that would be generalised to all cases, though; we don't
 want to force every indirect call to take the rcu_read_lock as that means
 no callee can ever synchronize_rcu().  I guess we could have our own
 separate RCU read lock just for indirect call patching?  (What does kgraft
 do?)

> Based on Josh’s previous feedback, I thought of improving the learning using
> some hysteresis. Anyhow, note that there are quite a few cases in which you
> wouldn’t want optpolines. The question is whether in general it would be an
> opt-in or opt-out mechanism.
I was working on the assumption that it would be opt-in, wrapping a macro
 around indirect calls that are known to have a fairly small number of hot
 targets.  There are plenty of indirect calls in the kernel that are only
 called once in a blue moon, e.g. in control-plane operations like ethtool;
 we don't really need to bulk up .text with trampolines for all of them.

-Ed