On 21 Feb 2022, at 15:53, Eelco Chaudron wrote:
> On 21 Feb 2022, at 14:33, Marcelo Leitner wrote: <SNIP> >>>> 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 ;) > So I’ve been doing some experiments (and running all system-traffic tests), and I think the following fix will solve the problem by just making sure the +trk flag is set in this case on the TC side. This will not change the behavior compared to the kernel. diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 0105d883f..3d2c1d844 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1541,6 +1541,12 @@ 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)) { + flower->key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED; + flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED; + } } I will send out a v3 of this set soon with this change included. //Eelco <SNIP> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
