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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Dave Seddon wrote:
> G'day,
> 
> __skb_flow_dissect() parses packet headers with a generic protocol-graph
> parser. The graph walk is what makes the dissector so flexible, but most
> machines spend that flexibility on the same traffic all day long: plain
> eth + IPv4/IPv6 + TCP/UDP, perhaps with a VLAN tag or a tunnel header in
> front. The intuition behind this series is that the common shapes can be
> dissected by straight-line code instead, without giving up the graph
> walk for everything else.
> 
> Concretely: the series adds an opt-in, static-branch-gated fast path per
> packet shape, covering the common cases and the IP-in-IP/GRE
> encapsulations. Each shape has its own gate under
> /proc/sys/net/flow_dissector/, every gate is disabled by default, and
> when a gate is off its added cost is one not-taken branch per dissect.
> The gates are manual in this series; a follow-up RFC will propose an
> optional auto mode that flips them from the measured traffic mix (via
> the patch-8 counters), so nobody has to hand-tune seven knobs
> (eth_ip, vlan, qinq, pppoe, mpls, ipip, gre). Nothing
> here depends on it -- it is where this is heading, not a prerequisite.

> Relationship to the BPF flow dissector
> ======================================
> The fast path is invoked strictly after the netns BPF flow dissector
> hook in __skb_flow_dissect(): if a program is attached and returns any
> verdict other than BPF_FLOW_DISSECTOR_CONTINUE, the function returns
> before flow_dissect_fast() is reached. Attached BPF dissectors therefore
> always take precedence, and a system running one sees bit-for-bit
> unchanged behaviour from this series (patch 1 additionally makes its
> program lookup cheaper for everyone else). Dissects fully handled by a
> BPF program are deliberately not counted in
> /proc/net/flow_dissector_stats (patch 8).
> 
> The gates are global (a static key patches code shared by every netns);
> per-netns dissector policy already has a mechanism -- the netns BPF
> flow dissector -- and it keeps full precedence here.

This series adds a lot of code: +2500 LoC.

If you want a linear fast path, the BPF dissector offers that.

I don't think the purported benefit justifies the significant new
code, I'm afraid.

The code is duplicative of existing paths, so there may be additional
maintenance cost keeping the two consistent.

And which paths justify a fast paths and which do not is highly
subjective. Why is GRE included, for instance?