Re: [PATCH iproute2 0/2] tc/cls_flower: Support for ip tunnel metadata set/release/classify

[Jiri Benc <jbenc@redhat.com>]

On Mon, 21 Nov 2016 12:20:54 +0200, Amir Vadai wrote:
> $ tc filter add dev vxlan0 protocol ip parent ffff: \
>     flower \
>       enc_src_ip 11.11.0.2 \
>       enc_dst_ip 11.11.0.1 \
>       enc_key_id 11 \
>       dst_ip 11.11.11.1 \
>     action tunnel_key release \
>     action mirred egress redirect dev vnet0

I really hate the "action tunnel_key release". This just exposes the
kernel internal implementation detail (dst_metadata) to the user. Why
should the user care about explicit releasing of the tunnel key? This
should happen automatically. Users do not care about our internal
implementation.

> $ tc filter add dev net0 protocol ip parent ffff: \
>     flower \
>       ip_proto 1 \
>       dst_ip 11.11.11.2 \
>     action tunnel_key set \
>       src_ip 11.11.0.1 \
>       dst_ip 11.11.0.2 \
>       id 11 \
>     action mirred egress redirect dev vxlan0

Do you see the asymmetry? This is not called "alloc tunnel_key", and
rightly so. It's very reasonable to call this "set", as it is what the
action looks like to the user.

The only argument for the existence of an explicit "release" (we should
rather call it "unset" in such case, though) is forwarding between two
tunnels, where metadata from the first tunnel will be used for
encapsulation done by the second tunnel. Or a similar case when there's
classification based on the tunnel metadata done on the mirred
interface. Somewhat corner cases, though. If we want to support them,
then let's call the action "unset" and not "release". And in any case,
it should not be mandatory to specify it, which should be made clear
in the documentation (including examples where it is needed - basically
only when forwarding between tunnels).

 Jiri