On 17 Feb 2022, at 14:00, Ilya Maximets wrote:
> On 1/31/22 11:53, Eelco Chaudron wrote:
>> This patch checks for none offloadable ct_state match flag combinations.
>> If they exist tell the user about it once, and ignore the offload.
>>
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
>> lib/netdev-offload-tc.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index 8135441c6..f3d1d3e61 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -1478,14 +1478,14 @@ flower_match_to_tun_opt(struct tc_flower *flower,
>> const struct flow_tnl *tnl,
>> flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
>> }
>>
>> -static void
>> +static int
>> parse_match_ct_state_to_flower(struct tc_flower *flower, struct match
>> *match)
>> {
>> const struct flow *key = &match->flow;
>> struct flow *mask = &match->wc.masks;
>>
>> if (!ct_state_support) {
>> - return;
>> + return 0;
>> }
>>
>> if ((ct_state_support & mask->ct_state) == mask->ct_state) {
>> @@ -1541,6 +1541,13 @@ parse_match_ct_state_to_flower(struct tc_flower
>> *flower, struct match *match)
>> flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>> flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
>> }
>> +
>> + if (flower->key.ct_state &&
>> + !(flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) {
>> + VLOG_INFO_ONCE("For ct_state match offload to work, you must "
>> + "include +trk with any other flags set!");
>
> Why should OVS care about that? Is it a workaround for a kernel bug?
This is not a kernel TC bug, but it just does not make sense to match on ct
flags without the +trk flag.
So TC marks these as invalid arguments and refuses to take the filter.
> I mean, kernel should reject offload attempts for unsupported flag
> combinations.
Yes, the kernel reports unsupported offload attempts by returning an EINVAL.
However, if this is returned as-is, the infrastructure sees this as an error,
not an incompatibility. For unsupported it only accepts EOPNOTSUPP. My original
fix had this:
@@ -1975,6 +1975,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
add_ufid_tc_mapping(netdev, ufid, &id);
}
+ if (err == EINVAL) {
+ err = EOPNOTSUPP;
+ }
+
return err;
}
But to make it easier to debug, I decided to catch this earlier and log it.
This way, hopefully, we do not get people asking why the offload did not work.
I would prefer to keep the error message (with the check), what do you think?
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
