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 <[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? 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. > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
