Hi, Yael. Thanks for the patch.
On 7/23/25 10:13 PM, Yael Chemla wrote:
> Commit aee4f9aec ("netdev-offload-tc: Match against tunnel flags
> if supported") added support to match against 'csum' and 'df' tunnel
> flags in TC, assuming they’re only set when actually needed. But
> the code in tnl_wc_init() always sets these flags by default,
> without checking if HW offload is enabled or if the flow explicitly
> requests them. This breaks HW offload, since mlx5 (and other NICs)
> do not support offloading when these flags are set.
>
> To work around this, users are currently have to add the following
> options to their flows "options:df_default=false options:csum=false".
> This patch helps by adjusting the wildcard mask according to actual
> tunnel config. If 'csum' or 'df' are disabled, the mask will no
> longer include them.
These are the fields of the packet header and they nave nothing to do
with the configuration of the tunnel port on the destination. Instead,
they depend on the tunnel configuration on the traffic source. So this
doesn't make a lot of sense to match or not match based on the
configuration of the destination port.
>
> Note that the decision to match these flags depends on
> `enc_flags_support`, which is probed once via
> `probe_enc_flags_support()`. In most cases, this probe is performed
> against the ovs-system device with `tc_policy=SKIP_HW`, which causes
> the probe to succeed regardless of whether the underlying hardware
> can actually offload these flags. Therefore, relying solely on
> `enc_flags_support` is insufficient and can result in incorrect
> behavior. This patch works around the issue by ensuring that even
> if support is globally enabled, the wildcard mask reflects the actual
> per-port tunnel configuration to avoid unnecessary matches that would
> break hardware offload.
Again, this match represents the actual packet header that depends on
the configuration of the tunnel port on the traffic source.
Can this be resolved on a kernel level, i.e. in the driver? These are
just packet header fields, so it should be possible for the NIC to set
them. E.g. the csum bit should be set if the udphdr->check != 0.
From OVS perspective, kernel reports that these fields are supported in
TC, so we use them, because that's the correct thing to do. There is
no real way to probe the actual card, especially for tunnel ports, as
offloading is happening through the bridge port instead.
There is an argument that can be made that OVS doesn't really need to
match on these two bits, but it's a different argument with a different
implementation. And we may need to match on these bits in the future.
Beside the patch being corrupted, as Aaron mentioned, it is also missing
some unit tests for the change.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
