On 2/21/22 13:34, Eelco Chaudron wrote:
>
>
> On 21 Feb 2022, at 12:36, Ilya Maximets wrote:
>
>> 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 <echau...@redhat.com>
>>>>> ---
>>>>> 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?
>
> Well in practice we supply the wrong arguments, as this is not a valid
> scenario, so EINVAL is ok.
>
>> 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 valid, as the bit value included with the mask, is 0, and this is not
> creating the warning.
> It will if we do -trk +new
>
>> This is misleading, unless that is documented
>> somewhere as invalid tc configuration. Is it?
>
> Don’t think it is, it makes perfect sense, as we should never have -trk and
> any other flag.
> Because if you are not tracking you can not have this state.
>
> For fail-safe you normally do “if (flower->key.ct_state &
> flower->mast.ct_state && ….” but we know that the above code will only set
> the bit if the mask is also set.
>
>
>> 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.
>>
>
> Yes, that is why I did not follow through.
>
>> 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.
>
> Yes, I agree, but this is how it is now, so maybe blocking this might render
> existing deployments no longer working.
> I know it will fail a hand full of self-tests ;)
>
>> '-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.
>
> Don’t think this is true, it will only print if +trk and any other flags are
> set.
> Guess this is where the miscommunication is.>
>> The message also seems to be a bit aggressive, especially since it will
>> almost always be printed.
Yeah. I missed the fact that you're checking for zero and flower->key.ct_state
will actually mark existence of other flags. So, that is fine.
However, I'm still not sure that the condition is fully correct.
If we'll take a match on '+est' with all other flags wildcarded, that will
trigger the condition, because 'flower->key.ct_state' will contain the 'est'
bit,
but 'trk' bit will not be set. The point is that even though -trk+est is not
a valid combination and +trk+est is, OVS may in theory produce the match with
'est' bit set and 'trk' bit wildcarded. And that can be a correct
configuration.
Or am I missing something else?
>>
>> 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?
>
> See above, I think the existing code is fine, so we do not try to offload it
> to TC.
>
>> And while we're here, I'm not sure why we need to check the +est+new
>> combination in that code either.
>
> Because according to the kernel these two flags are mutually exclusive, so
> you can not set a filter for both. A connection can be new and established at
> the same time, not sure why they choose to ignore the new state, guess the
> kernel module ignores this also.
>
> Guess I could also do a similar fix, i.e. force the +trk flag when not set?
> What do you think!? I did not test this, so it might bring new side effects ;)
>
>
>> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
