+Cc Wenxu, Paul and netdev On Tue, Feb 22, 2022 at 10:33:44AM +0100, Eelco Chaudron wrote: > > > 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 had meant to update the kernel instead. As Ilya was saying, as this is dealing with masks, the validation that tc is doing is not right. I mean, for a connection to be in +est, it needs +trk, right, but for matching, one could have the following value/mask: value=est mask=est which means: match connections in Established AND also untracked ones. Apparently this is what the test is triggering here, and the patch above could lead to not match the 2nd part of the AND above. When fixing the parameter validation in flower, we went too far. Marcelo > } > > 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
