On 8/26/25 2:31 PM, Yael Chemla wrote:
> 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?
I don't think so. We can change OVS to avoid matching on these flags
when it is not necessary, but datapath implementation should not ignore
things in general, especially since matching on these bits is exposed
not only through OVS but is also available for users setting up TC
flower directly.
Is it a limitation in the hardware? Seems strange that it can match
on the key flag, but can't match on df bit. Or is the key flag also
mostly just ignored?
>
>> 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.
How are you probing this? I guess it's not by trying to install flows
with skip_sw, as you mentioned above that it's not enough, but I'm not
sure how the probing can be done otherwise.
>>
>> 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.
"can be made", i.e. it was not previously discussed, so someone needs
to make that argument. The base idea would be that these flags are not
exposed through OpenFlow today and OVS doesn't make a lot of decisions
based on these two flags. At the same time the argument will fall apart
as soon some of these conditions are no longer true, in which case we
still need the datapath to behave correctly, i.e. reject the flows that
it can't properly support.
>
>> 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
