On 25/07/2025 22:02, Ilya Maximets wrote:
> 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.
>
Hi Ilya, thank you for the clarifications.
It seems we misunderstood the semantics of these options.
>>
>> 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.
>
In the case of the mlx5 driver, when hardware offload is enabled the
hardware cannot match on DF or CSUM. From your perspective, would it be
acceptable in such a case to skip matching those bits?
> 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.
Currently, probe_enc_flags_support() does not consider whether hardware
offload is enabled. We have another patch for netdev-offload-tc.c that
modifies this probing logic to explicitly test whether TC can match on
DF and CSUM.
>
> 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.
>
Could you please point me to that discussion? I’d like to review the
correspondence to better understand the context.
> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
