Re: [PATCH net-next v1 00/11] net: flow_dissector: opt-in byte-identical fast paths for common shapes

Dave Seddon wrote:
> Thanks Willem, for the quick response and the feedback.
> 
> I think your point about where to draw the protocol boundary is a good one.
> 
> Eth + IPv4/IPv6 + TCP/UDP is clearly the common case. Beyond that, choosing
> what belongs in-kernel becomes increasingly subjective: VLAN/QinQ, PPPoE,
> MPLS, GRE, GTP-U, etc. all have substantial deployments, but that's also an
> argument for leaving those cases to the BPF flow dissector rather than
> continuing to grow a parallel parser.
> 
> On the +2500 lines, most of that isn't actually the fast-path parser:
> 
>     01  +75    gate BPF lookup behind a static key
>     02  +302   Eth + IPv4/IPv6 + TCP/UDP fast path
>     03  +180   + VLAN / QinQ
>     04  +95    + PPPoE
>     05  +103   + MPLS
>     06  +134   + IP-in-IP (4in4 / 6in4 / 4in6)
>     07  +121   + GRE
>     08  +270   per-shape counters (/proc/net)
>     09  +37    bound tunnel recursion
>     10  +1102  KUnit fast/slow-path equivalence test
>     11  +137   Documentation
>         ------
>         ~+2500
> 
> The KUnit test is therefore a large fraction of the diff. I don't want to
> dismiss that as "just tests" -- it is code that has to be maintained -- but
> its purpose is specifically to make maintaining two implementations safer:
> the same corpus is dissected through both paths and the resulting flow_keys
> are compared byte-for-byte.
> 
> More importantly, if I take your protocol-selection concern to its logical
> conclusion, most of this series disappears.
> 
> A v2 could contain only:
> 
> 1. the static-key optimization for the existing BPF lookup; and
> 2. one fast path for Eth + IPv4/IPv6 + TCP/UDP.
> 
> That is roughly:
> 
>     +377 / -50 across 5 files
> 
> plus whatever reduced KUnit coverage we decide is appropriate for that one
> shape.
> 
> I think there is a useful distinction between that case and the less common
> protocols. A network operator or vendor with a specialized encapsulation can
> reasonably deploy a BPF flow dissector. Requiring BPF in order for ordinary
> hosts to optimize the overwhelmingly common Ethernet/IP/TCP/UDP case is a
> higher bar, especially since users of RPS/RFS, fq/fq_codel/cake and bonding
> benefit indirectly through skb_get_hash() without necessarily knowing that
> the flow dissector is involved.
> 
> The performance result is also fairly consistent across architectures. In
> the isolated Eth + IPv4 + TCP benchmark the straight-line path reduced
> dissection time by 47-55% across x86, ARM and RISC-V. With all shapes
> compiled in, the Eth/IP path still improved the tested CPUs, although by a
> wider 4.7-31.6% range.
> 
> I agree that duplicating parsing logic has a maintenance cost. The reason I
> think the single common shape may still be a reasonable trade is that the
> surface is small, the fast path is deliberately allowed to bail out to the
> generic dissector whenever the packet doesn't match, and equivalence with the
> generic path can be continuously tested rather than assumed.
> 
> For the other protocols, I've implemented the same idea as a loadable BPF
> flow dissector:
> 
> https://github.com/randomizedcoder/flow_dissector_ebpf
> 
> That seems like a better home for experimenting with specialized shapes
> without growing the in-kernel parser.
> 
> So rather than trying to justify seven in-kernel fast paths, I'd propose
> shrinking v2 to the static-key BPF lookup optimization plus the single
> Eth + IPv4/IPv6 + TCP/UDP fast path, with a correspondingly smaller
> equivalence test.
> 
> Would that reduced series be worth posting for review?

Mine is just one opinion and not the most important one at that.

Existing non-linear code can also be optimized quite well, e.g., with
direct-call optimizations to indirect calls, FDO, etc. That may be a
more promising path than logic duplication.

That said, showing a significant performance benefit end-to-end for a
representative workload benchmark may help. Say, a on a TCP_RR
transport test. It is harder to judge the real world impact of the
flow dissection micro-benchmark on its own. With transport level
improvement, a single fast patch could warrant a look.

And same independently for the BPF static-key. BPF hooks have been
highly optimized by default, I'm a bit skeptical without data that
is worthwhile.

But I think the bar would be high.

In my ideal world we would ship a BPF program with the kernel,
deprecate the whole C parser and have the BPF parser be autoloaded at
boot.