On 21 Feb 2022, at 13:29, Marcelo Leitner wrote:
> On Mon, Feb 21, 2022 at 12:36:22PM +0100, 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? >> 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? > > Wait wait. Tc does support matching on "-trk". What it doesn't > support, similarly to ovs, is a combination like "-trk+est", and this > is what the check above is checking for. > > Now on whether that should be EINVAL or EOPNOTSUPP, hm. I don't see a > reason for any future usage of combinations like this one. So at least > on tc level, EINVAL seems to fit as well. Yet, I don't have a strong > argument for A or B here. Well, other than that's probably more common > on UAPI to reject bogus parameters with EINVAL than anything else. Guess our emails crossed (like they always seem to do). To me it does not make sense to change the kernel side, I’m sure they will not accept 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. > > We had issues at tc that it accepted bogus/any combination of > ct_flags. That got fixed in both kernel and ovs and ovs can now probe > for which flags are supported by the running kernel. That's why the > check on ct_state_support in this function. > > This particular combination, with "-trk+something", should not be a > valid combination for ovs either (per the man page). I wonder what was > failing and if perhaps we could improve the tc code too/instead. Guess the kernel module does not care much. This was all in self-tests, can’t remember the specific tests, but I could re-run without the fix. >> '-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. > > If you mean about: > if (flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { > flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW); > flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW); > } > > I also don't see a reason for that manipulation. See my other email, not sure how OVS kernel/userspace reacts to this kind of matching, as it should not be valid. But the kernel is replying with the same EINVAL. > Best, > Marcelo > >> >> Best regards, Ilya Maximets. >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
