Re: [PATCH v2 0/4] Static calls

Edward Cree
2018-12-12T18:33:11Z

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