On 2/21/22 10:57, Eelco Chaudron wrote:
>
>
> 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.
Should we fix the return value in the kernel then?
I mean, -trk is a valid match criteria, and if tc doesn't support it,
it should say that it doesn't support it, instead of complaining that
arguments are invalid. This is misleading, unless that is documented
somewhere as invalid tc configuration. Is it?
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?
Code snippet above is a bit overboard as it converts every error into
EOPNOTSUPP, and that doesn't make much sense. For the change in the
current patch, I think it will also reject plain +est match or something
similar, because it checks the key, but not the mask.
All in all, all combinations of flags that OVS tries to offload should
be valid (unless there is an OVS bug in the flow translation layer).
And we can't make assumptions on why user created this kind of OF pipeline.
If we want to forbid some combination of matches, we should do that at
the point of OpenFlow flow insertion.
'-trk' seems to be a very popular match that will be used in almost every
pipeline, so the INFO message above will be printed always on every
setup with ct and offload enabled, so I'm not sure how much value it has.
'extack' message should, probably, be the right way to receive the
explanation why the certain flow is not offloaded.
The message also seems to be a bit aggressive, especially since it will
almost always be printed.
As a temporary solution we may convert the error to EOPNOTSUPP, but for
this case only, i.e.
err == EINVAL
&& !(flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)
&& flower->mask.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED
And maybe print a debug level message.
What do you think?
And while we're here, I'm not sure why we need to check the +est+new
combination in that code either.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
