On 21 Feb 2022, at 14:33, Marcelo Leitner wrote:
> On Mon, Feb 21, 2022 at 01:51:37PM +0100, Ilya Maximets wrote: >> 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 <[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. >> >> 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 > > Oh ow. tc flower will reject this combination today, btw. I don't know > about hw implications for changing that by now. > > https://elixir.bootlin.com/linux/latest/C/ident/fl_validate_ct_state > 'state' parameter in there is the value masked already. > > We directly mapped openflow restrictions to the datapath. > >> 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. > > I guess that means that the only possible parameter validation on > ct_state at tc level is about its length. Thoughts? > Guess I get it now also :) I was missing the wildcard bit that OVS implies when not specifying any :) I think I can fix this by just adding +trk on the TC side when we get the OVS wildcard for +trk. Guess this holds true as for TC there is no -trk +flags. I’m trying to replicate patch 9 all afternoon, and due to the fact I did not write down which test was causing the problem, and it taking 20-30 runs, it has not happened yet :( But will do it later tomorrow, see if it works in all cases ;) >> >> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
